Pull request overview

Fixes a threading bug in MSTestAdapter’s async execution pipeline where ConfigureAwait(false) caused the continuation after an async TestInitialize to resume on a thread pool thread, losing the UI-thread SynchronizationContext required by WinUI [UITestMethod] scenarios (leading to COMExceptions when creating WinUI objects).

Changes:

• Preserve the active SynchronizationContext in TestMethodInfo.ExecuteInternalAsync by removing ConfigureAwait(false) from the TestInitialize and test-method invocation awaits.
• Add a unit test that simulates UI-thread dispatch with a dedicated-thread SynchronizationContext and verifies the context is still present inside the test method after an async TestInitialize.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 2

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

The new test TestMethodInfo_AsyncTestInitialize_PreservesSynchronizationContextForTestMethod correctly simulates the WinUI UI-thread scenario and verifies the production fix. The SingleThreadedSynchronizationContextForTesting helper is well-structured and properly disposed. However, three reliability gaps were found:

1. [Flakiness – Hang Risk] await tcs.Task has no timeout. If the sync context's dedicated thread dies before setting the TCS (e.g., due to a synchronous exception in the callback), the test hangs indefinitely and blocks the CI run. Add .WaitAsync(TimeSpan.FromSeconds(30)).

2. [Flakiness – Silent Thread Death] SingleThreadedSynchronizationContextForTesting's thread loop calls d(state) without exception handling. An uncaught exception kills the thread silently, causing the TCS to never complete. The loop should catch and re-surface exceptions so the test fails clearly rather than hanging.

3. [Coverage] The PR title and description mention fixing both TestInitialize and TestCleanup, but only the TestInitialize → TestMethod execution path is tested. The TestMethod → TestCleanup path is unverified — a regression in TestCleanup's context preservation would go undetected.

Recommendations

• Wrap await tcs.Task with WaitAsync(TimeSpan.FromSeconds(30)) to bound CI hang risk.
• Add exception handling around d(state) in SingleThreadedSynchronizationContextForTesting (or in the callback passed to Post) so any synchronous throw propagates to the TCS.
• Add a sibling test that verifies SynchronizationContext.Current is preserved inside an async TestCleanup.

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

[Correctness] Comment overstates TestCleanup context preservation (inline comment)

The added comment at line 121 claims "the test method and TestCleanup run on the same thread as the SynchronizationContext." However, RunTestCleanupMethodAsync internally uses .ConfigureAwait(false) on InvokeCleanupMethodAsync, meaning async portions of TestCleanup continue on the thread pool, not on the UI thread. A user relying on this guarantee for an async [TestCleanup] that creates WinUI objects would still get a COMException. The comment should be narrowed to what is actually guaranteed: that the test method begins executing on the UI thread.

[Threading] GlobalTestInitializations loop still uses ConfigureAwait(false) (not in diff)

In ExecuteInternalAsync (~line 87, outside the diff), the loop:

foreach ((MethodInfo method, TimeoutInfo? timeoutInfo) in Parent.Parent.GlobalTestInitializations)
{
    await InvokeGlobalInitializeMethodAsync(method, timeoutInfo, timeoutTokenSource).ConfigureAwait(false);
}

runs before the now-fixed code. If any GlobalTestInitializations entry is async, .ConfigureAwait(false) discards the DispatcherQueueSynchronizationContext at that point — making the fix for RunTestInitializeMethodAsync a no-op. For WinUI test classes that define async assembly-level or class-level fixture methods, the fix silently does nothing.

Recommendation: Either remove ConfigureAwait(false) from this loop (consistent with the fix) or document that the fix only applies when GlobalTestInitializations is empty or synchronous.

[Correctness] Fix applies only to the _executionContext is null branch

When _executionContext is non-null (tests with non-cooperative timeout fixtures), the else branch uses ExecutionContextHelpers.RunOnContext(_executionContext, async () => { ... await invokeResult.ConfigureAwait(false); }), which was not changed. This means the SynchronizationContext is still lost for the test method body in that scenario. This is a pre-existing limitation but is worth documenting so future maintainers understand the scope.

Positive Observations

• The core fix is mechanically correct: removing ConfigureAwait(false) from await RunTestInitializeMethodAsync(...) in ExecuteInternalAsync correctly causes the continuation to post back to the captured SynchronizationContext (the DispatcherQueueSynchronizationContext), restoring the UI-thread semantics for the test method body.
• The SynchronizationContextPreservingTask / SynchronizationContextPreservingAsyncTaskMethodBuilder design is well-understood and complements this fix correctly (handling the synchronous-completion case where the BCL would otherwise restore the old context).
• The #pragma warning disable CA2007 with clear justification is the right approach for a deliberate deviation from the ConfigureAwait(false) norm.
• The new test is a well-structured simulation of the WinUI dispatch scenario and effectively validates the fix for the primary reported case.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

New commit (576c046) — Follow-up review

