Inject IRunSettingsHelper instead of reading RunSettingsHelper.Instance - #16205
Merged
Jakub Jareš (nohwnd) merged 1 commit intoJul 3, 2026
Merged
Conversation
The argument processors that write the request-scoped runsettings flags (IsDefaultTargetArchitecture, IsDesignMode) reach for RunSettingsHelper.Instance, and the readers deeper in the pipeline do the same, so those flags are shared process-wide static state that leaks across requests in design mode. This threads IRunSettingsHelper through the composition roots, defaulting to RunSettingsHelper.Instance so behavior is unchanged, mirroring the IRunSettingsProvider work in microsoft#16200. - ArgumentProcessorFactory.Create(...) takes an optional IRunSettingsHelper and threads it into the four processors that read/write the flags; Executor owns it and passes it in, defaulting to RunSettingsHelper.Instance. PortArgumentProcessor picks up injection for the first time. - TestRequestManager and DataCollectorAttachmentsProcessorsFactory take the helper through their DI constructors and read the injected instance. - DotnetTestHostManager is activated by reflection through the extension framework, not by the engine, so it stays on the .Instance fallback (same shared instance). RunSettingsHelper.Instance is not obsoleted; the remaining references are the composition-root defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors request-scoped RunSettings flags (IsDefaultTargetArchitecture, IsDesignMode) away from the process-wide RunSettingsHelper.Instance singleton and into IRunSettingsHelper constructor injection, keeping default runtime behavior unchanged by defaulting injected instances to RunSettingsHelper.Instance.
Changes:
- Thread
IRunSettingsHelperfrom composition roots into key argument processors/executors that write request-scoped flags (Port/Platform/Settings/CliRunSettings) viaArgumentProcessorFactory. - Inject
IRunSettingsHelperintoTestRequestManagerandDataCollectorAttachmentsProcessorsFactoryso readers can consume the same instance (defaulting toRunSettingsHelper.Instance). - Update unit tests to use per-test
RunSettingsHelperinstances and remove a[DoNotParallelize]previously needed due to shared static state.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vstest.console.UnitTests/TestPlatformHelpers/TestRequestManagerTests.cs | Uses per-test IRunSettingsHelper to avoid shared static state and re-enables parallelism. |
| test/vstest.console.UnitTests/Processors/Utilities/ArgumentProcessorFactoryTests.cs | Updates reflection-based processor instantiation to support new constructor shapes (provider/helper/both). |
| test/vstest.console.UnitTests/Processors/RunSettingsArgumentProcessorTests.cs | Updates processor/executor construction to pass an IRunSettingsHelper. |
| test/vstest.console.UnitTests/Processors/PortArgumentProcessorTests.cs | Updates Port processor/executor tests for injected helper; asserts design-mode flag propagation. |
| test/vstest.console.UnitTests/Processors/PlatformArgumentProcessorTests.cs | Updates Platform processor/executor tests for injected helper. |
| test/vstest.console.UnitTests/Processors/CLIRunSettingsArgumentProcessorTests.cs | Updates CLI runsettings processor/executor tests for injected helper. |
| src/vstest.console/TestPlatformHelpers/TestRequestManager.cs | Stores injected helper and uses it when computing/logging default architecture decisions. |
| src/vstest.console/Processors/Utilities/ArgumentProcessorFactory.cs | Extends factory to accept/pass IRunSettingsHelper and wires it into relevant processors. |
| src/vstest.console/Processors/RunSettingsArgumentProcessor.cs | Injects helper and writes IsDefaultTargetArchitecture via the injected instance. |
| src/vstest.console/Processors/PortArgumentProcessor.cs | Injects helper and writes IsDesignMode via the injected instance. |
| src/vstest.console/Processors/PlatformArgumentProcessor.cs | Injects helper and writes IsDefaultTargetArchitecture via the injected instance. |
| src/vstest.console/Processors/CLIRunSettingsArgumentProcessor.cs | Injects helper and writes IsDefaultTargetArchitecture via the injected instance. |
| src/vstest.console/CommandLine/Executor.cs | Executor now owns/passes IRunSettingsHelper into ArgumentProcessorFactory. |
| src/Microsoft.TestPlatform.CrossPlatEngine/AttachmentsProcessing/DataCollectorAttachmentsProcessorsFactory.cs | Reads IsDesignMode from injected helper (defaulting to RunSettingsHelper.Instance). |
Comment on lines
+35
to
+38
| public PortArgumentProcessor(IRunSettingsHelper runSettingsHelper) | ||
| { | ||
| _runSettingsHelper = runSettingsHelper; | ||
| } |
Comment on lines
139
to
145
| ValidateArg.NotNull(options, nameof(options)); | ||
| _commandLineOptions = options; | ||
| _testRequestManager = testRequestManager; | ||
| _designModeInitializer = designModeInitializer; | ||
| _processHelper = processHelper; | ||
| _runSettingsHelper = runSettingsHelper; | ||
| } |
Jakub Jareš (nohwnd)
enabled auto-merge (squash)
July 3, 2026 13:29
Azat Mukhametshin (azat-msft)
approved these changes
Jul 3, 2026
Jakub Jareš (nohwnd)
added a commit
to nohwnd/vstest
that referenced
this pull request
Jul 3, 2026
The argument processors reach for CommandLineOptions.Instance when they construct their executors, so the request-scoped command-line state (target framework, platform, test adapter paths, and the rest) is shared process-wide static state that leaks across requests in design mode. This threads CommandLineOptions through the composition roots the same way as the IRunSettingsProvider and IRunSettingsHelper work in microsoft#16200 and microsoft#16205, defaulting to CommandLineOptions.Instance so behavior is unchanged. - ArgumentProcessorFactory.Create(...) takes an optional CommandLineOptions and passes it into every default processor as the first constructor argument; Executor owns it and passes it in, defaulting to CommandLineOptions.Instance. - Each processor now holds an injected CommandLineOptions field instead of reading the static, and hands it to the executor it builds. - TestRequestManager and ConsoleLogger construct or read CommandLineOptions outside the processor path, so they stay on the .Instance fallback (same shared instance) for now. CommandLineOptions.Instance is not obsoleted; the remaining references are the composition-root defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jakub Jareš (nohwnd)
added a commit
to nohwnd/vstest
that referenced
this pull request
Jul 3, 2026
The run and discovery argument processors reach for TestRequestManager.Instance when they build their executors, so the request orchestrator is shared process-wide static state that is reused across requests in design mode. This threads ITestRequestManager through the composition roots the same way as the IRunSettingsProvider, IRunSettingsHelper, and CommandLineOptions work in microsoft#16200, microsoft#16205, and the CommandLineOptions change, defaulting to TestRequestManager.Instance so behavior is unchanged. TestRequestManager.Instance is relatively heavy: its parameterless constructor builds a TestPlatform, a metrics publisher, and reads the design-mode flag. Today it is resolved lazily, inside each processor's Lazy<IArgumentExecutor>, so it is only constructed when a run/discovery command actually executes and not for commands like --Help. To keep that timing byte-for-byte identical the injected instance is nullable and the processors fall back to TestRequestManager.Instance inside the lambda; the factory passes the parameter straight through without forcing it. - ArgumentProcessorFactory.Create(...) takes an optional ITestRequestManager and hands it to the six processors that build a request-manager-backed executor (ListTests, RunTests, RunSpecificTests, Port, UseVsixExtensions, and ListFullyQualifiedTests); Executor owns it and leaves it null in production so the lazy .Instance fallback keeps running. - Each of those processors holds an injected ITestRequestManager? field and uses _testRequestManager ?? TestRequestManager.Instance when it constructs its executor. - ArgumentProcessorFactoryTests builds each processor by reflection; its constructor-shape probe now covers the request-manager-bearing shapes. TestRequestManager.Instance is not obsoleted; the remaining references are the composition-root default and the lazy fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jakub Jareš (nohwnd)
added a commit
to nohwnd/vstest
that referenced
this pull request
Jul 3, 2026
The argument processors reach for CommandLineOptions.Instance when they construct their executors, so the request-scoped command-line state (target framework, platform, test adapter paths, and the rest) is shared process-wide static state that leaks across requests in design mode. This threads CommandLineOptions through the composition roots the same way as the IRunSettingsProvider and IRunSettingsHelper work in microsoft#16200 and microsoft#16205, defaulting to CommandLineOptions.Instance so behavior is unchanged. - ArgumentProcessorFactory.Create(...) takes an optional CommandLineOptions and passes it into every default processor as the first constructor argument; Executor owns it and passes it in, defaulting to CommandLineOptions.Instance. - Each processor now holds an injected CommandLineOptions field instead of reading the static, and hands it to the executor it builds. - TestRequestManager and ConsoleLogger construct or read CommandLineOptions outside the processor path, so they stay on the .Instance fallback (same shared instance) for now. CommandLineOptions.Instance is not obsoleted; the remaining references are the composition-root defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jakub Jareš (nohwnd)
added a commit
to nohwnd/vstest
that referenced
this pull request
Jul 3, 2026
The run and discovery argument processors reach for TestRequestManager.Instance when they build their executors, so the request orchestrator is shared process-wide static state that is reused across requests in design mode. This threads ITestRequestManager through the composition roots the same way as the IRunSettingsProvider, IRunSettingsHelper, and CommandLineOptions work in microsoft#16200, microsoft#16205, and the CommandLineOptions change, defaulting to TestRequestManager.Instance so behavior is unchanged. TestRequestManager.Instance is relatively heavy: its parameterless constructor builds a TestPlatform, a metrics publisher, and reads the design-mode flag. Today it is resolved lazily, inside each processor's Lazy<IArgumentExecutor>, so it is only constructed when a run/discovery command actually executes and not for commands like --Help. To keep that timing byte-for-byte identical the injected instance is nullable and the processors fall back to TestRequestManager.Instance inside the lambda; the factory passes the parameter straight through without forcing it. - ArgumentProcessorFactory.Create(...) takes an optional ITestRequestManager and hands it to the six processors that build a request-manager-backed executor (ListTests, RunTests, RunSpecificTests, Port, UseVsixExtensions, and ListFullyQualifiedTests); Executor owns it and leaves it null in production so the lazy .Instance fallback keeps running. - Each of those processors holds an injected ITestRequestManager? field and uses _testRequestManager ?? TestRequestManager.Instance when it constructs its executor. - ArgumentProcessorFactoryTests builds each processor by reflection; its constructor-shape probe now covers the request-manager-bearing shapes. TestRequestManager.Instance is not obsoleted; the remaining references are the composition-root default and the lazy fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 3, 2026
Jakub Jareš (nohwnd)
added a commit
that referenced
this pull request
Jul 7, 2026
…6208) * Inject CommandLineOptions into vstest.console argument processors The argument processors reach for CommandLineOptions.Instance when they construct their executors, so the request-scoped command-line state (target framework, platform, test adapter paths, and the rest) is shared process-wide static state that leaks across requests in design mode. This threads CommandLineOptions through the composition roots the same way as the IRunSettingsProvider and IRunSettingsHelper work in #16200 and #16205, defaulting to CommandLineOptions.Instance so behavior is unchanged. - ArgumentProcessorFactory.Create(...) takes an optional CommandLineOptions and passes it into every default processor as the first constructor argument; Executor owns it and passes it in, defaulting to CommandLineOptions.Instance. - Each processor now holds an injected CommandLineOptions field instead of reading the static, and hands it to the executor it builds. - TestRequestManager and ConsoleLogger construct or read CommandLineOptions outside the processor path, so they stay on the .Instance fallback (same shared instance) for now. CommandLineOptions.Instance is not obsoleted; the remaining references are the composition-root defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Re-run CI (flaky CrashDumpOnStackOverflow, unrelated) The Windows Integration Test leg failed only on CrashDumpOnStackOverflow with 'Expected at least 1 dump file in Attachments, but there were 0' -- a procdump capture race in the separate datacollector process, which does not consume the argument-processor CommandLineOptions this change threads. Empty commit to re-trigger CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jakub Jareš (nohwnd)
added a commit
to nohwnd/vstest
that referenced
this pull request
Jul 7, 2026
The run and discovery argument processors reach for TestRequestManager.Instance when they build their executors, so the request orchestrator is shared process-wide static state that is reused across requests in design mode. This threads ITestRequestManager through the composition roots the same way as the IRunSettingsProvider, IRunSettingsHelper, and CommandLineOptions work in microsoft#16200, microsoft#16205, and the CommandLineOptions change, defaulting to TestRequestManager.Instance so behavior is unchanged. TestRequestManager.Instance is relatively heavy: its parameterless constructor builds a TestPlatform, a metrics publisher, and reads the design-mode flag. Today it is resolved lazily, inside each processor's Lazy<IArgumentExecutor>, so it is only constructed when a run/discovery command actually executes and not for commands like --Help. To keep that timing byte-for-byte identical the injected instance is nullable and the processors fall back to TestRequestManager.Instance inside the lambda; the factory passes the parameter straight through without forcing it. - ArgumentProcessorFactory.Create(...) takes an optional ITestRequestManager and hands it to the six processors that build a request-manager-backed executor (ListTests, RunTests, RunSpecificTests, Port, UseVsixExtensions, and ListFullyQualifiedTests); Executor owns it and leaves it null in production so the lazy .Instance fallback keeps running. - Each of those processors holds an injected ITestRequestManager? field and uses _testRequestManager ?? TestRequestManager.Instance when it constructs its executor. - ArgumentProcessorFactoryTests builds each processor by reflection; its constructor-shape probe now covers the request-manager-bearing shapes. TestRequestManager.Instance is not obsoleted; the remaining references are the composition-root default and the lazy fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jakub Jareš (nohwnd)
added a commit
that referenced
this pull request
Jul 8, 2026
…16228) The run and discovery argument processors reach for TestRequestManager.Instance when they build their executors, so the request orchestrator is shared process-wide static state that is reused across requests in design mode. This threads ITestRequestManager through the composition roots the same way as the IRunSettingsProvider, IRunSettingsHelper, and CommandLineOptions work in #16200, #16205, and the CommandLineOptions change, defaulting to TestRequestManager.Instance so behavior is unchanged. TestRequestManager.Instance is relatively heavy: its parameterless constructor builds a TestPlatform, a metrics publisher, and reads the design-mode flag. Today it is resolved lazily, inside each processor's Lazy<IArgumentExecutor>, so it is only constructed when a run/discovery command actually executes and not for commands like --Help. To keep that timing byte-for-byte identical the injected instance is nullable and the processors fall back to TestRequestManager.Instance inside the lambda; the factory passes the parameter straight through without forcing it. - ArgumentProcessorFactory.Create(...) takes an optional ITestRequestManager and hands it to the six processors that build a request-manager-backed executor (ListTests, RunTests, RunSpecificTests, Port, UseVsixExtensions, and ListFullyQualifiedTests); Executor owns it and leaves it null in production so the lazy .Instance fallback keeps running. - Each of those processors holds an injected ITestRequestManager? field and uses _testRequestManager ?? TestRequestManager.Instance when it constructs its executor. - ArgumentProcessorFactoryTests builds each processor by reflection; its constructor-shape probe now covers the request-manager-bearing shapes. TestRequestManager.Instance is not obsoleted; the remaining references are the composition-root default and the lazy fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 9, 2026
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.
RunSettingsHelper.Instanceis a settable public static that holds two request-scoped flags —IsDefaultTargetArchitectureandIsDesignMode. The argument processors set them while parsing the command line, and the engine, attachment processing, and test host provider read them back deeper in the pipeline. So they're shared, process-wide static state: in design mode (VS / a long-running host) requests reuse the process, and tests have to poke and reset the singleton to isolate cases. This is the second step of pulling that state out into constructor injection, after #16200 did the same forIRunSettingsProvider.The tricky part is that the writers and the readers have to end up on the same instance, or design-mode propagation silently breaks. I kept that safe by threading one
IRunSettingsHelperfrom each composition root and defaulting it toRunSettingsHelper.Instanceeverywhere, so writers and readers still share the same object and runtime behavior is unchanged.Change
ArgumentProcessorFactory.Create(...)now takes an optionalIRunSettingsHelper, defaulting toRunSettingsHelper.Instance, and threads it into the four processors that write the flags:RunSettingsArgumentProcessor,PlatformArgumentProcessor,CliRunSettingsArgumentProcessor, andPortArgumentProcessor. Those take the helper through a required constructor instead of reaching for.Instance.PortArgumentProcessorpicks up injection for the first time (it wasn't one of the phase-1 processors).Executor(the composition root for the writers) owns the helper and passes it to the factory, defaulting toRunSettingsHelper.Instance.TestRequestManagertakesIRunSettingsHelperthrough its DI constructor (the parameterless ctor still defaults to.Instance) and reads the injected instance inGetDefaultArchitectureand the verbose log.DataCollectorAttachmentsProcessorsFactorytakes the helper through an optional constructor (default.Instance) and reads it inCreate.TestRequestManagerkeeps a thin delegating constructor that defaults the helper to.Instanceso the existing test construction sites compile unchanged.DotnetTestHostManageralso readsRunSettingsHelper.Instance, but it's activated by reflection throughActivator.CreateInstancein the extension framework, not constructed by the engine, so there's no constructor to inject through without reworking activation. I left it on the.Instancefallback — it's the same shared instance, so it stays correct. A later phase can take it if we rework extension activation.I did not
[Obsolete]RunSettingsHelper.Instance— later phases and other consumers still read it. The only.Instancereferences left are the composition-root defaults (and the deliberateDotnetTestHostManagerone), so this is behavior-preserving.One small testability win falls out of this:
SettingDefaultPlatformUsesItForAnyCPUSourceButNotForNonAnyCPUSourceused to need[DoNotParallelize]because it wroteRunSettingsHelper.Instance.IsDefaultTargetArchitectureand raced other tests. With the helper injected per-test, I removed that attribute.Verification
vstest.console.UnitTests— 635 pass onnet481(2 pre-existing skips).Microsoft.TestPlatform.CrossPlatEngine.UnitTests— 670 pass onnet481(1 pre-existing skip).build.cmd -pack(Debug) — clean; binding redirects and DLL frameworks verified.test.cmd -smokeTest): Acceptance 5/5net11.0-x64, Library 4/4net11.0-x64 + 4/4net481-x86.