Bump .NET SDK to 11.0.100-preview.7.26376.106 and nest merged TRX reports - #10277
Conversation
There was a problem hiding this comment.
Pull request overview
Bumps the pinned .NET SDK to activate RFC 018 artifact post-processing and prevents merged TRX results from being double-published in CI.
Changes:
- Updates the SDK to
11.0.100-preview.7.26376.106. - Adds cross-platform handling that logs and parks merged TRX reports.
- Integrates the parking step into Windows, Linux, and macOS pipelines.
Show a summary per file
| File | Description |
|---|---|
global.json |
Updates the pinned SDK version. |
azure-pipelines.yml |
Adds merged-TRX handling to Windows jobs. |
eng/pipelines/steps/test-non-windows.yml |
Adds merged-TRX handling to Linux/macOS jobs. |
eng/pipelines/steps/park-merged-trx.yml |
Logs and relocates merged TRX reports. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
9baffec to
9dbd93e
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (4)
docs/RFCs/018-Artifact-Post-Processing.md:461
- Section 7.13 now makes nested output a processor requirement, but Appendix A.1 still tells implementers to use
Path.Combine(outputDirectory, "merged.trx")(line 593), producing the sibling layout this section forbids. Update the illustrative output to<outputDirectory>/merged/merged.trxso the RFC is internally consistent.
Processors therefore write into a `merged/` subdirectory of the supplied output directory (`<outputDirectory>/merged/merged-<runId>.trx`). This keeps the merged artifact out of those globs by construction rather than requiring every consumer to special-case the merged file name.
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxArtifactPostProcessor.cs:60
- Before using this fixed child directory, reject a pre-existing symlink or junction.
MergeToFileAsynctreatsPath.GetDirectoryName(outputPath)as its confinement base and checks reparse points only below that base, while dispatcher validation is lexical. Consequently,<outputDirectory>/merged -> outsideredirects both report and attachment writes outside the supplied output directory and is still accepted. Please create and validate a real directory before merging.
string outputPath = Path.Combine(outputDirectory, MergedReportDirectoryName, $"merged-{runId:N}.trx");
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxArtifactPostProcessor.cs:56
- These examples are inaccurate:
PublishTestResults@2defaults to the recursive**/TEST-*.xmlpattern, not*.trx, and ReportGenerator consumes coverage reports rather than TRX test results. Arcade's explicit*.trxsetting already demonstrates the actual issue; removing the unsupported examples keeps this rationale factual.
// per-module reports, and the usual way those get consumed is a non-recursive '*.trx' glob (Arcade's
// "Publish TRX Test Results" step, the PublishTestResults@2 defaults, ReportGenerator setups, ...).
docs/RFCs/018-Artifact-Post-Processing.md:459
PublishTestResults@2does not default to*.trx; its default is the recursive**/TEST-*.xml, and ReportGenerator does not consume TRX files. Please retain the verified Arcade example but avoid presenting these tools as non-recursive TRX defaults.
This issue also appears on line 461 of the same file.
Orchestrators hand the processor an output directory, and `dotnet test` sets that to the run's `--results-directory` — the same directory that already holds the per-module reports the merge consumed. A processor that wrote its output directly there would make the merged artifact a **sibling of its own inputs**, which breaks the near-universal convention of collecting a results directory with a non-recursive glob (Arcade's "Publish TRX Test Results" step uses `*.trx`, as do the `PublishTestResults@2` defaults and typical ReportGenerator setups). Such a consumer would ingest the merged report *and* the reports it summarizes, double-counting every test.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Medium
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…orts Bumps global.json (tools.dotnet + sdk.version) to 11.0.100-preview.7.26376.106. That build carries the SDK-side artifact post-processing orchestration (dotnet/sdk#55419, backported to release/11.0.1xx-preview7 as dotnet/sdk#55453), which flowed into the VMR via '[release/11.0.1xx-preview7] Source code updates from dotnet/sdk' (dotnet/dotnet#7944), so RFC 018 phase 2 now actually runs for this repo's multi-module 'dotnet test' invocations. Enabling it surfaced a problem that is not specific to this repo. The SDK points the processor's output directory at the run's --results-directory, which already holds the per-module reports the merge consumed, so TrxArtifactPostProcessor was writing the merged report as a sibling of its own inputs. Collecting a results directory with a non-recursive '*.trx' glob is near-universal (Arcade's 'Publish TRX Test Results' step, the PublishTestResults@2 defaults, typical ReportGenerator setups), and every such consumer would ingest both the merged report and the reports it summarizes, double-counting every test. TrxArtifactPostProcessor now writes to '<outputDirectory>/merged/', which keeps the merged report out of those globs by construction instead of asking each consumer to special-case the merged file name. Nesting also keeps the report co-located with its attachments, which a CI-side move could not. MergeToFileAsync derives the attachment deployment root from the output path and records it in the TRX as a relative runDeploymentRoot, so the report and its attachment tree have to stay in the same directory for those references to resolve; writing the report into the subdirectory moves both together. Adds a regression test covering that, and asserting nothing of the merged report is left in the results-directory root. Verified end to end on this SDK: two MTP modules (23 + 2 tests) run through 'dotnet test' with a shared --results-directory produce 'merged/merged-<runId>.trx' with total=25, reported back over the existing pipe as an out-of-process file artifact, with the per-module reports left intact at the root and nothing else matching a non-recursive '*.trx' glob there. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a6c06b2-7dcb-407e-bb9e-ca0c8302fa93
…rationale Follow-up to the nesting change, addressing low-confidence review observations that hold up on inspection. 'merged' is a fixed, predictable child name, and MergeToFileAsync confines its writes to Path.GetDirectoryName(outputPath). A symlink/junction planted at that name would therefore become the confinement base itself and silently redirect both the report and its attachment tree outside the orchestrator-supplied output directory, since Directory.CreateDirectory succeeds on an existing link and the engine's existing reparse-point checks only cover components strictly below its base. Materialize the directory in the processor and return null rather than merge through a reparse point, which leaves the per-module reports untouched and keeps the never-fail-the-run invariant. Covered by a new test that plants a directory symlink and asserts nothing is written through it (NETCOREAPP only, as Directory.CreateSymbolicLink is not available on net4x). Also corrects the justification in both the code comment and RFC 018 section 7.13: PublishTestResults@2 defaults to the recursive '**/TEST-*.xml', not '*.trx', and ReportGenerator consumes coverage reports rather than TRX, so those two examples did not support the claim. Arcade's explicit testResultsFiles: '*.trx' does, and is now the only example cited. Updates the Appendix A.1 sketch, which still showed the sibling layout that 7.13 forbids. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a6c06b2-7dcb-407e-bb9e-ca0c8302fa93
bee0b88 to
d37d73c
Compare
This comment has been minimized.
This comment has been minimized.
ProcessAsync_WithTwoInputs_WritesUniquelyNamedMergedReport had grown to cover two unrelated concerns after the nesting assertions were added: where the merged report is written, and that reprocessing the same inputs in a different order reproduces it byte for byte. Splitting them keeps each test focused and makes a failure point at one concern rather than both. The extracted test is named for what it actually verifies - the inputs are reordered, not merely repeated - and additionally asserts the output path is unchanged, since the run id is derived from the ordered inputs and a reordering regression would otherwise surface only as a byte mismatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a6c06b2-7dcb-407e-bb9e-ca0c8302fa93
This comment has been minimized.
This comment has been minimized.
❌ Build Failure AnalysisRoot cause: The The Error (same on every leg): Fix: Regenerate all XLF files for the analyzer project by running: dotnet msbuild src/Analyzers/MSTest.Analyzers/MSTest.Analyzers.csproj /t:UpdateXlfThen commit the updated
|
|
Correcting the automated root-cause analysis above, since it has now been posted twice and the suggested fix would discard translations.
That is not where The failure is pre-existing on Actual root cause: the neutral Running #10281 ("Restore localization filters after #10268") already fixes exactly this, across the same file set. This PR is simply waiting on it; once it merges this branch picks it up and CI should clear. No action needed here. |
This comment has been minimized.
This comment has been minimized.
…dk-enable-artifact-post-merge
🧪 Test quality grade — PR #10277
This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Re-run with
|
Bumps the .NET SDK to a build that carries the SDK-side artifact post-processing orchestration, so RFC 018 phase 2 actually runs for this repo, and fixes a problem that enabling it surfaced.
Why now
The SDK-side orchestration (dotnet/sdk#55419) was backported to
release/11.0.1xx-preview7as dotnet/sdk#55453 and flowed into the VMR via[release/11.0.1xx-preview7] Source code updates from dotnet/sdk(dotnet/dotnet#7944). The first published SDK containing it is11.0.100-preview.7.26376.106, so no wait for rc1 is needed.Verified by scanning that SDK's
dotnet.dll:ArtifactPostProcessingManager,ArtifactPostProcessingPlanner,SupportedPostProcessorKinds,SupportedPostProcessorExtensionsLegacyandIsArtifactPostProcessingare all present (all absent from the previously pinned26366.102).The problem enabling it surfaced
The SDK points the processor's output directory at the run's
--results-directory, which already holds the per-module reports the merge consumed.TrxArtifactPostProcessorwas therefore writing the merged report as a sibling of its own inputs.Collecting a results directory with a non-recursive
*.trxglob is near-universal: Arcade's "Publish TRX Test Results" step (eng/common/core-templates/job/job.yml,testResultsFiles: '*.trx'), thePublishTestResults@2defaults, typical ReportGenerator setups. Every one of those would ingest the merged report and the reports it summarizes, double-counting every test. For this repo that also means feeding duplicated data to the--report-azdo-slow-test-history/--report-azdo-flaky-historyoptions we run against our own pipeline history.The fix
TrxArtifactPostProcessornow writes to<outputDirectory>/merged/, keeping the merged report out of those globs by construction instead of asking every consumer to special-case the merged file name. This is a one-line change to the output path plus a doc note; TRX is currently the only registeredIArtifactPostProcessor.Nesting also keeps the report co-located with its attachments, which a CI-side move could not.
MergeToFileAsyncderives the attachment deployment root from the output path and records it in the TRX as a relativerunDeploymentRoot, so the report and its attachment tree must stay in the same directory for those references to resolve. Writing the report into the subdirectory moves both together.RFC 018 gains a section (7.13) documenting the decision and its rationale.
Verification
End to end on the repo's own pinned SDK (no side-by-side SDK, no
global.jsonoverride):…DotnetTestProtocolContract.UnitTests_net8.0_x64.trx(root)…TerminalReporterContract.UnitTests_net8.0_x64.trx(root)merged/merged-4fbd4d3d….trxA non-recursive
*.trxglob at the results-directory root now sees only the two per-module reports.New unit test
ProcessAsync_NestsAttachmentDeploymentRootBesideTheMergedReportbuilds two inputs with colliding attachments, then resolves the merged TRX's recordedrunDeploymentRootthe way a consumer would (relative to the report's own directory) and asserts both attachments are readable, that the report sits inmerged/, and that neither a*.trxnor the deployment root is left in the results-directory root.Microsoft.Testing.Extensions.UnitTests: 787 total, 782 passed, 5 skipped, 0 failed. Fullbuild.cmd -c Debuggreen (0 warnings, 0 errors).Notes for reviewers
park-merged-trx.ymlpipeline step that moved the merged report after the fact. That has been dropped: Copilot's review correctly pointed out it would strand the attachment deployment root, and having CI chase the TRX's internal layout was the wrong layer. The current fix makes that class of bug impossible and touches no pipeline files.Publish Test Results foldersartifact.