Default TRX to controller-backed recovery - #10808
Merged
Amaury Levé (Evangelink) merged 9 commits intoAug 28, 2026
Merged
Conversation
- Make plain `--report-trx` use controller-backed recovery whenever the
current platform supports launching a test-host process, by having
TrxProcessLifetimeHandler/TrxEnvironmentVariableProvider require
isolation whenever TrxModeHelpers.IsTestHostControllerSupported is
true, without a `--crashdump` gate and without introducing a
`--trx-mode` option or in-process opt-out.
- Replace the direct `--crashdump` check in TrxModeHelpers with actual
controller-presence state: TrxModeHelpers.ShouldUseOutOfProcessTrxGeneration
now checks PlatformCommandLineProvider.TestHostControllerPIDOptionKey in
the child test host, instead of assuming which extension caused
isolation.
- Keep the existing in-process implementation as the automatic
compatibility fallback on browser, iOS, tvOS, and WASI, where process
restart is unavailable.
- Update TRX artifact-heading expectations ("In process" -> "Out of
process") in TrxTests.cs and TrxDataRowTests.cs to reflect the new
default, drop the now-unnecessary `--crashdump` flag from crash/timeout
acceptance tests, and add explicit coverage
(Trx_WhenOnlyReportTrxIsSpecified_UsesControllerBackedRecoveryByDefault)
proving plain `--report-trx` is controller-backed on supported
platforms.
- Add TrxModeHelpersTests covering actual controller-presence handling.
Depends on #10797 (prerequisite timeout-lifecycle work), currently at
d521a2c; this branch is based on main and does not include its
timeout changes.
Fixes #10792
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Defaults TRX reporting to controller-backed recovery on supported platforms while retaining in-process fallback elsewhere.
Changes:
- Detects controller support and child-process controller presence.
- Enables controller-backed TRX registration by default.
- Updates unit, crash-recovery, and artifact-output tests.
Show a summary per file
| File | Description |
|---|---|
TrxProcessLifetimeHandlerTests.cs |
Removes the crash-dump prerequisite. |
TrxModeHelpersTests.cs |
Tests controller-presence detection. |
TrxTests.cs |
Verifies default controller-backed reporting and recovery. |
TrxDataRowTests.cs |
Updates artifact-heading expectations. |
TrxTestApplicationLifecycleCallbacks.cs |
Uses controller-presence logic. |
TrxReportExtensions.cs |
Conditionally registers controller components. |
TrxProcessLifetimeHandler.cs |
Enables default controller recovery. |
TrxModeHelpers.cs |
Defines platform and controller detection. |
TrxEnvironmentVariableProvider.cs |
Enables controller pipe configuration. |
TrxDataConsumer.cs |
Updates TRX mode diagnostics. |
InternalAPI.Unshipped.txt |
Tracks the new internal API. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxModeHelpers.cs:31
- There is still no recovery acceptance test for the HangDump combination called out by #10792. The helper unit test supplies the controller PID directly, so it cannot detect registration/lifecycle conflicts when HangDump is what isolates and terminates the child. Add a
--hangdump --report-trxtermination test that verifies the recovered TRX, failedResultSummary, and abnormal-run diagnostic.
// Used from within the test host (child) process: rely on the controller-presence state that MTP
// actually established for this process, rather than recomputing which extension requested
// isolation. This stays true even when another extension (HangDump, --timeout, ...) is the one
// that caused the controller to be used.
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Balanced
- Add RawPlatform_TrxReport_FallsBackInProcessUnderWasi acceptance test (WasmExecutionTests.cs) asserting plain --report-trx on wasi-wasm reports its artifact "In process" (never "Out of process"), directly proving TRX falls back automatically instead of attempting controller-backed recovery on a platform that cannot launch a test-host process. - Extend WasmRuntime.RunUnderWasmtimeAsync with an optional arguments parameter so wasmtime invocations can pass extra MTP command-line options. - Document the controller-backed-by-default behavior and its reliability/startup-cost tradeoff in TrxReport's PACKAGE.md, per issue #10792 acceptance criterion 6, including measured startup overhead for a trivial run: no statistically significant difference on .NET, roughly 700-800ms on .NET Framework. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
main (via #10798 "Improve unknown command-line option guidance", merged before this branch's main sync) added two new PlatformResources.resx entries without a corresponding OneLocBuild pass, which fails CI's `--report-trx`-unrelated Build Linux/Windows checks with an "xlf is out-of-date" error from Microsoft.DotNet.XliffTasks. Regenerated via `dotnet msbuild src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj /t:UpdateXlf` per this repo's localization guidelines; no manual xlf edits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
TrxModeHelpers.IsTestHostControllerSupported hardcoded `true` for every non-NETCOREAPP build, but this assembly also ships a netstandard2.0 asset that is explicitly marked as supporting browser/iOS/tvOS/WASI. Consumers selecting that asset would register the named-pipe/process controller path unconditionally instead of the promised in-process fallback, and browser controller registration can throw. Restore the `Polyfills` import and evaluate all four OperatingSystem.Is*() checks unconditionally: under NETCOREAPP they resolve via the BCL, and under netstandard2.0/.NET Framework they resolve via the Polyfills OperatingSystem extension, which already returns constant false for these platforms on .NET Framework (which cannot run on them) while evaluating the actual runtime via RuntimeInformation.IsOSPlatform for netstandard2.0 hosts that can (e.g. Mono/MAUI). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Platform/Microsoft.Testing.Extensions.TrxReport/PACKAGE.md:25
- A hang by itself does not trigger recovery: the controller finalizes TRX only after the child exits. Please avoid implying that plain
--report-trxrecovers a still-running hung host; name the terminating mechanism, such as HangDump, instead.
On platforms that can launch a test-host process (all except browser, iOS, tvOS, and WASI), TRX uses controller-backed recovery by default: the test host still streams results and generates the report during normal execution, but a surviving controller process can recover completed results into a TRX report if the test host crashes, hangs, or is stopped by `--timeout`. Browser, iOS, tvOS, and WASI cannot launch a test-host process, so TRX automatically falls back to its original in-process implementation there — no controller-backed recovery is attempted, and no configuration is required to get this fallback.
- Files reviewed: 27/27 changed files
- Comments generated: 1
- Review effort level: Balanced
Amaury Levé (Evangelink)
marked this pull request as ready for review
August 28, 2026 08:58
Amaury Levé (Evangelink)
enabled auto-merge (squash)
August 28, 2026 08:58
Contributor
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.
✅ 22/22 dimensions clean — no findings.
Summary of changes reviewed:
- TRX report generation is now controller-backed by default on platforms that support it (decoupled from
--crashdump). - Platform guards extended from browser-only to browser + iOS + tvOS + WASI, with correct
[UnsupportedOSPlatform]/[UnsupportedOSPlatformGuard]annotations and lowercase platform names. ShouldUseOutOfProcessTrxGenerationnow checksTestHostControllerPIDOptionKey(actual controller presence) instead ofCrashDumpOptionName, correctly decoupling the child-process detection from any specific extension.IsTestHostControllerSupportedis a static cached property — no per-call overhead, thread-safe by construction.- New unit tests (
TrxModeHelpersTests), updated integration tests, and a new WASI acceptance test (RawPlatform_TrxReport_FallsBackInProcessUnderWasi) all verify the behavioral change. - XLF files contain regenerated entries for
CommandLineOptionRequiresExtensionandCommandLineOptionSuggestion(resx already present on the branch). - PACKAGE.md updated with clear documentation of the controller-backed default and its performance characteristics.
- Internal API surface declared in
InternalAPI.Unshipped.txt.
Petr Pokorny (0101)
approved these changes
Aug 28, 2026
Contributor
🧪 Expert test review — PR #10808
This advisory comment was generated automatically. Grades are heuristic
|
Contributor
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 173.7 AIC · ⌖ 1.18 AIC · ⊞ 16.9K · ◷
Trx_WhenOnlyReportTrxIsSpecified_UsesControllerBackedRecoveryByDefault only asserted the console heading text, which would still pass even if a regression printed the right heading but skipped writing the TRX file. Give the run an explicit --report-trx-filename and assert the file exists, matching the sibling tests in this file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
🧪 Expert test review — PR #10808
This advisory comment was generated automatically. Grades are heuristic
|
Contributor
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 97.1 AIC · ⌖ 1.17 AIC · ⊞ 16.9K · ◷
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxModeHelpers.cs — Android is also a supported target for this package (`Microsoft.Testing.Extensions.TrxReport.csproj:… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportExtensions.cs — This unconditional controller registration breaks --report-trx for sandboxed… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Platform/Microsoft.Testing.Extensions.TrxReport/PACKAGE.md:25
- This supported-platform list incorrectly includes Android among controller-capable platforms. The package targets Android, but Android cannot launch the child test-host process used by controller-backed mode; document Android as an in-process fallback alongside the other sandboxed targets once the predicate is corrected.
On platforms that can launch a test-host process (all except browser, iOS, tvOS, and WASI), TRX uses controller-backed recovery by default: the test host still streams results and generates the report during normal execution, but a surviving controller process can recover completed results into a TRX report if the test host crashes, hangs, or is stopped by `--timeout`. Browser, iOS, tvOS, and WASI cannot launch a test-host process, so TRX automatically falls back to its original in-process implementation there — no controller-backed recovery is attempted, and no configuration is required to get this fallback.
ShouldUseOutOfProcessTrxGeneration_ReflectsControllerSupport_WhenTestHostControllerPidOptionIsSet compared its result against TrxModeHelpers.IsTestHostControllerSupported itself, which is tautological on every CI host this project targets (net462/net472/net8.0/net9.0, none of which are browser/ios/tvos/wasi): the assertion would still pass even if both sides broke identically. Assert the fixed expected value (true) instead, since this test always runs on a supported platform. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
🧪 Expert test review — PR #10808
Note: the private helper This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Suggestions on the Files changed tab can be applied with one click. Re-run with
|
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxModeHelpers.cs — The new PID-based decision is intended to fix the HangDump combination, but no acceptance test… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxModeHelpers.cs — Android is also a supported target for this package (`Microsoft.Testing.Extensions.TrxReport.csproj:… View comment |
.NET Android cannot use the platform's default Process.Start-based launch path (TestHostControllersTestHost.ProcessLifecycle) to spawn an arbitrary child test host, even though this package targets android as a supported platform (see the csproj's SupportedPlatform entries). Before this fix, TrxModeHelpers.IsTestHostControllerSupported treated Android as controller-capable, so making --report-trx controller-backed by default (per #10792) would have regressed Android runs with a process-launch failure: previously TRX was in-process everywhere by default and only went out-of-process when --crashdump was explicitly set, so Android was never exposed to this failure mode until now. Add OperatingSystem.IsAndroid() to the same exclusion set already used for browser/ios/tvos/wasi throughout TrxModeHelpers, TrxProcessLifetimeHandler, TrxReportExtensions, and TrxTestApplicationLifecycleCallbacks, and document the fallback in PACKAGE.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxModeHelpers.cs — The new PID-based decision is intended to fix the HangDump combination, but no acceptance test… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxModeHelpers.cs — Android is also a supported target for this package (`Microsoft.Testing.Extensions.TrxReport.csproj:… View resolved comment |
Contributor
🧪 Expert test review — PR #10808
All test-quality findings previously raised by earlier automated review passes on these files (missing TRX-file assertion on This advisory comment was generated automatically. Grades are heuristic
|
Address review feedback: the new PID-based controller-backed decision in TrxModeHelpers is meant to make TRX recover correctly whenever a controller is present for any reason (HangDump, --timeout, plain --report-trx, ...), but no test exercised a HangDump termination combined with TRX. ForwardCompatibilityTests runs --hangdump --report-trx together, but only on a host that completes normally, so it never reaches the controller's crash-recovery path. Add HangDumpPlusTrxTests: a dedicated asset simulates a real hang (one test completes and is streamed to the TRX sidecar, the next sleeps well past --hangdump-timeout), asserts HangDump actually kills the host, and verifies the recovered TRX has a failed ResultSummary and contains the completed test but not the one that never finished. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
🧪 Expert test review — PR #10808
The other test-file edits in this PR ( This advisory comment was generated automatically. Grades are heuristic
|
Contributor
🧵 Parallel-safety audit — PR #10808Parallelization — assemblies containing the changed test files:
Both audited assemblies opt into
Findings: A Nothing in this PR introduces a parallel-safety hazard. Each new/changed test relies on the existing per-test asset-generation and GUID-named artifact pattern, which is already safe under Advisory only — heuristic, non-blocking. Re-run with
|
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxModeHelpers.cs — The new PID-based decision is intended to fix the HangDump combination, but no acceptance test… View resolved comment |
Amaury Levé (Evangelink)
deleted the
dev/amauryleve/trx-controller-recovery
branch
August 28, 2026 14:27
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.
Fixes #10792
Depends on #10797 (prerequisite timeout-lifecycle work; currently at
656b949fbd789cd923d6aa8bc29ea259d0b4d55b). This branch is based onmaindirectly and does not copy #10797's timeout changes — it only needs the eventual controller-finalization behavior that PR provides, not any of its diff.Summary
Run TRX in controller-backed mode whenever the platform supports a test-host controller.
--report-trxalone now consistently selects the reliable controller-backed implementation on supported platforms — no--trx-modeoption, no in-process opt-out, and no new CLI surface.This does not move normal per-test TRX generation into the controller. The test host continues streaming and generating the report; the surviving controller owns recovery when the child exits before finalization.
Changes
TrxModeHelpers.IsTestHostControllerSupportedreplaces the old--crashdumpgate as the signal used by the controller-registration side (TrxProcessLifetimeHandler,TrxEnvironmentVariableProvider,TrxReportExtensions.AddTrxReportProvider) to require/enable controller-backed TRX whenever the platform can launch a test-host process (excludes browser, iOS, tvOS, WASI).TrxModeHelpers.ShouldUseOutOfProcessTrxGeneration(used inside the test host / child process) now checks the actualPlatformCommandLineProvider.TestHostControllerPIDOptionKeycontroller-presence state instead of assuming which extension caused isolation — so it correctly reflects reality when combined with HangDump,--timeout, etc.TrxTests.csandTrxDataRowTests.csto reflect the new default, and dropped the now-unnecessary--crashdumpflag from the crash/timeout/handshake acceptance tests (plain--report-trxis now sufficient).Trx_WhenOnlyReportTrxIsSpecified_UsesControllerBackedRecoveryByDefaultas explicit acceptance coverage proving plain--report-trxgoes through the controller on supported platforms, andTrxModeHelpersTestsunit coverage for the new controller-presence logic.Validation
.\build.cmd -c Debug— warning-clean build..\build.cmd -pack -c Release— warning-clean pack.Microsoft.Testing.Extensions.UnitTestsTRX-related tests onnet8.0andnet462— all pass (154/154 and 152/152 respectively), including the newTrxModeHelpersTests.net462/net8.0/net10.0):TrxTests+TrxDataRowTests— 43/44 pass. The one failure (Trx_WhenReportTrxAndResultsDirectoryAreSpecifiedWithArtifact_ArtifactIsCopiedUnderRelativeResultsDirectoryonnet462) is a pre-existing, environment-specificPathTooLongExceptioncaused by this worktree's long checkout path exceeding WindowsMAX_PATHon .NET Framework; reproduced identically with the production-code change reverted, so it is unrelated to this PR.BrowserWasmExecution_TrxReport_RunsWithoutPlatformNotSupported— still passes, confirming the automatic in-process fallback on browser-wasm is unaffected.