Evangelink's latest commit correctly addresses the test routing problem identified after the previous review: the original test used _testMethodInfo (which has a 1-hour timeout set), causing IsTimeoutSet = true and routing through the non-cooperative timeout path where ExecuteInternalAsync runs on a plain thread-pool thread with SynchronizationContext.Current == null. The fix creates a fresh TestMethodInfo without a timeout, correctly routing through the _executionContext is null branch that the production fix targets.

Key Findings

[Correctness] ContinueWith missing IsCanceled handler (inline comment above)

The else { tcs.SetResult(t.Result); } branch throws AggregateException if the antecedent task is cancelled, because the exception escapes into the discarded continuation task, leaving tcs permanently unset and the test hanging. A explicit else if (t.IsCanceled) { tcs.SetCanceled(); } arm is needed.

Still Outstanding from Prior Review

• GlobalTestInitializations loop (ConfigureAwait(false) at line ~81): For WinUI classes with async assembly/class-level fixtures, the fix is still silently ineffective. Either document the limitation or apply the same treatment to that loop.
• Comment accuracy (line 121): The comment claims TestCleanup also runs on the SynchronizationContext thread, but RunTestCleanupMethodAsync internally uses ConfigureAwait(false), so async TestCleanup continuations still run on the thread pool.

Positive Observations

• The routing fix is mechanically correct and the test now exercises the exact code path that was patched.
• SingleThreadedSynchronizationContextForTesting faithfully mirrors DispatcherQueue semantics and its lifecycle (CompleteAdding → Join → Dispose) is sound.
• The _ fire-and-forget pattern inside Post is correct because InvokeAsync is designed to never throw synchronously.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Re-review after commit 576c0466

Evangelink's latest commit correctly addresses the structural flaw flagged in the previous review: the test was using _testMethodInfo (which has a 1-hour timeout), routing through ExecuteInternalWithTimeoutAsync's non-cooperative path on a thread-pool thread where SynchronizationContext.Current is null. The new no-timeout TestMethodInfo ensures the test exercises the exact code path the production fix targets. This is a meaningful improvement.

Remaining Open Issues (from previous review, still unresolved)

The three inline comments posted in the previous run are still unresolved:

1. [Flakiness – Hang Risk] await tcs.Task (line ~703) has no timeout. If the sync context's thread dies before completing the TCS — e.g., due to a synchronous exception in the Post callback — the test hangs indefinitely and blocks the CI run. Fix: await tcs.Task.WaitAsync(TimeSpan.FromSeconds(30)).

2. [Flakiness – Silent Thread Death] The SingleThreadedSynchronizationContextForTesting thread loop calls d(state) without a try/catch (line ~1687). Any synchronous exception kills the thread silently, making the TCS unreachable and confirming issue #1 (hang). Fix: wrap d(state) in try/catch that calls tcs.SetException or at minimum swallows to keep the thread alive.

3. [Coverage] The PR title says "Fix async TestInitialize/TestCleanup..." but only the TestInitialize → TestMethod path is tested. The TestMethod → TestCleanup context-preservation path is unverified. A regression in TestCleanup context handling would go undetected.

Recommendations

• Add .WaitAsync(TimeSpan.FromSeconds(30)) to bound CI hang risk.
• Add a try/catch around d(state) in the helper's thread loop.
• Add a sibling test verifying SynchronizationContext.Current inside an async TestCleanup.

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

The fix is correct and complete for the stated scenario. No blocking issues found.

Correctness — ✅ Sound

The root cause analysis is accurate. ConfigureAwait(false) on await RunTestInitializeMethodAsync(...) discarded the captured DispatcherQueueSynchronizationContext, causing the test-method body to resume on the thread pool. Switching to ConfigureAwait(true) re-enables context capture so the continuation is posted back to the UI thread.

The interaction with SynchronizationContextPreservingAsyncTaskMethodBuilder is critical and correct: its Start() override skips BCL's AsyncMethodBuilderCore.Start which normally saves/restores the SynchronizationContext. This means that even when RunTestInitializeMethodAsync internally uses ConfigureAwait(false) for every inner await, the outer ConfigureAwait(true) on the SynchronizationContextPreservingTask<bool> returned to ExecuteInternalAsync correctly captures and posts the continuation to the UI context.

The _executionContext is not null path (when [TestClassInitialize] ran) is also correctly handled: ExecutionContextHelpers.RunOnContext uses ExecutionContext.Run on the calling thread (the UI thread after ConfigureAwait(true) returned), so the test method starts on the UI thread in that path too.

For the cleanup test: RunTestCleanupMethodAsync also uses SynchronizationContextPreservingAsyncTaskMethodBuilder, so its Start() executes synchronously on the calling thread (UI thread). InvokeCleanupMethodAsync likewise starts synchronously on the UI thread, meaning the cleanup body's opening code always runs with the UI SynchronizationContext set — confirming the test assertion is valid.

Threading — ✅ No regressions

