test_runner: flush report stream before force exit - #64875
Open
Navaneethp007 wants to merge 1 commit into
Open
Conversation
Signed-off-by: Navaneeth Prabha <nvps742@gmail.com>
Collaborator
|
Review requested:
|
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.
Summary
Fixes: #64833
When
--test-force-exitis used together with process isolation, each test file runs in a child process that streams its results back to the test runner's parent process over itsstdout, which is a pipe. Writes to a pipe are asynchronous, so theprocess.exit()issued on force exit could quit the process while report frames were still buffered in the pipe. The trailingtest:pass/test:failevents were silently dropped, yet the process still exited with code0— so verdicts disappeared with no error and no failing exit status.The flush loop added in #55099 only drains reporter destinations that expose a
close()method (for example file destinations). A pipe has noclose(), so that branch resolved immediately without flushing anything, leaving the async pipe writes to be truncated by the exit. This is why the loss only reproduces where the pipe is non-blocking (Linux); on Windows named pipes, TTYs, and file destinations the writes are effectively blocking, so the output looked clean.Fix
In the force-exit path, for a destination that has no
close()(the child-process pipe case), switch the underlying handle to blocking with_handle.setBlocking(true)— the same idiom Node already uses before exit inlib/net.jsandlib/tty.js— thenend()the stream and await it, so any queued bytes are flushed to the OS before the process exits. Existing file destinations continue to use theclose()path unchanged.Additionally, yield once via
setImmediateafter tearing down the harness and beforeprocess.exit(), so report events that have already arrived on the parent's readable side get dispatched to the reporters before exit. This part follows the approach proposed in #64857 by @NeverBetterEnough; on its own a single tick is probabilistic under concurrency and did not fully eliminate the loss in my testing, but combined with the blocking flush the result is deterministic.Test
Adds
test/parallel/test-runner-force-exit-no-verdict-loss.js, which generates 12 files of 1000 tests each and runs them with--test-force-exitat--test-concurrency=16, then asserts that all 12000 verdicts are present in the reporter output and the process exits0. The test fails onmain(e.g.11532 !== 12000) and passes with this change. Existingtest-runner-force-exit*tests, including the one from #55099, continue to pass.