Repository navigation
Kill a timed-out test's whole process group in the test runner - #241
Merged
Merged
Conversation
The timeout killed only the npx wrapper; the orphaned tsx kept the output pipes open, so one hung test stalled the run until someone killed it by hand.
Merged
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.
The test runner's 180s timeout sent SIGKILL to the
npxwrapper only. Thetsxprocess under it was orphaned and kept the output pipes open. The runner resolves a test on the pipes closing, so a single hung test stalled the whole run until someone killed that process by hand. This happened twice in Phase 30 (slices E and G), both times on a one-off Chrome hang intest-text-props.ts, and the second time a full-tier run sat for 25 minutes.The fix
detached: true), and the timeout kills the whole group. The hung test is reported as timed out and the run moves on.FRAMESMITH_TEST_TIMEOUT_MSoverrides the 180s timeout, so the new test can run in seconds.Known limit
A browser that Puppeteer launched can still outlive a killed test. Puppeteer starts Chrome in a process group of its own, so killing the test's group doesn't reach it. That was already true before this change.
Testing
test-runner-timeout.ts(fast tier) runs the real runner in a temp dir with one test that never exits and one that passes, using a 3s timeout. It checks four things: the run finishes, the hung test is reported as timed out, the other test still passes, and no process of the hung test is left running. It fails on the old runner (the run never finished; its own 45s guard tripped) and passes in 5s now.npm test: 80/80.