Inject IRunSettingsProvider into vstest.console argument processors#16200
Merged
Conversation
vstest.console built every argument processor with a parameterless ctor and read RunSettingsManager.Instance internally, so the active runsettings were shared process-wide and awkward to isolate in tests. Thread an IRunSettingsProvider from Executor through ArgumentProcessorFactory into the 18 processors that consume runsettings; the factory defaults to RunSettingsManager.Instance so behavior is unchanged. RunTests/RunSpecificTests also flow the provider into their nested TestRunRequestEventsRegistrar. First step toward removing mutable static state from the console. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The skill told you to run smoke tests with `-projects smoke` / `-p smoke`, but `-projects` is passed through Resolve-Path and only accepts a real path/glob, so a bare category name fails with "Cannot find path". Smoke/integration/perf/compat are switches (`-smokeTest`, `-integrationTest`, ...). Also documented the Windows DOTNET_ROOT gotcha: test.cmd (unlike test.sh) doesn't set DOTNET_ROOT, so the self-hosted preview-TFM apphosts fail to launch until you point it at the repo .dotnet. Dropped the TestRunnerAdditionalArguments --filter example that Build.ps1 now rejects in favor of the -filter parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors vstest.console argument processor construction to support constructor injection of an IRunSettingsProvider, reducing reliance on the process-wide RunSettingsManager.Instance singleton and improving test isolation (especially for long-running design-mode hosts).
Changes:
- Extended
ArgumentProcessorFactory.Create(...)to accept an optionalIRunSettingsProvider(defaulting toRunSettingsManager.Instance) and threaded it into the processors that read/write runsettings. - Updated affected argument processors (and nested event registrars) to depend on the injected provider instead of reading
RunSettingsManager.Instancedirectly. - Updated
Executorto own and flow the provider into the factory, and adjusted unit tests to construct processors using a test runsettings provider.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vstest.console.UnitTests/Processors/Utilities/ArgumentProcessorFactoryTests.cs | Updates factory tests to instantiate processors that now require IRunSettingsProvider. |
| test/vstest.console.UnitTests/Processors/TestAdapterPathArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/RunTestsArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/RunSettingsArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider (also normalizes file header). |
| test/vstest.console.UnitTests/Processors/ResultsDirectoryArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/PlatformArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/ParallelArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/ListTestsArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/ListFullyQualifiedTestsArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider (but currently has a test targeting the wrong processor). |
| test/vstest.console.UnitTests/Processors/InIsolationArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/FrameworkArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/EnableLoggersArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider. |
| test/vstest.console.UnitTests/Processors/EnableCodeCoverageArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider (also normalizes file header). |
| test/vstest.console.UnitTests/Processors/EnableBlameArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider (also normalizes file header). |
| test/vstest.console.UnitTests/Processors/CollectArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider (also normalizes file header). |
| test/vstest.console.UnitTests/Processors/CLIRunSettingsArgumentProcessorTests.cs | Updates processor construction for injected runsettings provider (also normalizes file header). |
| src/vstest.console/Processors/Utilities/ArgumentProcessorFactory.cs | Adds IRunSettingsProvider parameter to Create and injects provider into runsettings-affecting processors. |
| src/vstest.console/Processors/TestAdapterPathArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/TestAdapterLoadingStrategyArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/RunTestsArgumentProcessor.cs | Adds injected runsettings provider and flows it into executor + nested event registrar. |
| src/vstest.console/Processors/RunSpecificTestsArgumentProcessor.cs | Adds injected runsettings provider and flows it into executor + nested event registrar. |
| src/vstest.console/Processors/RunSettingsArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/ResultsDirectoryArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/PlatformArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/ParallelArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/ListTestsArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/ListFullyQualifiedTestsArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/InIsolationArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/FrameworkArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/EnvironmentArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/EnableLoggerArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/EnableCodeCoverageArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/EnableBlameArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/CollectArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/Processors/CLIRunSettingsArgumentProcessor.cs | Adds injected runsettings provider and flows it into the executor. |
| src/vstest.console/CommandLine/Executor.cs | Makes Executor the composition root for the runsettings provider and passes it into the factory + defaults. |
| .github/skills/vstest-build-test/SKILL.md | Updates build/test skill docs for correct switches and project selection guidance (with a couple doc portability fixes needed). |
Comment on lines
98
to
99
| var processor = new ListTestsArgumentProcessor(new TestableRunSettingsProvider()); | ||
| Assert.IsTrue(processor.Metadata.Value is ListTestsArgumentProcessorCapabilities); |
Member
Author
There was a problem hiding this comment.
Pre-existing copy-paste — the test constructed ListTestsArgumentProcessor and asserted ListTestsArgumentProcessorCapabilities, so it never actually covered the FQN processor. My diff just surfaced it on that line. Pointed it at ListFullyQualifiedTestsArgumentProcessor in 82f4026.
|
|
||
| ### Test Categories (smoke / integration / performance / compatibility) | ||
|
|
||
| These are **switches** handled by `eng/Build.ps1` — NOT `-projects` values: |
| ### Filter by Test Name | ||
|
|
||
| Use the `-filter` parameter. Do **not** pass `--filter` inside `TestRunnerAdditionalArguments` — | ||
| `eng/Build.ps1` explicitly throws if you do. |
Comment on lines
+107
to
+111
| For a single project you can also build+test its csproj directly with the bootstrapped SDK: | ||
|
|
||
| ```bash | ||
| ./.dotnet/dotnet.exe test test/Microsoft.TestPlatform.Extensions.HtmlLogger.UnitTests/*.csproj -c Debug | ||
| ``` |
Member
Author
There was a problem hiding this comment.
Scoped it in 82f4026 — Windows keeps ./.dotnet/dotnet.exe, with a ./.dotnet/dotnet variant for Linux/macOS.
The ListFullyQualifiedTests metadata test was constructing ListTestsArgumentProcessor and asserting ListTestsArgumentProcessorCapabilities, so it never covered the processor its name claims. Point it at ListFullyQualifiedTestsArgumentProcessor. Also fix the skill doc: eng/build.ps1 casing (case-sensitive on Linux/macOS) and scope the .dotnet/dotnet.exe example to Windows with a dotnet variant for Linux/macOS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
azat-msft
approved these changes
Jul 3, 2026
nohwnd
added a commit
that referenced
this pull request
Jul 3, 2026
…ce (#16205) 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 #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>
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>
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>
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>
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 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.
vstest.consolebuilds every argument processor with a parameterless constructor and readsRunSettingsManager.Instanceinternally. So the active runsettings are 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 first step of pulling that state out into constructor injection.Change
ArgumentProcessorFactory.Create(...)now takes an optionalIRunSettingsProvider, defaulting toRunSettingsManager.Instance, and threads it into the 18 processors that actually read/write runsettings. The rest stay parameterless..Instance, so they can't silently pick up the ambient singleton.Executor(the composition root) owns the provider and passes it to the factory. It defaults toRunSettingsManager.Instance, so runtime behavior is unchanged.RunTestsArgumentProcessor/RunSpecificTestsArgumentProcessoralso flow the provider into their nestedTestRunRequestEventsRegistrar(the.Instancereads there become a null-flow local +TPDebug.Assert).I did not
[Obsolete]RunSettingsManager.Instance— the client and later phases still read it. The only.Instancereferences left invstest.consoleare the composition-root defaults, so this is behavior-preserving.Verification
build.cmd -pack(Debug) — clean; binding redirects and DLL frameworks verified.vstest.console.UnitTests— 637 pass onnet481andnet11.0.test.cmd -smokeTest): Acceptance 5/5net11.0-x64, Library 4/4net11.0-x64 + 4/4net481-x86.The second commit is unrelated to the refactor: it fixes the
vstest-build-testskill doc, which pointed at-projects smokefor smoke tests — that value isResolve-Pathd and fails, the switch is-smokeTest. Happy to split it out if you'd rather keep this focused.