Uh oh!
There was an error while loading. Please reload this page.
Add MTP server-mode cancellation E2E acceptance test - #10888
Open
Amaury Levé (Evangelink) wants to merge 10 commits into
Open
Add MTP server-mode cancellation E2E acceptance test#10888Amaury Levé (Evangelink) wants to merge 10 commits into
Amaury Levé (Evangelink) wants to merge 10 commits into
Conversation
Add an end-to-end acceptance test proving that cancelling Microsoft.Testing.Platform.ServerMode.Client.Sources'' RunTestsAsync propagates all the way into a real, executing MSTest test through TestContext.CancellationToken, and that the server/client shut down cleanly afterwards. The test launches a real MSTest MTP app in server mode via MtpServerClient, waits for an in-progress test-node update to confirm the test actually started (avoiding a timing-only race), cancels the run, and asserts the test itself observed TestContext.CancellationToken via a marker file written from inside a catch block keyed on that exact token instance. A marker file is used instead of asserting on a final test-node update over the wire: AsyncConsumerDataProcessor.ConsumeAsync intentionally stops relaying test-node updates once a run is hard-canceled via $/cancelRequest (as opposed to the cooperative IGracefulStopTestExecutionCapability path), so the final canceled-test node update is not guaranteed to reach the client. That is expected platform behavior, not a propagation gap. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 3
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 | test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — This unfiltered catch also consumes the OperationCanceledException raised when testTimeoutToken… |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — Because timeoutSource is linked to cancellationToken, this condition is true for both the local… |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — This does not verify the ShutsDownCleanly behavior: ExitAsync only sends the notification, and… |
What changed in this PR
Adds E2E coverage for MTP server-mode cancellation reaching an executing MSTest test.
Changes:
- Launches a real server-mode MSTest application.
- Verifies cancellation through an out-of-band marker file.
- Adds bounded waits and shutdown handling.
| File | Description |
|---|---|
MtpServerClientCancellationAcceptanceTests.cs | Adds the cancellation acceptance test and generated test asset. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Filter the catch around runTask.WaitAsync on runCancellation.Token so a harness-level testTimeoutToken cancellation (a real hang) is never misreported as the expected client-side run cancellation. - Fix WaitForFileAsync''s catch filter to distinguish the local timeout from outer-token cancellation, matching the existing pattern in AcceptanceTestBase.RunWindowsApplicationModelCommandAsync. - Add WaitForProcessExitAsync to actually verify the server process exits on its own after ExitAsync, instead of relying on the using block''s Dispose(), which force-kills the process if it is still alive and would let a server that ignores ''exit'' still pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — This does not verify the ShutsDownCleanly behavior: ExitAsync only sends the notification, and… View resolved comment |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — Because timeoutSource is linked to cancellationToken, this condition is true for both the local… View resolved comment |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — This unfiltered catch also consumes the OperationCanceledException raised when testTimeoutToken… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs:258
File.WriteAllTextcreates the file before its buffered contents are flushed and closed, while the parent stops waiting as soon asFile.Existsis true. The parent can therefore read an empty/partial marker and fail intermittently even though cancellation propagated. Publish the marker atomically by writing a sibling temporary file and renaming it.
File.WriteAllText(signalFilePath, $"{nameof(LongRunningCancellableTest)} observed TestContext.CancellationToken");
The parent acceptance test polls for the marker file via File.Exists, but File.WriteAllText makes the file visible before its contents are fully flushed, so the parent could observe an empty or partial marker under rare timing. Write to a sibling .tmp file and atomically rename it into place instead. Also clean up a stray .tmp file in the acceptance test''s cleanup. Addresses a "previously missed" finding from Copilot code review. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Petr Pokorny (0101)
approved these changes
Aug 31, 2026
CI build failed on all platforms with IDE0007 (use 'var' instead of explicit type) because the repo's .editorconfig treats csharp_style_var_when_type_is_apparent as an error-level rule and CancellationTokenSource.CreateLinkedTokenSource(...) is a recognized 'apparent type' factory call. Switch both local declarations to 'var'. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
🔍 Build Failure AnalysisSummary — All four Debug/Release build legs (Linux, macOS, Windows Release, Windows Debug) failed identically with the Root cause: |
| Code | Project | File:Line | Message |
|---|---|---|---|
IDE0007 | MSTest.Acceptance.IntegrationTests | MtpServerClientCancellationAcceptanceTests.cs:142 | use 'var' instead of explicit type |
IDE0007 | MSTest.Acceptance.IntegrationTests | MtpServerClientCancellationAcceptanceTests.cs:159 | use 'var' instead of explicit type |
🤖 Generated by the Build Failure Analysis workflow using (a href="(dev.azure.com/redacted) · commit 6fead25
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · auto · 96.5 AIC · ⌖ 1.98 AIC · ⊞ 13.3K · [◷]( · ◷)
Contributor
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · auto · 96.5 AIC · ⌖ 1.98 AIC · ⊞ 13.3K · ◷
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs:87
- The
in-progressupdate does not prove that the test method has started. MSTest publishes it before callingRunSingleTestAsync(TestExecutionManager.Runner.cs:113,141), and that call still creates contexts/runs initializers before invoking the method (UnitTestRunner.RunSingleTest.cs:132-238). Cancellation can therefore happen first; the method can later enter with an already-canceledTestContext.CancellationToken, write the marker, and let this test pass without proving cancellation reached an executing test. Have the asset emit a separate “body started” signal immediately beforeTask.Delay, wait for that signal here, and only then cancel.
// Wait until the server confirms the test is actually running before canceling. This avoids a
// timing-only race between "cancel" and "test has started" that a blind Task.Delay would risk.
await testStarted.Task.WaitAsync(WaitTimeout, testTimeoutToken);
The in-progress test-node update is published by MtpTestResultRecorder before RunSingleTestAsync even runs TestInitialize/constructors, let alone invokes the test method. Waiting only on that update to gate cancellation left a window where cancellation could land before the test method body - and its await Task.Delay(..., TestContext.CancellationToken) - had even started, letting the test pass without proving cancellation reached genuinely executing test code. Have the test asset write a separate "body started" marker file immediately as the first statement of the test method, before entering the try/await Task.Delay. The acceptance test now waits for both that signal and the in-progress node update before cancelling, closing the gap. Shared the atomic write-then-rename helper between both marker files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Suppressed comments (1)
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs:141
ProcessId == 0proves only that the child terminated;MtpServerProcess.ProcessIdreturns 0 for every exit, including a crash or non-zero exit. SinceExitAsynconly writes a notification, that write can also complete before the client detects that the peer has already died. The test could therefore pass when cancellation crashes the server after writing the marker. Capture aSystem.Diagnostics.Processhandle from the initial PID before cancellation, await its exit afterExitAsync, and assert thatExitCodeis zero.
await client.ExitAsync(testTimeoutToken).WaitAsync(WaitTimeout, testTimeoutToken);
await WaitForProcessExitAsync(client, WaitTimeout, testTimeoutToken);
MtpServerProcess.ProcessId returns 0 for BOTH a graceful exit and a crash, so polling it after ExitAsync could not distinguish "the server processed exit cleanly" from "the server crashed right after writing the marker file" - ExitAsync only writes a fire-and-forget notification, so its completion does not prove the peer processed it before dying. Capture a System.Diagnostics.Process handle for the launched server''s PID immediately after Launch (while it is guaranteed alive), await its exit via Process.WaitForExitAsync after sending ''exit'', and assert ExitCode == 0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
enabled auto-merge (squash)
August 31, 2026 14:34
Avoid Process.GetProcessById when asserting the server's exit code in the cancellation acceptance test. Linux does not treat that Process instance as the owner of the launched child, so ExitCode throws even after the server exits. Reuse the launched process owned by MtpServerClient instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.Platform.ServerMode.Client.Sources/Client/MtpServerProcess.cs — This does not compile for the project's netstandard2.0 target:… |
Uh oh!
There was an error while loading. Please reload this page.
Microsoft.Testing.Platform.ServerMode.Client.Sources is a source-only package that also targets netstandard2.0 for .NET Framework consumers. ExitCode/WaitForExitAsync (added in 32f1ae4 to let the acceptance test assert a clean server shutdown) called Process.WaitForExitAsync, a .NET 5+-only BCL method, which failed the netstandard2.0 build leg with IDE0007/CS-level errors on Linux CI (flagged by review). Adding the repo''s existing (but currently unpackaged) ProcessExtensions polyfill to fix this down-level build would expand this PR''s scope beyond a test-only change and touch the packaging of a shared, shipped library. Per review guidance, remove the new accessors instead and keep the acceptance test''s existing ProcessId-based polling approach (proves the server exited naturally in response to ''exit'' without requiring ownership-restricted ExitCode access), with a comment documenting why the test does not assert on exit code specifically. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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 |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — InitializeAsync is the only server/client operation here without the 60-second bound. If a… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
| src/Platform/Microsoft.Testing.Platform.ServerMode.Client.Sources/Client/MtpServerProcess.cs — This does not compile for the project's netstandard2.0 target:… View resolved comment |
Uh oh!
There was an error while loading. Please reload this page.
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 |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — using var invokes synchronous Dispose() only when this scope exits, without any deadline. This… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — InitializeAsync is the only server/client operation here without the 60-second bound. If a… View resolved comment |
Uh oh!
There was an error while loading. Please reload this page.
Prevent Windows Release CI from hanging indefinitely by bounding client initialization and moving synchronous disposal to a bounded asynchronous cleanup path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2e0816a8-bfba-488c-abb0-1517c2b4cc7b
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientCancellationAcceptanceTests.cs — using var invokes synchronous Dispose() only when this scope exits, without any deadline. This… View resolved comment |
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 freeto 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.
Why
Unit tests already prove that cancelling the source-only server-mode client's (
Microsoft.Testing.Platform.ServerMode.Client.Sources)RunTestsAsynccall emits a$/cancelRequestnotification, but nothing proved the cancellation actually reaches a running MSTest test throughTestContext.CancellationToken, or that the server/client shut down cleanly afterwards. This adds that missing end-to-end coverage.What this does
MtpServerClientCancellationAcceptanceTestslaunches a real MSTest MTP application in server mode viaMtpServerClientand:Task.Delay(5min, TestContext.CancellationToken)).in-progresstest-node update before cancelling, so the cancel is never racing against test startup (no blind sleep).RunTestsAsynctoken, which sends$/cancelRequest.TestContext.CancellationToken- not just that the client-side task was cancelled or that$/cancelRequestwas sent - via a marker file the test method writes from inside acatch (OperationCanceledException ex) when (ex.CancellationToken == TestContext.CancellationToken)block.RunTestsAsync,ExitAsync, and clientDisposeall complete within bounded waits, so a regression here fails fast instead of hanging CI.A note on the design (no product bug found)
My first attempt asserted on a final
failed/"was canceled" test-node update sent back over the wire, mirroring the existing self-cancellation tests. That doesn't work for a hard$/cancelRequestcancel:AsyncConsumerDataProcessor.ConsumeAsyncintentionally stops relaying further test-node updates once a run's cancellation token fires (its own comments describe this as letting "a cooperative consumer bail out of its own work immediately"). That's different from the cooperativeIGracefulStopTestExecutionCapabilitypath (exercised byDotnetTestPipeServerCancellationTests), which does guarantee reporting survives. Since this is intentional platform behavior rather than a propagation gap, I redesigned the test's "did the test observe cancellation" proof around an out-of-band marker file instead of relying on that channel.Verification
dotnet build test\IntegrationTests\MSTest.Acceptance.IntegrationTests\MSTest.Acceptance.IntegrationTests.csproj -c Release -f net11.0- 0 errors.--filter "Name~RunTestsAsync_WhenCanceledAfterTestStarts"- passed in ~2s, 5/5 repeated runs green.TestContext.CancellationTokenfrom the test asset's delay call to confirm the test fails loudly (clearTimeoutException) instead of passing vacuously; reverted after confirming.