When SynchronizationContext.Current is null (all standard, non-WinUI tests), ConfigureAwait(true) behaves identically to ConfigureAwait(false) — both schedule continuations on the default ThreadPool. No performance or behavioral regression.

Noted Pre-existing Limitation (out of scope for this PR)

The foreach loop over Parent.Parent.GlobalTestInitializations immediately above the fixed code uses .ConfigureAwait(false). If a WinUI project has any async global test initializations ([AssemblyInitialize]-level), the UI SynchronizationContext would be captured as null before the fixed code is reached, making ConfigureAwait(true) a no-op for those tests. This is pre-existing and not introduced or worsened by this PR — the common WinUI test scenario without global initializations is fully fixed.

Test Quality — ✅ Well-structured

SingleThreadedSynchronizationContextForTesting correctly models a DispatcherQueueSynchronizationContext: a BlockingCollection driven by a dedicated background thread with Post() correctly enqueuing work. Timeout handling in RunInvokeAsyncOnContextAsync (30 s CancellationTokenSource + Task.WhenAny) is safe — a timeout causes an immediate assertion failure before await tcs.Task is reached, and Dispose() gracefully handles the stuck thread via _thread.Join(5s).

Recommendations

No changes required. The stated bug is fixed correctly and tested appropriately.

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

Small suggestion on comment so that it's easier to read and more correct.

I don't see any immediate issues with the change, but be aware that it's risky and might (or might not) introduce unexpected regressions.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 2

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

No new test issues found. The test file (TestMethodInfoTests.cs) has not changed since the previous Test Expert review at 2026-05-11T21:35:06Z. The only PR-specific commit since then is a production-code comment update (863563e) co-authored by Youssef Fahmy — no test code was affected.

The two new tests added in this PR remain well-structured:

• TestMethodInfo_AsyncTestInitialize_PreservesSynchronizationContextForTestMethod — correctly exercises the fixed code path using a SingleThreadedSynchronizationContextForTesting that mirrors DispatcherQueueSynchronizationContext semantics. The assertion capturedContextInTestMethod.Should().BeSameAs(syncContext) directly verifies the bug is fixed.
• TestMethodInfo_AsyncTestCleanup_PreservesSynchronizationContextForTestCleanup — verifies the analogous cleanup scenario.

Infrastructure quality:

• using var syncContext ensures reliable disposal
• 30-second CancellationTokenSource + Task.WhenAny prevents CI hangs without blocking indefinitely
• BlockingCollection background thread uses IsBackground = true as a safety net
• TaskCreationOptions.RunContinuationsAsynchronously on the TaskCompletionSource prevents the context thread from being blocked by synchronous continuations
• Static DummyTestClass hook fields are set before use in each test, consistent with the established convention in this file

Recommendations

None. The tests accurately verify the production fix and guard against regression.

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

The core fix — changing ConfigureAwait(false) to ConfigureAwait(true) on await RunTestInitializeMethodAsync(...) (line 122) and await invokeResult (line 129) — is correct and safe for the common case:

• When SynchronizationContext.Current is null (thread-pool tests), ConfigureAwait(true) is a no-op, so there's no behavioral change or performance impact for the vast majority of tests.
• When SynchronizationContext.Current is the WinUI DispatcherQueueSynchronizationContext, continuations are correctly posted back to the UI thread, so both the test method body and (via the call chain) TestCleanup start on the UI thread.
• The SynchronizationContextPreservingAsyncTaskMethodBuilder correctly avoids the BCL's sync-context save/restore in Start(), allowing the sync context set by a UITestMethod dispatcher to flow into the test lifecycle without being silently discarded.

Threading — Incomplete fix for the _executionContext is not null path

await tcs.Task.ConfigureAwait(false) at line 164 (the else branch for non-null _executionContext) was not changed. After that await, result.Outcome = UnitTestOutcome.Passed and RunTestCleanupMethodAsync (line 226) execute on a thread-pool thread. A WinUI UITestMethod where any fixture in the hierarchy used a non-cooperative [Timeout] — which sets _executionContext via CaptureExecutionContextAfterFixtureIfNeeded — would still deliver TestCleanup on the wrong thread. The two new tests only exercise the _executionContext is null path.

Positive observations

• The SingleThreadedSynchronizationContextForTesting helper is a well-designed, realistic proxy for DispatcherQueueSynchronizationContext. The BlockingCollection-based message loop, background-thread flag, and CompleteAdding-before-Join teardown are all correct.
• RunInvokeAsyncOnContextAsync correctly uses TaskCreationOptions.RunContinuationsAsynchronously and TaskScheduler.Default for the ContinueWith so TCS completion can't re-enter the single-threaded loop.
• The 30-second watchdog timer prevents CI hangs if the single-threaded context stalls.

Recommendations

1. Change await tcs.Task.ConfigureAwait(false) → ConfigureAwait(true) at line 164 to align the _executionContext is not null path with the fix applied to the null path.
2. Add a third test that sets _executionContext on the TestMethodInfo (simulating a prior non-cooperative fixture) to cover this code branch.

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