Add CreateNoNewWindow RunConfiguration setting#15585
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new RunConfiguration setting (CreateNoNewWindow) to control whether testhost processes are started with ProcessStartInfo.CreateNoWindow = true, defaulting to true to preserve existing behavior.
Changes:
- Introduces
RunConfiguration.CreateNoNewWindowwith XML serialization/deserialization and unit tests. - Threads the setting through
DefaultTestHostManager/DotnetTestHostManagerinto process launch. - Extends
IProcessHelper.LaunchProcess/ProcessHelper.LaunchProcesswith a newcreateNoNewWindowparameter and updates PublicAPI declarations.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vstest.console.UnitTests/Processors/AeDebuggerArgumentProcessorTest.cs | Updates Moq setup to accept new LaunchProcess parameter. |
| test/vstest.ProgrammerTests/Fakes/FakeProcessHelper.cs | Updates fake helper signature to include createNoNewWindow. |
| test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DotnetTestHostManagerTests.cs | Updates callbacks to match new LaunchProcess signature. |
| test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/DefaultTestHostManagerTests.cs | Updates setups/callbacks to match new LaunchProcess signature. |
| test/Microsoft.TestPlatform.ObjectModel.UnitTests/RunSettings/RunConfigurationTests.cs | Adds tests for default, XML parsing, invalid value handling, and XML output. |
| test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/DataCollection/DotnetDataCollectionLauncherTests.cs | Updates verifications to include new LaunchProcess parameter. |
| test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyOperationManagerTests.cs | Updates callback to match new LaunchProcess signature. |
| src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs | Stores runsetting and passes it to process launch. |
| src/Microsoft.TestPlatform.TestHostProvider/Hosting/DefaultTestHostManager.cs | Stores runsetting and passes it to process launch. |
| src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs | Implements createNoNewWindow by mapping to ProcessStartInfo.CreateNoWindow. |
| src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/uap10.0/PublicAPI.Shipped.txt | Updates shipped API signature for ProcessHelper.LaunchProcess. |
| src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/netstandard2.0/PublicAPI.Shipped.txt | Updates shipped API signature for ProcessHelper.LaunchProcess. |
| src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/netcoreapp3.1/PublicAPI.Shipped.txt | Updates shipped API signature for ProcessHelper.LaunchProcess. |
| src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/net8.0/PublicAPI.Shipped.txt | Updates shipped API signature for ProcessHelper.LaunchProcess. |
| src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/net462/PublicAPI.Shipped.txt | Updates shipped API signature for ProcessHelper.LaunchProcess. |
| src/Microsoft.TestPlatform.PlatformAbstractions/PublicAPI/PublicAPI.Shipped.txt | Updates shipped API signature for IProcessHelper.LaunchProcess. |
| src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/System/IProcessHelper.cs | Extends interface method signature with createNoNewWindow. |
| src/Microsoft.TestPlatform.ObjectModel/RunSettings/RunConfiguration.cs | Adds setting property, XML read/write logic, and default. |
| src/Microsoft.TestPlatform.ObjectModel/PublicAPI/PublicAPI.Unshipped.txt | Adds new public API for RunConfiguration.CreateNoNewWindow. |
| /// <param name="createNoNewWindow">When true, prevents the process from creating a new console window. Default is true.</param> | ||
| /// <returns>The process created.</returns> | ||
| object LaunchProcess(string processPath, string? arguments, string? workingDirectory, IDictionary<string, string?>? envVariables, Action<object?, string?>? errorCallback, Action<object?>? exitCallBack, Action<object?, string?>? outputCallBack); | ||
| object LaunchProcess(string processPath, string? arguments, string? workingDirectory, IDictionary<string, string?>? envVariables, Action<object?, string?>? errorCallback, Action<object?>? exitCallBack, Action<object?, string?>? outputCallBack, bool createNoNewWindow = true); |
There was a problem hiding this comment.
Changing the IProcessHelper method signature is a binary-breaking change for any external implementers and consumers compiled against the previous interface (and for callers of ProcessHelper.LaunchProcess compiled against the old 7-parameter method, as optional parameters don’t preserve the old runtime signature). To avoid breaking, keep the existing signature and introduce a new API surface (e.g., a new method name or a new interface like IProcessHelper2), then have internal code prefer the new API while maintaining the old one for compatibility.
| .Callback<string, string, string, IDictionary<string, string>, Action<object, string>, Action<object>, Action<object, string>, bool>( | ||
| (var1, var2, var3, dictionary, errorCallback, exitCallback, outputCallback, createNoNewWindow) => | ||
| { | ||
| var process = Process.GetCurrentProcess(); | ||
|
|
There was a problem hiding this comment.
The updated tests accept createNoNewWindow but don’t assert its value. Since this PR introduces user-visible behavior (opening/not opening a new console window), add assertions in the relevant launch-process callbacks to validate the default (true) and the runsettings-driven value (false) are actually being passed through the managers to IProcessHelper.LaunchProcess.
|
not sure how to integration test it, on windows you can run test, make the test wait for you. Then you look at the child processes of current test, and see if testhost has conhost, or not. IMHO |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| public void ReportConsoleWindowStatus() | ||
| { | ||
| var hasConsoleWindow = GetConsoleWindow() != IntPtr.Zero; | ||
| Console.WriteLine($"HAS_CONSOLE_WINDOW={hasConsoleWindow}"); |
There was a problem hiding this comment.
This output won't reach the output when test passes, instead throw exception that will have your info, and match that in the assertions.
|
|
||
| InvokeVsTest(arguments); | ||
|
|
||
| ExitCodeEquals(0); |
There was a problem hiding this comment.
you don't have any assertion, and the test project will exit with code 1 which is fine, you need to look at the output to detect the message about new window
There was a problem hiding this comment.
you must ensure HAS_CONSOLE_WINDOW=True in standard output
| InvokeVsTest(arguments); | ||
|
|
||
| ExitCodeEquals(1); | ||
| StdErrorContains("HAS_CONSOLE_WINDOW=False"); |
| InvokeVsTest(arguments); | ||
|
|
||
| ExitCodeEquals(1); | ||
| StdErrorContains("HAS_CONSOLE_WINDOW=False"); |
| /// <param name="outputCallBack">Call back for on process output</param> | ||
| /// <param name="createNoNewWindow">When true, prevents the process from creating a new console window.</param> | ||
| /// <returns>The process created.</returns> | ||
| object LaunchProcess(string processPath, string? arguments, string? workingDirectory, IDictionary<string, string?>? envVariables, Action<object?, string?>? errorCallback, Action<object?>? exitCallBack, Action<object?, string?>? outputCallBack, bool createNoNewWindow); |
There was a problem hiding this comment.
add parameter to the method above
Add a new RunConfiguration setting 'CreateNoNewWindow' that controls whether testhost processes are started with CreateNoWindow=true. Default is true, preserving existing behavior. When set to false in runsettings XML, the testhost process is allowed to create a new console window, enabling child processes and native applications to propagate their console output. Changes: - Add CreateNoNewWindow property to RunConfiguration (default true) - Add XML serialization/deserialization for the setting - Add createNoNewWindow parameter to IProcessHelper.LaunchProcess - Pass the setting through DefaultTestHostManager and DotnetTestHostManager - Update PublicAPI declarations - Add unit tests for the new setting Closes microsoft#15584 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uses GetConsoleWindow P/Invoke in a net462 test asset to verify: - CreateNoNewWindow=false → testhost HAS a console window - CreateNoNewWindow=true → testhost has NO console window - Default (not set) → testhost has NO console window (preserves existing behavior) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…assertions - Pass original string value in CreateNoNewWindow parse error (not boolValue) - Add LaunchProcess overload instead of breaking IProcessHelper signature - Assert createNoNewWindow value in DefaultTestHostManager tests - Use Assert.IsTrue in ConsoleWindowCheck for reliable output capture Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Data collectors call the old 7-param LaunchProcess, not the new 8-param overload. Fix Verify calls to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nIsolation only - ConsoleWindowCheck: throw Exception instead of Assert.IsTrue so the HAS_CONSOLE_WINDOW status always appears in the error message output - CreateNoNewWindowTests: use InIsolation only (InProcess does not launch a separate testhost, so CreateNoNewWindow has no effect) - Check StdOutputContains for HAS_CONSOLE_WINDOW=True/False - All tests expect exit code 1 (test always throws) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9c1f417 to
d405297
Compare
The ConsoleWindowCheck P/Invoke test was unreliable in headless/CI
environments. Replace with diag log checks that verify the
CreateNoWindow value is correctly passed through to process launch:
- Add CreateNoWindow={value} to EqtTrace in both DefaultTestHostManager
and DotnetTestHostManager
- Acceptance tests enable /Diag and check logs for the correct value
- Remove ConsoleWindowCheck test asset (P/Invoke GetConsoleWindow)
- Tests use InIsolation only (InProcess has no separate testhost)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #15584
Summary
Add a new
CreateNoNewWindowRunConfiguration setting that controls whether testhost processes are started withProcessStartInfo.CreateNoWindow = true.Default:
true- preserving existing behavior. When set tofalsein runsettings XML, the testhost process is allowed to create a new console window, enabling child processes and native applications to propagate their console output.Usage
Changes
CreateNoNewWindowproperty toRunConfiguration(defaulttrue)createNoNewWindowparameter toIProcessHelper.LaunchProcessDefaultTestHostManagerandDotnetTestHostManager