Repository navigation
Add failed-test attachments to GitHub Actions reports - #12184
Amaury Levé (Evangelink) wants to merge 13 commits into
Conversation
Preserve per-test attachment associations, bound staging, and keep artifact upload under workflow control. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require an absolute staging root, cover producer changes in CI, and publish real artifact links in bounded supplementary per-test details. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add boundary, cancellation, retry, fragment and packed-consumer regressions; harden smoke package selection and truncated-manifest reporting. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Retain explicit missing-metadata assertions and validate module versus merged TRX reports without double-counting executions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tone Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Summary
I read the diff for the new GitHub Actions failed-test attachment feature. I focused on GitHubActionsAttachmentCollector, the reporter integration, and the new test-gh-attachments workflow and script. I did not build or run anything. I left one inline comment and have no blocking findings.
Collector and reporter
- The size and count limits,
CreateNewplus cleanup of partial files, atomic manifest write through a.tmpfile, filename sanitization, and cancellation propagation all look sound. - Failures in capture or manifest writing are reported as warnings and do not fail the test run.
- Only failed, non-superseded results are captured, which matches the stated scope.
- Open item: the module file counter also counts the synthetic
count-limitentry (see the inline comment).
Build, dependencies & scripts
- No dependency version changes.
- The workflow uses minimal permissions (
contents: read) andpersist-credentials: false. All actions are pinned by SHA. - Please confirm that
cancel-in-progressonpushtomainis intended, because it can cancel a post-merge validation run. Consider limiting it to pull requests.
Localization and API
- The
.resxand.xlffiles are consistent. The new internal API is declared inInternalAPI.Unshipped.txt. - I did not verify that the
.xlffiles were regenerated throughUpdateXlf, or that the--helpand--infoexpectations match the new CLI option.HelpInfoAllExtensionsTestsis updated in this PR, which suggests they do.
Tests
- The unit and acceptance coverage is broad. It includes byte-exact staging, limits, cancellation, and a round trip through the real uploaded archive.
- The test-value evidence in the decision record looks adequate. I did not re-execute it.
There was a problem hiding this comment.
🧪 Test quality review — PR #12184
Compared merge base 3a95626573dd6932505a860cbd059dae4f79a81d with head dd69b2f47dc2ab1f4b9df23c8d2d3451f21ec1cd. Evidence applies to this head only.
Scope note: this was a bounded, static triage pass. I did not complete per-test rubric grading or run the specialist parallel-safety subtask, and no tests were executed.
Test-value evidence
- New attachment collector contract:
GitHubActionsAttachmentCollectorTestshas 16 test methods covering staged bytes and manifest, duplicate basenames, long names, byte and count budgets, growth, cancellation and warning-callback failure. They look relevant to the changed contract. This is a static prediction only, with no executed red/green records. - Summary reporter and command-line option changes:
GitHubActionsSummaryReporterAttachmentTests(17 tests), the updatedGitHubActionsCommandLineProviderTestsandGitHubActionsSummaryReporterTests, plus the acceptance tests inGitHubActionsAttachmentTests(5) andHelpInfoAllExtensionsTests, appear to cover the new behavior. Their exact assertions were not individually traced, so the evidence is unverified. - No executed paired results were available for this head.
🧵 Parallel safety
STATUS: PARTIAL. The specialist task was not run. A spot check found the new unit tests use a unique per-instance temp directory (Guid-based), and grep found no DoNotParallelize, Thread.Sleep or Environment.SetEnvironmentVariable use in the new test files. The acceptance test ConcurrentModulesSharingRoot_KeepDistinctManifestsAndBytes deliberately shares a root and was not reviewed for isolation. This is not a clean result.
Per-test assessment
Per-test grades were not produced in this pass, so every changed test method is Uncertain pending the full rubric. There are no inline findings.
This advisory review is heuristic and non-blocking. Re-run with /review-tests, or /parallel-audit for the parallel-safety specialist.
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · 26.7 AIC · ⌖ 0.816 AIC · ⊞ 7.8K · ◷
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧪 Test quality review — PR #12184
Compared merge base 3a95626573dd6932505a860cbd059dae4f79a81d with head 41c23d5301d4ac807c0f4530bfe1d93b22ba1404. Evidence applies to this head, not every intermediate commit. This was a static, read-only pass: nothing was built or executed, and no inline findings are published.
Test-value evidence
- Attachment staging, limits and cancellation (
GitHubActionsAttachmentCollectorTests, ~16 methods). Static prediction: the byte-exact staging assertion, exact-limit versus limit+1 byte boundaries, the count limits, cancellation cleanup and the manifest-retention cases should catch an implementation that stages nothing or ignores a limit. The PR description reports an executed ablation forCaptureAsync_StagesExactBytesAndWritesManifest. I could not access the paired TRX/binlog records, so this is unverified here. - Reporter integration (
GitHubActionsSummaryReporterAttachmentTestsand the changedGitHubActionsSummaryReporterTests). Static prediction: the failure-kind data rows read the staged bytes back and compare them exactly. The author reports an executed ablation of the deferred-fragment case. This is likewise unverified here. - CLI option (
GitHubActionsCommandLineProviderTests). Static prediction: the option arity and visibility, the invalid-path data rows, and the check that a directory is not created or an existing file overwritten are all asserted exactly. TheHelpInfoAllExtensionsTestsexpectation edits cover the new help and info output. - Acceptance (
GitHubActionsAttachmentTests, 5 methods). These use the packed package. Static prediction: they cover opt-in, disabled and outside-CI behavior, unavailable files, and concurrent modules sharing a root. I did not trace each one end to end. - Non-test contracts (workflow, PowerShell script, docs). These are not applicable to unit-level evidence. The hosted upload and download run is pending, per the author.
- No executed paired results were available to me, so no red-green claim is made.
🧵 Parallel safety
STATUS: CLEAN. The specialist did not run analyzers, so this rests on source reading.
| Assembly | Scope |
|---|---|
| Microsoft.Testing.Extensions.UnitTests | MethodLevel |
| Microsoft.Testing.Platform.Acceptance.IntegrationTests | assembly-level method parallelization |
- The changed tests use Guid-unique temp directories.
- No static mutable state or process-wide environment-variable mutation was found.
- The acceptance
GH_ATTACHMENT_*variables are only read in the child test asset, and each run gets its own root underTestRunDirectory. - Not reviewed: the
HelpInfoAllExtensionsTests.csexpectation-string edits. - Counts: Critical=0, High=0, Warning=0, Info=0.
Per-test assessment
Per-test rubric grading was not completed method by method. Methods are grouped by class and graded from the source read. The rows therefore do not list each method.
Pseudo-mutation evidence is not included, and any survivor claim would be a static prediction rather than an executed result.
| Result | Quality | Test | Notes | How to improve |
|---|---|---|---|---|
| Pass | A (90–100) | new GitHubActionsAttachmentCollectorTests. |
Exact bytes, boundaries and cancellation outcomes are asserted. | None |
| Pass | A (90–100) | new GitHubActionsSummaryReporterTests. |
Staged bytes and the failure-kind data rows are compared exactly. | None |
| Pass | A (90–100) | new/mod GitHubActionsCommandLineProviderTests. |
Option shape, rejected paths, and no-side-effect behavior are asserted. | None |
| Pass | B (80–89) | new GitHubActionsAttachmentTests. |
Process-level tests are long, but their assertions are on observable outputs. | None |
| Uncertain | — | mod GitHubActionsSummaryReporterTests. |
These edits were not traced individually. | Trace each edited existing method to confirm its assertions are unchanged. |
This advisory review is heuristic and non-blocking.
Re-run the combined review with /review-tests, or only the parallel-safety specialist with /parallel-audit.
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · 56.7 AIC · ⌖ 0.834 AIC · ⊞ 7.8K · ◷
There was a problem hiding this comment.
🟡 Changes recommended
A FIFO attachment with no writer can block synchronously on Linux, preventing session shutdown and summary generation; the new byte-count suffix also bypasses localization resources.
2 open issues
What changed in this PR
Adds opt-in failed-test attachment support to Microsoft.Testing.Extensions.GitHubActionsReport for #12131, keeping artifact upload and authentication in the workflow rather than the test process.
Changes:
- Stages declared failed-test files with size/count limits and per-session manifests.
- Preserves attachment associations through summaries, aggregation, and retries.
- Adds workflow documentation, upload/download validation, localization resources, and unit and acceptance coverage.
Test-value evidence: The PR reports two-test green/red/green feature-ablation evidence against 6b9bbb4b0f956a32f1920e2b8e53410fa85e8da2 with retained assertion changes. Execution artifacts were not independently verified here; hosted upload/download and rendered UI validation remain pending.
| File | Description |
|---|---|
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/GitHubActionsSummaryReporterTests.cs |
Extends the shared reporter fixture. |
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/GitHubActionsSummaryReporterAttachmentTests.cs |
Tests attachment reporting, retries, and compatibility. |
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/GitHubActionsCommandLineProviderTests.cs |
Tests attachment-option validation. |
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/GitHubActionsAttachmentCollectorTests.cs |
Tests staging limits, manifests, and failure handling. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoAllExtensionsTests.cs |
Updates help and info expectations. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/GitHubActionsAttachmentTests.cs |
Adds packed-consumer attachment scenarios. |
src/Platform/SharedExtensionHelpers/SummaryReporterHelpers.cs |
Adds attachment metadata to failure details. |
src/Platform/SharedExtensionHelpers/CiRunSummaryModels.cs |
Adds optional attachment serialization models. |
src/Platform/SharedExtensionHelpers/CiRunSummaryAggregation.cs |
Carries attachments into failure summaries. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.UnknownAndBootstrapValidation.cs |
Recognizes the attachment option. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.zh-Hant.xlf |
Adds Traditional Chinese catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.zh-Hans.xlf |
Adds Simplified Chinese catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.tr.xlf |
Adds Turkish catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.ru.xlf |
Adds Russian catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.pt-BR.xlf |
Adds Brazilian Portuguese catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.pl.xlf |
Adds Polish catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.ko.xlf |
Adds Korean catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.ja.xlf |
Adds Japanese catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.it.xlf |
Adds Italian catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.fr.xlf |
Adds French catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.es.xlf |
Adds Spanish catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.de.xlf |
Adds German catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/xlf/GitHubActionsResources.cs.xlf |
Adds Czech catalog entries. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/Resources/GitHubActionsResources.resx |
Defines attachment help, labels, and warnings. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/PACKAGE.md |
Documents staging and workflow-owned upload. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/InternalAPI/InternalAPI.Unshipped.txt |
Tracks new internal attachment APIs. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsSummaryReporter.SummaryWriting.cs |
Writes manifests and isolates reporting errors. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsSummaryReporter.Snapshot.cs |
Retains attachments in failure snapshots. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsSummaryReporter.Markdown.PerProject.cs |
Passes attachments to Markdown rendering. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsSummaryReporter.cs |
Collects attachments from eligible failed results. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsFailureDetails.cs |
Renders archive-relative attachment paths. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsCommandLineProvider.cs |
Registers and validates the staging directory. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsCommandLineOptions.cs |
Defines --report-gh-attachments. |
src/Platform/Microsoft.Testing.Extensions.GitHubActionsReport/GitHubActionsAttachmentCollector.cs |
Implements bounded copying and manifest generation. |
src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/InternalAPI/InternalAPI.Unshipped.txt |
Tracks shared failure-model additions. |
docs/decisions/github-actions-test-attachments.md |
Records design choices and validation evidence. |
.github/workflows/test-gh-attachments.yml |
Adds hosted artifact upload/download validation. |
.github/scripts/test-gh-attachments.ps1 |
Verifies packed consumers, provenance, and file associations. |
Review effort: Balanced
There was a problem hiding this comment.
🧪 Test quality review — PR #12184
Compared merge base d3ce600610baaebac245acdbd4162615f5dfcaae with head cc156a9267f76a2d5c1f8712e4b1ae64b18a3db7. Evidence applies to this head only.
Test-value evidence
This is a static review. No tests, builds or mutation tools were run, and no paired execution records were available. Green CI or author claims do not count as red/green proof.
- Attachment staging and manifest (
GitHubActionsAttachmentCollector):GitHubActionsAttachmentCollectorTestshas about 17 tests. They cover exact bytes, duplicate basenames, long names, size/byte/count boundaries, unavailable files, growing files, cancellation and manifest failures. The decisive assertions look independent of the implementation. Static prediction: a wrong limit, overwrite or leaked partial file would likely be caught. Not executed. - Summary reporter attachment output (
GitHubActionsSummaryReporterAttachmentTests, about 40 cases): static prediction only, not deeply traced. - CLI option, help/info and acceptance (
GitHubActionsCommandLineProviderTests,HelpInfoAllExtensionsTests,GitHubActionsAttachmentTests): changes are expected to be protected, but not traced in detail. - Unassessed: I did not trace
IFileSystem/SystemFileSystemchanges, the shared summary-aggregation changes, the workflow/script/docs files, or the small edits inAzureDevOpsLivePublishingTests,TrxResultStreamingStoreTestsandGitHubActionsSummaryReporterTests. This was due to review budget. No gaps are claimed for them.
🧵 Parallel safety
Status: CLEAN, but coverage was partial.
| Assembly | Scope |
|---|---|
| Microsoft.Testing.Extensions.UnitTests | MethodLevel |
| Microsoft.Testing.Platform.Acceptance.IntegrationTests | assembly-level method parallelization |
- The specialist read only the three focus files: the new collector test file, the new summary reporter attachment test file and the new acceptance test file.
- Each new test uses its own Guid-named directory and per-instance mocks, with no process-wide state.
- The other 5 changed test files were not read in detail.
Per-test assessment
Per-test grading is not published. About 90 new test methods were in scope and each could not be graded individually in this run. This is an evidence gap, not a clean result. No inline findings met the publication gate. Pseudo-mutation evidence is static prediction only.
This advisory review is heuristic and non-blocking. Re-run with /review-tests, or /parallel-audit for parallel safety only.
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · ~50 AIC · ⌖ ~1 AIC · ⊞ 8K
There was a problem hiding this comment.
🟡 Changes recommended
SizeBytes is recorded before stream disposal succeeds, so a disposal failure can leave the manifest reporting staged bytes for an unavailable attachment whose destination was deleted.
1 open issue
1 resolved since last review
Review effort: Balanced
Preserve zero staged bytes for failed destination disposal and cancellation after copying. Add deterministic manifest regressions for both finalization paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧪 Test quality review — PR #12184
Compared merge base d3ce600610baaebac245acdbd4162615f5dfcaae with head 682446fb54465e4e5f543ddf631583fa2ae0171b. Evidence applies to this head only.
Scope limitation: this PR is very large (about 4.3k added lines, 8 changed test files, 33 source files). This run did a bounded static review and did not complete a full per-method grading pass. No inline findings were verified, so none are published.
Test-value evidence
- Executed: none from this workflow, which does not run code. The PR description claims a red/green comparison for
SessionFinishing_LinuxFifo_RejectsBeforeOpenAndPreservesPrimarySummaryandCaptureAsync_NonRegularSource_IsRejectedBeforeOpening(guard removed,Times.Neveropen assertions fail, then pass). I could not match that claim to accessible run records for this exact head, so treat it as an author claim. - Static prediction:
GitHubActionsAttachmentCollectorTestscovers the main collector contracts with boundary data cases: file-size limit, byte budget, per-test and module counts, metadata budget, unavailable sources, cancellation, and manifest write failure. These look sensitive to the matching production limits, but I did not trace each assertion. - Not assessed: the reporter integration tests (
GitHubActionsSummaryReporterAttachmentTests,GitHubActionsSummaryReporterTests), the acceptance tests (GitHubActionsAttachmentTests), help/info expectations,GitHubActionsCommandLineProviderTests, and the workflow/script. The--infoand--helpexpectation updates should be confirmed by running the acceptance tests after-pack.
🧵 Parallel safety
STATUS: PARTIAL. The parallel-safety specialist was not run in this budget, so coverage is incomplete and must not be read as clean. The collector tests use per-instance temporary directories and a Dispose cleanup, which looks favorable. The acceptance test ConcurrentModulesSharingRoot_KeepDistinctManifestsAndBytes deliberately shares a root and should be checked for shared generated assets.
Per-test assessment
<table>
<thead>
<tr><th>Result</th><th>Quality</th><th>Test</th><th>Notes</th><th>How to improve</th></tr>
</thead>
<tbody>
<tr>
<td>Uncertain</td>
<td>—</td>
<td>new tests in the 8 changed test files</td>
<td>Per-method grading was not completed for this large diff.</td>
<td>Re-run with <code>/review-tests</code>, or review by file.</td>
</tr>
</tbody>
</table>
This advisory review is heuristic and non-blocking. Existing-thread deduplication was not completed, so no inline comments were published. Re-run the combined review with /review-tests, or only the parallel-safety specialist with /parallel-audit.
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · ~29 AIC · ⌖ ~1 AIC · ⊞ 8K
There was a problem hiding this comment.
🔵 Needs a closer look
Maintainer validation is needed because the new Unix native interop lacks NativeAOT execution evidence and authenticated expanded GitHub summary rendering remains uninspected.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
This comment has been minimized.
This comment has been minimized.
Do not assume the pack step produced the net8.0 unit-test executable. Keep the two-test FIFO selection and execution minimum while letting dotnet test restore and build the selected host. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧪 Test quality review — PR #12184
Compared merge base d3ce600610baaebac245acdbd4162615f5dfcaae with head 6f9de78dc0e276c91b3b6ab889cdaffca5d0fe3e. Evidence applies to this head, not every intermediate commit.
Test-value evidence
Static, budget-limited assessment. Nothing was executed in this workflow.
- FIFO and non-regular source rejection:
CaptureAsync_NonRegularSource_IsRejectedBeforeOpeningandSessionFinishing_LinuxFifo_RejectsBeforeOpenAndPreservesPrimarySummaryassert that no stream is opened. Static prediction: removing the regular-file guard would be detected. The PR description reports an executed red/green result (2 failed, then 2 passed), but I did not have matching records to verify it. - Staging, limits, manifest:
CaptureAsync_StagesExactBytesAndWritesManifest,CaptureAsync_ModuleByteBudget_AllowsExactLimitButRejectsNextByte,CaptureAsync_PerTestCountLimit_StagesTenAndRecordsSkippedFileandCaptureAsync_ModuleCountLimit_StopsAtOneThousandAndMarksManifestTruncatedassert exact bytes, names, boundary values and manifest fields. Static prediction: likely detected. - Command-line option and help/info:
GitHubActionsCommandLineProviderTestsandHelpInfoAllExtensionsTestscover the option. Static prediction: likely detected. - Not assessed or not applicable: the workflow, script, documentation and
.xlffiles are not applicable. Reporter rendering of the attachment links is only covered by static reading, and the rendered GitHub UI is a validation gap that the PR already discloses.
🧵 Parallel safety
STATUS: PARTIAL. The specialist task was not launched. A manual scan of the new test files found no changed parallelization configuration, no Environment.SetEnvironmentVariable, no Directory.SetCurrentDirectory and no mutable statics. The unit-test classes use per-instance GUID temp directories. This is not a clean verdict for the whole assembly.
Per-test assessment
Pseudo-mutation evidence is not enumerated below. Any such evidence would be a static prediction, not an executed mutation result. The new test files contain about 78 test methods across the changed test files (GitHubActionsAttachmentCollectorTests 19, GitHubActionsSummaryReporterAttachmentTests 19, GitHubActionsCommandLineProviderTests 34, acceptance 5, plus modified existing tests). I did not do a per-method grade pass, so I am not publishing per-row results.
<table>
<thead>
<tr><th>Result</th><th>Quality</th><th>Test</th><th>Notes</th><th>How to improve</th></tr>
</thead>
<tbody>
<tr>
<td>Uncertain</td>
<td>—</td>
<td>new <code>GitHubActionsAttachment*Tests</code> (all methods)</td>
<td>Per-method grading was not completed within this run's budget.</td>
<td>Re-run with <code>/review-tests</code> for a per-method grading pass.</td>
</tr>
</tbody>
</table>
No verified, actionable inline findings were identified, and dedup context from prior reviews did not show an existing report for this head.
<sub>This advisory review is heuristic and non-blocking.
Re-run the combined review with /review-tests, or only the parallel-safety specialist with /parallel-audit.</sub>
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · ~32 AIC · ⌖ ~1 AIC · ⊞ 8K
This comment has been minimized.
This comment has been minimized.
|
Final test-triage resolution — Azure Pipelines build 1662929 Resolution: inconclusive. Evidence is incomplete (1 artifact download failure and the download budget was exhausted). Historical comparison is also incomplete, so I can't claim these failures have no prior occurrences. No issue was created. Build 1662929 (
No crash or hang diagnostics were collected, and there were no slow-test regressions. None of these failures shows a fail-then-pass recovery, so I'm not calling any of them flaky. Next steps:
|
Use .NET 10 for the FIFO host, generated smoke consumers and runtime/evidence paths, matching the MTP 3.0 supported frameworks in the CI merge base. Preserve historical validation and all execution/integrity gates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧪 Test quality review — PR #12184
Compared merge base d3ce600610baaebac245acdbd4162615f5dfcaae with head b485c8b0653c7807712d40b6b2284eceabf377ec. Evidence applies to this head, not every intermediate commit.
Test-value evidence
This review is static. It did not run code or tests.
-
Attachment staging and manifests (
GitHubActionsAttachmentCollectorTests, e.g.CaptureAsync_StagesExactBytesAndWritesManifest): static prediction that removing or corrupting staging or manifest output would fail the exact-bytes and manifest assertions. This is unverified. -
Non-regular source rejection (
CaptureAsync_NonRegularSource_IsRejectedBeforeOpening,SessionFinishing_LinuxFifo_RejectsBeforeOpenAndPreservesPrimarySummary): static prediction that removing the regular-file guard trips theTimes.Neveropen assertions. The PR description claims an executed red/green comparison (2 failed without the guard, 2 passed with it). I did not verify it, and no matching run record was accessible to this review. -
Reporter integration (
GitHubActionsSummaryReporterAttachmentTests): static prediction that dropping attachment metadata would fail the rendering and aggregation assertions. This is unverified. -
CLI option and help text (
GitHubActionsCommandLineProviderTests,HelpInfoAllExtensionsTests): static prediction that option validation and help expectations would fail if the option changed. This is unverified. -
Not applicable or not assessed:
- The workflow, the script and the docs are not executable test contracts.
- Resource
.xlffiles and theInternalAPItext files are not assessed. - The small edits to
AzureDevOpsLivePublishingTestsandTrxResultStreamingStoreTestsare not assessed. - The new
SystemFileSystem.Unix.csstat P/Invoke is covered only by the two Linux FIFO tests. Those tests are not run on other platforms.
🧵 Parallel safety
STATUS: PARTIAL. The parallel-safety-reviewer specialist was not run in this session, so this is not a clean result.
A quick manual scan of the new unit-test classes found per-instance Guid temp directories and no static mutable state. GitHubActionsAttachmentCollectorTests implements IDisposable for cleanup. I found no cross-test hazard. The acceptance tests in GitHubActionsAttachmentTests.cs were not analyzed.
Per-test assessment
Per-test grading was not performed. About 40 new test methods were identified across GitHubActionsAttachmentCollectorTests, GitHubActionsSummaryReporterAttachmentTests and GitHubActionsAttachmentTests. I did not grade or pseudo-mutate them individually, so I cannot assign a result or quality to any of them.
Coverage gaps:
- No per-method grades.
- Existing-thread deduplication was only partly done. I read the earlier automated review and found no prior report from this workflow for this head.
- No inline findings were verified, so none are published.
<sub>This advisory review is heuristic and non-blocking.
Re-run the combined review with /review-tests, or only the parallel-safety specialist with /parallel-audit.</sub>
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · ~31 AIC · ⌖ ~1 AIC · ⊞ 8K



