Stop asserting on the machine's whole cmd.exe population - #11
Merged
Merged
Conversation
The parent-and-child test failed on main right after #10 merged, then passed on a plain re-run. It asserts `Get-Process cmd` is empty, which is a query across every cmd.exe on the host rather than the one the test started, so any shell a CI agent happens to be running fails it. It now holds the process object from Start-Process and checks HasExited, which no name collision or PID reuse can spoof. The test is admin-gated, so it only ever runs in CI. Reading Stop-ProcessTree to confirm the contract turned up a real bug next to it: the per-id loop resolved the entire -ProcessId array instead of the current id, so the first id stopped every requested process. Later ids were already dead by their turn, Win32_Process reported no children for them, and their descendants were orphaned rather than stopped. Only single-id pipeline calls happen today, which is why it stayed hidden. - Assert HasExited on the spawned process instead of a global process-name query - Resolve $id, not $ProcessId, so each tree is walked before it is killed - Add mocked Stop-ProcessTree tests, verified to fail against the previous line - Bump to 1.4.1
Test Results162 tests +5 162 ✅ +5 8m 35s ⏱️ -33s Results for commit 491478d. ± Comparison against base commit f304118. This pull request removes 7 and adds 12 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
SummarySummary
Coveragesrc/Private - 80%
src/Public - 93.2%
|
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
1 participant
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.
Stop-DevProcess.Tests.ps1assertedGet-Process cmd | Should -BeNullOrEmpty— a query acrossevery
cmd.exeon the host, not the one the test started. Any shell a CI agent happens to berunning falsifies it, which is why the identical commit passed on the PR build and failed on
maintwelve minutes later.
It now keeps the object from
Start-Process -PassThruand assertsHasExited. That is strictlystronger than a PID lookup: it cannot be fooled by a name collision, nor by PID reuse in the window
between the kill and the assertion.
Worth knowing: these tests are
-Skip:(-not $script:isAdmin), so they never run on a non-admindeveloper machine. CI is the only place they execute, and the only place this could have surfaced.