Fix #6198: surface global test init/cleanup timeout/cancellation failures#8288
Fix #6198: surface global test init/cleanup timeout/cancellation failures#8288Evangelink wants to merge 2 commits into
Conversation
The InvokeGlobalInitializeMethodAsync and InvokeGlobalCleanupMethodAsync helpers already produce a TestFailedException with the appropriate timeout/cancellation message, but the callers in TestMethodInfo.Execution.cs and TestMethodInfo.Lifecycle.cs discarded the return value, so timeouts and cancellations on [GlobalTestInitialize]/[GlobalTestCleanup] methods never failed the test. Capture the returned exception and surface it through the existing failure paths (throw from the inner try in ExecuteInternalAsync; assign to testCleanupException after the finally in RunTestCleanupMethodAsync), then remove the [Ignore] attributes added in #8267 for the 11 acceptance tests tracking this bug. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes the adapter bug where timeouts/cancellations in [GlobalTestInitialize] / [GlobalTestCleanup] were being constructed but then discarded, causing those failures to not fail the test and leaving acceptance coverage skipped.
Changes:
- Surface global test initialize failures by throwing the
TestFailedException?returned fromInvokeGlobalInitializeMethodAsync. - Surface global test cleanup failures by capturing the returned
TestFailedException?fromInvokeGlobalCleanupMethodAsyncand merging it into the existing cleanup exception flow. - Unskip the previously added acceptance tests by removing the
[Ignore]attributes that referenced #6198.
Show a summary per file
| File | Description |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TimeoutWhenExpiresTests.cs | Unskips global init/cleanup timeout-expiration acceptance tests. |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TimeoutWhenCanceledTests.cs | Unskips global init/cleanup TestContext-cancellation acceptance tests. |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TimeoutCooperativeGlobalTestCancellationTests.cs | Unskips cooperative cancellation tests for global init/cleanup timeouts. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.Lifecycle.cs | Captures and surfaces global cleanup timeout/cancel failures during test cleanup. |
| src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.Execution.cs | Captures and throws global init timeout/cancel failures so they fail the test. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 2
| { | ||
| // The returned TestFailedException already carries the per-method timeout/cancellation | ||
| // message produced by FixtureMethodRunner; throwing it lets the outer catch record it | ||
| // as the test failure (HandleMethodException returns TestFailedException instances as-is). |
| globalTestCleanupException = await InvokeGlobalCleanupMethodAsync(method, timeoutInfo, timeoutTokenSource).ConfigureAwait(false); | ||
| if (globalTestCleanupException is not null) | ||
| { | ||
| // Stop on first global cleanup failure, mirroring the local test cleanup loop above. | ||
| globalTestCleanupFailingMethod = method; | ||
| break; |
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary
This PR fixes a real, well-tracked bug (#6198): the return values of InvokeGlobalInitializeMethodAsync and InvokeGlobalCleanupMethodAsync were silently discarded, so timeouts/cancellations on [GlobalTestInitialize] and [GlobalTestCleanup] methods never failed the test. The fix is minimal and targeted.
Dimension Results
| Dimension | Result |
|---|---|
| Algorithmic Correctness | |
| Threading & Concurrency | ✅ No issues — ConfigureAwait(false) used throughout, no shared mutable state added |
| Security & IPC Contract Safety | ✅ N/A |
| Public API & Binary Compatibility | ✅ No public API changes |
| Performance & Allocations | ✅ No hot-path regressions |
| Cross-TFM Compatibility | ✅ No new TFM-specific APIs introduced |
| Resource & IDisposable Management | ✅ No new disposable leaks |
| Error Handling & Diagnostics | |
| Test Quality | Outcome enum value — the Timeout→Failed regression is undetected |
| Localization | ✅ No new strings added |
| All other dimensions | ✅ N/A |
Key findings
-
Timeoutoutcome is downgraded toFailedfor global init (inline comment onExecution.cs:87): WhenglobalTestInitException(Outcome=Timeout) is thrown and caught by the existing inner catch, the outcome is unconditionally set toFailed. LocalTestInitializetimeouts are handled by a specialif (testInitializeException is TestFailedException tfe) { result.Outcome = tfe.Outcome; }fast-path inRunTestInitializeMethodAsync. The same approach should be used here. -
Same
Timeout→Faileddowngrade in cleanup + minor message redundancy (inline comment onLifecycle.cs:103): Pre-existing for local cleanup but now also affects global cleanup. A fast-pathis TestFailedExceptioncheck would fix both.
Both items are pre-existing patterns elsewhere in the codebase; this PR doesn't worsen things, but since the [Ignore] annotations were added specifically for these code paths, this is a good moment to also align the outcome reporting with the rest of the lifecycle.
Generated by Expert Code Review (on open) for issue #8288 · ● 11.1M
| // The returned TestFailedException already carries the per-method timeout/cancellation | ||
| // message produced by FixtureMethodRunner; throwing it lets the outer catch record it | ||
| // as the test failure (HandleMethodException returns TestFailedException instances as-is). | ||
| throw globalTestInitException; |
There was a problem hiding this comment.
When globalTestInitException has Outcome = Timeout (the common case for this bug fix), throwing it into the inner catch causes result.Outcome to be set to Failed instead of Timeout. The catch block's logic is:
// else branch (not OperationCanceledException):
result.TestFailureException ??= HandleMethodException(ex, realException, ...); // returns TestFailedException as-is
if (result.Outcome != UnitTestOutcome.Passed) // true (default is Failed)
{
result.Outcome = ex is AssertInconclusiveException ... ? Inconclusive : Failed; // always Failed
}This is inconsistent with local TestInitialize timeouts, where RunTestInitializeMethodAsync has a special path that preserves tfe.Outcome:
if (testInitializeException is TestFailedException tfe)
{
result.Outcome = tfe.Outcome; // correctly Timeout
result.TestFailureException = testInitializeException;
return false;
}Consider adopting the same pattern here instead of throwing. For example, after the foreach loop exit you could check if globalTestInitException is not null and return early via a setTestContextSucessful = false-style guard, or handle it directly analogously to RunTestInitializeMethodAsync.
The current tests only verify message text (AssertOutputContains("...timed out after 1000ms")), not the Outcome value, so this regression isn't caught by the test suite.
| // so the wrapping logic below produces a TestFailedException with the per-method message. | ||
| if (testCleanupException is null && globalTestCleanupException is not null) | ||
| { | ||
| testCleanupException = globalTestCleanupException; |
There was a problem hiding this comment.
When globalTestCleanupException is a TestFailedException with Outcome = Timeout, the re-wrapping code:
Exception realException = testCleanupException.GetRealException(); // returns itself (not TargetInvocationException)
UnitTestOutcome outcomeFromRealException = realException is AssertInconclusiveException
? UnitTestOutcome.Inconclusive
: UnitTestOutcome.Failed; // always Failed, never Timeout...causes the final test outcome to be Failed rather than Timeout. Separately, GetFormattedExceptionMessage() on the TestFailedException produces a message that already contains the method name and "timed out after Xms", so the final UTA_CleanupMethodThrows format wraps it as:
TestCleanup method 'GlobalTestCleanupMethod' threw exception: Test cleanup method 'GlobalTestCleanupMethod' timed out after 1000ms
This is a minor redundancy in the message (method name appears twice, "timed out" framing is inside "threw exception").
This same issue affects local TestCleanup timeouts too (pre-existing), so this PR doesn't make it worse — but it's worth tracking since adding a is TestFailedException tfe → result.Outcome = tfe.Outcome fast-path to RunTestCleanupMethodAsync (similar to RunTestInitializeMethodAsync) would fix both.
Fixes #6198.
Root cause
InvokeGlobalInitializeMethodAsync/InvokeGlobalCleanupMethodAsync(inTestMethodInfo.Lifecycle.cs) already construct aTestFailedExceptioncarrying the per-method "timed out after Xms" / "was canceled" message viaFixtureMethodRunner.RunWithTimeoutAndCancellationAsync. However, the callers inTestMethodInfo.Execution.csandTestMethodInfo.Lifecycle.csdiscarded the returned exception, so timeouts and cancellations on[GlobalTestInitialize]and[GlobalTestCleanup]methods silently never failed the test.This is the underlying bug tracked in #6198 and called out in the
[Ignore]messages added with the 11 acceptance tests in #8267.Changes
TestMethodInfo.Execution.cs(ExecuteInternalAsync): capture the returnedTestFailedException?fromInvokeGlobalInitializeMethodAsyncandthrowit. The outer catch records it as the test failure ‒HandleMethodExceptionreturnsTestFailedExceptioninstances as-is, preserving the per-method message inresult.TestFailureException.TestMethodInfo.Lifecycle.cs(RunTestCleanupMethodAsync): capture the returnedTestFailedException?fromInvokeGlobalCleanupMethodAsyncin locals declared outside the try/finally, then merge intotestCleanupExceptionafter the outer catch — only when no earlier (local) cleanup failure was recorded. Stop on first global cleanup failure, mirroring the localwhile (... && testCleanupException is null)pattern. The existingUTA_CleanupMethodThrowswrapping surfaces the per-method timeout/cancellation message in the test output.[Ignore]attributes referencing Add timeout tests for global test init/cleanup #6198 from the acceptance tests added in Add timeout tests for GlobalTestInitializeAttribute and GlobalTestCleanupAttribute #8267:TimeoutWhenExpiresTests.cs(5 tests)TimeoutWhenCanceledTests.cs(2 tests)TimeoutCooperativeGlobalTestCancellationTests.cs(4 tests)Validation
dotnet build src/Adapter/MSTestAdapter.PlatformServices/MSTestAdapter.PlatformServices.csproj -c Debug— 0 warnings, 0 errors..\build.cmd -pack— succeeded.--filter "FullyQualifiedName~Timeout": 234 / 234 passed, 0 failed, 0 skipped (previously 201 passed / 33 skipped, where the 33 skipped were the 11 new tests × 3 target frameworks).MSTestAdapter.PlatformServices.UnitTestsacross net462/net48/net8.0/net9.0/net8.0-windows: all succeeded.