Close SDK-side gaps in MTP artifact post-processing - #55554
Open
Evangelink wants to merge 2 commits into
Open
Conversation
Follow-up to #55419 and #55480. The remaining gaps against microsoft/testfx RFC 018 that are owned by this repo: - Skip post-processing when the run was cut short by --maximum-failed-tests or --timeout. Only Ctrl+C skipped it, so a truncated run's artifacts were merged into one authoritative-looking report that hid the truncation. The decision moves into ShouldPostProcessArtifacts so it is directly testable. - Forward --config-file to the relaunched merge host. A merge host resolves its extensions like a test host does, so a configuration file that governs which extensions load has to reach it, or the merge runs with a different extension set than the run that produced the artifacts. - Prefer a directory the elected application already writes to when --results-directory is absent, instead of whichever input sorted first. - Make the 15-minute merge-host bound overridable through DOTNET_CLI_TEST_ARTIFACT_POST_PROCESSING_TIMEOUT_SECONDS, with 0 removing it. Values the framework would reject collapse to 'no bound' rather than throwing out of the wait and failing every merge. - Print a progress line before merging. Post-processing runs after the last module exits and before the summary, so a slow merge was indistinguishable from a hung CLI. - Emit test/artifact-post-processing telemetry (RFC 7.11). Kinds and extensions outside a fixed well-known list are bucketed as 'other' so a private post-processor's identifier is never uploaded. - Guard the whole of ExecuteAsync, not just each job, so planning, the progress line and telemetry also cannot turn a finished run into a crash. A merge killed by Ctrl+C no longer counts as a failed job. Adds documentation/general/dotnet-test-artifact-post-processing.md and the telemetry event entry; the feature had no documentation at all. Also fixes MultiProjectRun_MergesTrxArtifacts, which failed on main: this suite runs under Microsoft.Testing.Platform, so its execution id leaked into the inner 'dotnet test' and made both invocations look like one execution, collapsing their merged reports onto the same name. Relates to #47613. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a41f8680-43fb-4285-b81e-ed5a2788fbd2
|
Azure Pipelines: Successfully started running 2 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Closes remaining SDK-owned gaps in Microsoft.Testing.Platform (MTP) artifact post-processing for dotnet test, aligning behavior with the intended “best-effort, never misrepresent a truncated run” contract and adding missing observability/documentation.
Changes:
- Skip artifact post-processing for truncated runs (
--maximum-failed-tests,--timeout, Ctrl+C) via a centralizedShouldPostProcessArtifactsdecision with unit coverage. - Improve merge-host behavior and UX: forward
--config-file, choose a more appropriate merged output directory, add a “merge starting” progress line, and make the merge-host timeout configurable via env var. - Add telemetry (
test/artifact-post-processing) + docs describing behavior, troubleshooting, and knobs.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Test/GivenDotnetTestBuildsAndRunsArtifactPostProcessingMTP.cs | Adds E2E coverage for “single app => no merge”, “max-failed-tests => skip merge”, --test-modules merge, and attempts to isolate execution-id behavior. |
| test/dotnet.Tests/CommandTests/Test/ArtifactPostProcessingTelemetryTests.cs | Adds unit coverage ensuring telemetry shape/duration reporting and bucketing of unknown kinds/extensions (privacy). |
| test/dotnet.Tests/CommandTests/Test/ArtifactPostProcessingPlannerTests.cs | Adds planner regressions for kind-vs-extension grouping and ensuring tagged artifacts aren’t double-routed. |
| test/dotnet.Tests/CommandTests/Test/ArtifactPostProcessingManagerTests.cs | Adds tests for merge-host argument forwarding, timeout parsing, skip decision logic, and merged output directory selection. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.zh-Hant.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.zh-Hans.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.tr.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ru.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.pt-BR.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.pl.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ko.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ja.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.it.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.fr.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.es.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.de.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.cs.xlf | Adds localization entry for the new “post-processing started” progress line. |
| src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs | Extracts merge-host argument building (incl. --config-file), and adds configurable merge timeout parsing. |
| src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs | Adds an information-message writer used to display merge-start progress. |
| src/Cli/dotnet/Commands/Test/MTP/MicrosoftTestingPlatformTestCommand.cs | Centralizes skip logic for post-processing into ShouldPostProcessArtifacts. |
| src/Cli/dotnet/Commands/Test/MTP/ArtifactPostProcessingTelemetry.cs | Implements telemetry event/property creation with bucketing to avoid uploading private identifiers. |
| src/Cli/dotnet/Commands/Test/MTP/ArtifactPostProcessingManager.cs | Adds progress line, telemetry emission, stronger outer exception guard, better output directory selection, and improves failure counting semantics. |
| src/Cli/dotnet/Commands/Test/CliConstants.cs | Introduces env var name for configuring the merge-host timeout. |
| src/Cli/dotnet/Commands/CliCommandStrings.resx | Adds the localized string for the merge-start progress line. |
| documentation/project-docs/telemetry.md | Documents the new test/artifact-post-processing telemetry event. |
| documentation/general/dotnet-test-artifact-post-processing.md | Adds end-user documentation for when post-processing runs, skip conditions, configuration, and troubleshooting. |
Copilot's findings
- Files reviewed: 26/26 changed files
- Comments generated: 2
- Parse the merge-host timeout override as long rather than int. A value above int.MaxValue chosen to mean 'a very long time' overflowed the parse and silently fell back to the 15-minute default, the opposite of what the caller asked for, while a merely large value correctly became unbounded. - Remove TESTINGPLATFORM_DOTNETTEST_EXECUTIONID from the child environment in the end-to-end tests instead of setting it to an empty string. The test framework already exposes EnvironmentToRemove, which genuinely unsets the variable, so the test no longer depends on the platform treating empty and unset alike, and it now does what its comment says. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a41f8680-43fb-4285-b81e-ed5a2788fbd2
Evangelink
enabled auto-merge
July 31, 2026 16:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #55419 (the feature) and #55480 (hardening). Artifact post-processing shipped as SDK orchestration plus a TRX post-processor, but a handful of SDK-owned gaps against microsoft/testfx RFC 018 were left open. This closes them.
A truncated run could be merged into one authoritative-looking report
Only Ctrl+C skipped post-processing. A run stopped by
--maximum-failed-testsor--timeoutstill merged whatever artifacts existed: modules that never started contributed nothing, and modules killed mid-flight wrote whatever they had. The merged report then looked like a complete run. Both cases now skip, exactly as Ctrl+C already did.The decision moved out of the inline condition into
ShouldPostProcessArtifacts, so all five reasons to skip (help, discovery, opt-out, cancellation, policy truncation) are covered by deterministic unit tests instead of only by a timing-dependent end-to-end test.The merge host did not see
--config-fileA merge host resolves and enables its extensions exactly like a test host does, so a configuration file that governs which extensions load has to reach it. Without it the merge ran with a different extension set than the run that produced the artifacts, and a post-processor disabled by configuration silently came back for the merge.
--config-fileis now forwarded alongside--diagnostic-output-directory.--results-directoryis deliberately still not forwarded: the merged output location travels in the manifest so the SDK keeps control of it even when it has to be derived. That is now stated in a comment rather than being implicit.Merged output landed in an arbitrary project's bin folder
Without
--results-directorythe output directory was the directory of whichever input sorted first. It is now the directory of an input produced by the elected application when there is one, so the merged artifact lands beside the reports it summarizes rather than inside an unrelated project's output. Path order remains the fallback, because an application can be elected purely for the kinds it advertises without having produced any input.Other gaps
DOTNET_CLI_TEST_ARTIFACT_POST_PROCESSING_TIMEOUT_SECONDS, with0removing it entirely (which is what makes attaching a debugger to a merge host possible). Values the framework would reject, above roughly 49.7 days, collapse to "no bound" instead of throwing out of the wait and failing every merge.test/artifact-post-processingevent with jobs planned/executed/failed, artifact count, kinds, extensions and wall time. Kinds and extensions are producer-chosen strings, so anything outside a fixed well-known list is reported asotherand a private post-processor's identifier is never uploaded. Runs that plan nothing emit nothing.documentation/general/dotnet-test-artifact-post-processing.mdcovering when it runs and when it is skipped, how groups and elections are computed, where merged output lands, the--no-buildcaveat, why this is unrelated to VSTest'sVSTEST_DISABLE_ARTIFACTS_POSTPROCESSING, and troubleshooting. Plus the telemetry event entry.Hardening found in review
ExecuteAsyncguarded each job but not the code around them. Planning, the progress line and the telemetry call sat outside anycatch, andExecuteAsyncis invoked with.GetAwaiter().GetResult()under atry/finallywith nocatch, so a throw from any of them could still turn a finished run into a CLI crash with a different exit code. The whole body is now guarded. Separately, a merge killed by Ctrl+C reportedjobs_failedin telemetry even though the user-facing warning was deliberately suppressed for that case;ReportFailureUnlessCancellednow returns whether it reported, and the counter follows it.Pre-existing test failure fixed
MultiProjectRun_MergesTrxArtifactswas failing onmain. I confirmed this by stashing everything and rebuilding before touching it.This suite runs under Microsoft.Testing.Platform, so its process already carries a
TESTINGPLATFORM_DOTNETTEST_EXECUTIONID. A test application only generates its own when that variable is unset, so the applications launched by the innerdotnet testadopted the harness's id and both invocations looked like the same execution. The merged report name is derived from the inputs and their execution ids, so the second run collided with the first. The test now clears the variable per invocation.Worth flagging as a possible follow-up, deliberately not changed here: this means
dotnet testpropagates an inherited execution id to every test application it starts, contrary to the platform's documented "each test application gets its own" model. It is niche and upstream-adjacent, so I did not change that hot path.Still out of scope
Coverage merging needs a post-processor in the extension that produces
.coverage(RFC Phase 3, microsoft/codecoverage); coverlet is Phase 4; de-duplicating retried tests belongs to the merge engine. Until Phase 3 ships, TRX remains the only format that consolidates, which is why #47613 stays open.Validation
build.cmd -c Debugis clean. 52/52ArtifactPostProcessing*tests pass, plus 89 neighbouring tests (TerminalTestReporter,TestApplicationHandler,TestProgressState,MTPHelpSnapshot,TestCommandParser, and the max-failed-tests, timeout and retry end-to-end tests). All 15.xlffiles were regenerated by the build rather than hand-edited.New coverage: 2 planner tests (kinds sharing an extension are not conflated; a tagged artifact is never routed through both its kind group and the extension fallback), 11 manager tests (output directory selection, timeout parsing, merge-host argument construction, the skip decision), 3 telemetry tests including one asserting no path or project name reaches any property, and 3 end-to-end tests from the RFC behavior matrix (single application produces no merged report, a run truncated by
--maximum-failed-testsis not merged,--test-modulesmerges and the merged counters equal the sum of the per-module ones).Relates to #47613.