Stop the process-tree timeout test flashing a console window - #158
Merged
Conversation
The process-tree timeout test spawns `ping -n 30` as a grandchild via `Start-Process`, which defaults to UseShellExecute and therefore allocates a new console. On Windows 11 that surfaces as a Windows Terminal window that flashes on screen for the ~2s until the deadline kills the tree. `ProcessRunner` already starts the outer powershell with CreateNoWindow, but that does not propagate to a grandchild launched through Start-Process. `-NoNewWindow` makes ping inherit the parent's (absent) console instead. The test's assertions are unaffected: -PassThru still returns the child, the pid file is still written, and the process-tree relationship under test is unchanged.
There was a problem hiding this comment.
Pull request overview
Prevents the Windows process-tree timeout test from flashing a console window.
Changes:
- Adds
-NoNewWindowwhen spawningping. - Documents why console inheritance is required.
Show a summary per file
| File | Description |
|---|---|
src/Tests/ProcessRunnerTests.fs |
Suppresses the temporary ping console window. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
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.
Problem
A terminal window periodically flashed on screen for about a second on my dev machine and vanished before it could be read. It turned out to be this test suite.
ProcessRunnerTests.fs— "timeout returns a typed error and terminates the process tree" — spawnsping -n 30as a grandchild to proveProcessRunner's deadline kills a whole process tree, not just the direct child:Start-Processdefaults toUseShellExecute = $true, so Windows allocates a new console forping.exe. On Windows 11, where Windows Terminal is the default console host, that appears as a Windows Terminal window. Two seconds later the timeout kills the tree and the window disappears.ProcessRunneralready starts the outerpowershell.exewithCreateNoWindow = true(ProcessRunner.fs), but that does not propagate to a grandchild launched throughStart-Process— which is exactly the gap.The test carries
Category("Unit")/Category("Fast"), so unlike the other window-spawning fixtures (SessionManagerSpawnTests,ActionLaunchSpawnTests), it is not gated behind[<Explicit>]and runs in every normal test run. With several worktrees and background agents running the suite, the flashes appeared at unpredictable intervals.I tracked this down with a process/window watcher, which caught the exact ancestry:
Fix
Add
-NoNewWindowto theStart-Processcall.pingthen inherits the parent PowerShell's console — and since that parent is started withCreateNoWindow = true, there is no console to inherit and nothing is displayed.-NoNewWindowis preferred over-WindowStyle Hidden: the latter is a display hint that only applies withUseShellExecuteand is ignored by some hosts, whereas-NoNewWindowsuppresses console allocation outright.Everything the test asserts is unchanged:
-PassThrustill returns the child, the pid file is still written, and the parent/child process-tree relationship under test is untouched. The non-Windowssh/sleep 30branch needed no change.Tests
dotnet test src/Tests/Tests.fsproj --filter "FullyQualifiedName~ProcessRunner"I also A/B tested the window suppression directly, reproducing how
ProcessRunnerstarts the outer shell (UseShellExecute = false,CreateNoWindow = true) and counting visible console-class top-level windows across the run:-NoNewWindow)-NoNewWindow)The window is gone, and the child pid is still captured — confirming the fix is cosmetic-only and the test's guarantees hold.