Pull request
Summary
Fixes #12131. Adds opt-in failed-test attachments to Microsoft.Testing.Extensions.GitHubActionsReport:
--report-gh-attachments <absolute-directory>stages declared regular files with bounded size/counts and per-session test/module/retry manifests.non-regularwith no staged path. This prevents a no-writer Linux FIFO from blocking session completion and the primary summary.Native issue type: Feature. Next open milestone: MSTest 5.0 / MTP 3.0, preserving the maintainer's concurrent update. Current build/package versions remain 4.6/2.6 below.
Design and decision record
Maintained research and decisions.
GPT-6.1 Sol and Claude Opus 5.5 reviewed the plan, followed by three independent code-review/fix rounds. In-process artifact credentials, a separately shipped companion action, and deferred primary reporting were rejected: upload failure must not hide normal diagnostics.
FIFO follow-up: the filesystem abstraction now exposes metadata-only regular-file classification. Unix uses the runtime's fixed-width, normalized stat ABI, not architecture-specific libc stat layouts.
File.Exists,FileAttributesand size-zero checks cannot distinguish FIFOs from empty regular files. Producers continue to own path lifetime and must not retarget a declared path concurrently with consumption.Demonstration
Final-head hosted validation passed on ubuntu-latest at
3936f06c13a65d2354cbcc30d528cde062dc038d: standard pack, both real Linux FIFO regressions, composed smoke, official upload, the exact documented post-upload script, real download, and byte/test/file/URL verification.Actual attachment archive · reproducible validation evidence. Four staged files plus four manifests were downloaded and checked. Archive digest:
sha256:3699c1290fd0b5468743e71dbaa6d8be02c4760c746fba6f068fff0c336f0608.Reproducer:
.\.github\scripts\test-gh-attachments.ps1 -WorkDirectory <fresh-task-owned-directory>— wrapper exit 0, with 12 actual consumer executions and five intentional failed results. Fresh MSTest packages: 4.6.0-dev; platform/MSBuild/TRX/GitHubActionsReport: 2.6.0-dev; pinned SDK: 12.0.100-alpha.1.26509.101. The script checks nupkg SHA256, resolved runtime-DLL hashes, final associations and merged TRX without double-counting.The public browser showed the successful job and artifacts. Authenticated expanded summary rendering, wrapping and controls were not visually inspected because the available browser is signed out. Saved real Markdown/console transcripts are the demonstration; no substitute rendering or fabricated screenshot is claimed.
Validation and regression proof
Current FIFO fix:
3936f06c13a65d2354cbcc30d528cde062dc038d. Pinned SDK compilation of net8.0 and netstandard2.0 withTreatWarningsAsErrors=true: exit 0, no warnings. Pinned Linux .NET 8.0.33 executed all 272 focused cases: 272 passed, 0 failed/skipped. Windows executed 270 passed, with the two explicitly Linux-only cases skipped there; those skips are not Linux proof.The hosted job executes this exact focused regression command after packing — exit 0, 2 executed/passed, 0 skipped:
./.dotnet/dotnet test --project test/UnitTests/Microsoft.Testing.Extensions.UnitTests/Microsoft.Testing.Extensions.UnitTests.csproj --configuration Release --framework net8.0 --no-build --no-restore -p:EnableCodeCoverage=false -bl:artifacts/log/Release/fifo-regression-{}.binlog --filter FullyQualifiedName~GitHubActionsSummaryReporterTests.SessionFinishing_LinuxFifo_RejectsBeforeOpenAndPreservesPrimarySummary --minimum-expected-tests 2 --report-trx --report-trx-filename fifo-regression.trx --results-directory artifacts/TestResults/fifo-regressionCaptureAsync_NonRegularSource_IsRejectedBeforeOpening— passed; verifies no open, warning, null path and manifestSessionFinishing_LinuxFifo_RejectsBeforeOpenAndPreservesPrimarySummary— both direct-FIFO and symlink cases passed locally and in hosted Linux CICaptureAsync_StagesExactBytesAndWritesManifest,CaptureAsync_ModuleByteBudget_AllowsExactLimitButRejectsNextByte,ConsumeAsync_AttachmentFailure_DoesNotDropFailedRecordOrHistory,ReadAndAggregate_ProcessRetries_RenderLastFailureAndKeepEarlierArchive— included in 272-case Linux passEarlier feature validation remains applicable history: 269 cases passed on net8.0 and 269 on net462/net481 (netstandard2.0), plus 11 packed acceptance/help cases, with actual package/cache/DLL provenance verified. TRX/JUnit/CTRF exports were reconciled. The current Linux run additionally covers the FIFO fix.
Guards:
check_source_bom.py,check_action_pins.py,git diff --check, andmarkdownlint-cli2on changed docs — passed. Resource catalogs were generated with owning-projectUpdateXlf; the original localization check passed 41 guard cases and 364 catalogs. The FIFO fix adds no resource strings.Local limitations: the full Windows package verifier expects legacy UAP payloads omitted by local
SkipUapTargetFramework=true; it was not weakened or called successful. Standard Linuxbuild.sh -packpassed on the current head. An optional standalone NativeAOT probe stopped during restore because the shared drive filled; this is not NativeAOT execution proof. The import useslibSystem.Native, matching the runtime's NativeAOT direct-P/Invoke library name. Only owned optional SDK/comparison resources were removed; no global caches were cleared.Test-value evidence
FIFO red/green: retained the same two Linux cases/assertions and removed only the collector's regular-file guard in a task-owned comparison. Both executed and failed
Times.Neverassertions because one forbidden source open was attempted per case (exit 2, 2 failed, 0 skipped). The stream-open trap fails immediately rather than opening a FIFO, so no hung worker/writer or infrastructure timeout is counted as proof. Rebuilt restored production libraries were hash-verified; the identical tests then passed (exit 0, 2 passed, 0 skipped).SessionFinishing_LinuxFifo_RejectsBeforeOpenAndPreservesPrimarySummary(FIFO and symlink)CaptureAsync_StagesExactBytesAndWritesManifestSessionFinishing_DeferredFragment_RetainsAttachmentsForAggregateRenderingOriginal feature-ablation base:
6b9bbb4b0f956a32f1920e2b8e53410fa85e8da2; support patch SHA2562DF6A32E9C2267B4B1B60A0C9265070280AB88AD375655F47F7F91E1C0A620C5; production ablation SHA25695A07BC3991A486811747B55BFBD51A89074FCB107C78DECA3E74F0224EA470B. Initial null-reference and setup/disk failures were rejected as behavioral evidence. Retained test bytes, comparison patches, actual reports and logs were preserved before owned comparison cleanup.