[fix] Fix flaky EnableLoggersArgumentProcessorTests on macOS#16054
Merged
Conversation
…sManager The test class uses RunSettingsManager.Instance (a static singleton) but did not reset it between tests, causing potential cross-test contamination on macOS where test execution order may differ. Add TestInitialize reset and TestCleanup to ensure each test starts with and leaves behind a clean RunSettingsManager state. This matches the existing pattern in RunSettingsManagerTests.cs. Fixes #15614 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses macOS flakiness in EnableLoggersArgumentProcessorTests by ensuring the static RunSettingsManager.Instance singleton does not leak state between test runs, keeping the unit test environment deterministic.
Changes:
- Reset
RunSettingsManager.Instancetonullin[TestInitialize]to start each test with a fresh singleton. - Add a
[TestCleanup]method that also resetsRunSettingsManager.Instancetonullto avoid leaving shared state behind for subsequent tests.
…nitialize Ensure clean state before setting up mocks, not after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Commit pushed:
|
nohwnd
commented
May 25, 2026
nohwnd
left a comment
nohwnd
left a comment
Member
Author
There was a problem hiding this comment.
Clean, minimal fix. RunSettingsManager.Instance = null in [TestInitialize] and [TestCleanup] correctly isolates tests from cross-class singleton bleed — the class already had [DoNotParallelize] for within-class safety. Pattern matches RunSettingsManagerTests.cs. No concerns.
🧠 Reviewed by Expert Code Reviewer
🧠 Reviewed by Expert Code Reviewer 🧠
nohwnd
enabled auto-merge (squash)
May 26, 2026 11:47
nohwnd
disabled auto-merge
May 26, 2026 11:48
This was referenced May 26, 2026
nohwnd
added a commit
that referenced
this pull request
Jun 11, 2026
…ost hasn't launched yet (#16065) * Fix race condition: skip hang dump when testhost hasn't launched yet When the inactivity timer fires before the testhost process has launched, _testHostProcessId is 0 (default int). Previously this caused ProcessDumpUtility.StartHangBasedProcessDump to attempt to dump PID 0 (the Idle process on Windows / Swapper on Linux), resulting in an empty or incorrect dump file. The fix adds an early-return guard in CollectDumpAndAbortTesthost: if _testHostProcessId == 0, log a warning and skip the dump/kill. Also updates three existing hang dump unit tests to properly simulate the happy-path scenario (testhost launches before the timer fires) by: - Using a 50 ms timeout instead of 0 ms so the TestHostLaunched event can be raised before the timer callback runs - Raising TestHostLaunched with PID 1234 before the timer fires Adds a new test that verifies StartHangBasedProcessDump is NOT called when the timer fires before TestHostLaunched. Fixes #15588 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: mark _testHostProcessId as volatile for cross-thread visibility Ensures the hang-dump guard added in the parent commit sees a fresh value on the timer-callback thread (ARM64 / weakly-ordered CPUs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: start hang-dump timer only after testhost launches Instead of guarding against PID 0 in the timer callback, don't start the inactivity timer until TestHostLaunched fires. This eliminates the race at the source — the timer can never fire before we know which process to dump. - Remove ResetInactivityTimer() call from Initialize; timer is created but not started until TestHostLaunchedHandler sets the PID and calls ResetInactivityTimer. - Swap order in TestHostLaunchedHandler: set PID before starting timer. - Remove PID==0 guard and resource string (no longer needed). - Revert volatile — no cross-thread visibility concern with proper ordering. - Update tests to reflect that timer starts on TestHostLaunched, not Initialize. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: add integration test for hang dump when testhost fails to start Poisons runtimeconfig.json with net9999.0 so testhost can't launch, then runs with CollectHangDump. Verifies vstest exits cleanly without hanging or producing a dump against PID 0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: restore runtimeconfig.json after integration test The test mutates SimpleTestProjectMessedUpTargetFramework's runtimeconfig.json, which is shared with ProcessesInteractionTests. Wrap in try/finally to restore the original content. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use Interlocked.Exchange to set _testHostProcessId Reviewer preference: use Interlocked for cross-thread visibility instead of plain int assignment, as it is more common and explicit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: use regex for version replacement, add DoNotParallelize, assert runtime-not-found error Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: use StdErrorRegexIsMatch for runtime-not-found assertion Use the same assertion approach as ProcessesInteractionTests which is proven to work on Windows. The previous Assert.MatchesRegex on combined StdOut+StdErr may fail due to multiline regex behavior differences. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: check both stdout and stderr for runtime-not-found assertion The .NET runtime-not-found error message can appear in either stdout or stderr depending on the platform and vstest host configuration. Check both streams to make the assertion robust across environments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: skip inactivity timer reset until testhost has launched ResetInactivityTimer was called from SessionEndedHandler (and TestCase events) even when testhost had never launched. This started the 30-second hang-dump timer, which would then fire against PID 0 (the Windows Idle process). Add a guard: only reset/start the timer when _testHostProcessId != 0, meaning testhost has already launched. Since TestHostLaunchedHandler sets the PID before calling ResetInactivityTimer, the timer will start correctly for normal runs, and will simply never start when testhost fails to launch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: remove manual /Diag flag from HangDumpOnTimeout IntegrationTestBase now auto-injects /diag and throws when tests pass it manually (see #16055). Remove the explicit /Diag argument to match the new convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix flaky PortArgumentProcessor test by caching port locally (#16050) PortArgumentExecutor.Execute() was reading _commandLineOptions.Port from the shared CommandLineOptions.Instance singleton. When InProcessVsTestConsoleWrapper runs executor.Execute(args) on a background Task.Run thread, it initializes the port argument (e.g. port=1234), which overwrites CommandLineOptions.Instance.Port. If this races with another test that set Port=2345, the wrong port is passed to ConnectToClientAndProcessRequests. Fix: capture the port number in a local _port field during Initialize() and use that captured value in Execute() instead of re-reading the shared singleton. Fixes #15730 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * [fix] Fix flaky EnableLoggersArgumentProcessorTests on macOS (#16054) * Fix flaky EnableLoggersArgumentProcessorTests by resetting RunSettingsManager The test class uses RunSettingsManager.Instance (a static singleton) but did not reset it between tests, causing potential cross-test contamination on macOS where test execution order may differ. Add TestInitialize reset and TestCleanup to ensure each test starts with and leaves behind a clean RunSettingsManager state. This matches the existing pattern in RunSettingsManagerTests.cs. Fixes #15614 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Reorder: reset RunSettingsManager before SetupMockExtensions in TestInitialize Ensure clean state before setting up mocks, not after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use [^"] pattern to match pre-release version strings in runtimeconfig.json The regex [\d.]+ only matches digits and dots, so it fails to match pre-release version strings like '11.0.0-preview.3.26170.106'. With that pattern the runtimeconfig.json was not modified, testhost started successfully with the real runtime, and the assertion that '9999.0.0' appeared in the output always failed on CI. Change to [^"]+ to match any non-quote characters, which handles both stable ('11.0.0') and pre-release ('11.0.0-preview.X') versions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Summary
🤖 This is an automated fix by the Issue Repro Triage agent.
Fixes #15614
Root Cause
EnableLoggersArgumentProcessorTestsusesRunSettingsManager.Instance— a static singleton — but did not reset it in[TestInitialize]or[TestCleanup]. When tests from other classes run in parallel and mutate this shared singleton, the state bleeds into these tests, causing non-deterministic failures (particularly on macOS where test execution order can differ).Fix
Added
RunSettingsManager.Instance = nullin both[TestInitialize]and[TestCleanup]:TestInitialize: ensures a freshRunSettingsManagerbefore each test, regardless of what previous tests left behindTestCleanup: leaves the singleton in a clean state for subsequent testsThis matches the existing pattern in
RunSettingsManagerTests.cs, which already resetsRunSettingsManager.Instance = nullin itsTestCleanup.Verification
All 15 tests in
EnableLoggersArgumentProcessorTestspass after the change.