test_runner: flush report stream before force exit - #64875
Conversation
Signed-off-by: Navaneeth Prabha <nvps742@gmail.com>
|
Review requested:
|
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64875 +/- ##
==========================================
+ Coverage 90.15% 90.16% +0.01%
==========================================
Files 746 746
Lines 242778 242781 +3
Branches 45747 45764 +17
==========================================
+ Hits 218878 218915 +37
+ Misses 15378 15354 -24
+ Partials 8522 8512 -10
π New features to boost your workflow:
|
krsnaSuraj
left a comment
There was a problem hiding this comment.
Review: approach is correct β thanks for the measurement
The head-to-head data in the issue thread is convincing: writableNeedDrain +
drain does not close the race (buffered bytes below highWaterMark), while
_handle.setBlocking(true) + end() gives 0 loss across all runs. The blocking
flush is the right mechanism for the child-side pipe.
Two optional hardening suggestions:
-
Bound the
end()wait β if a destination is already destroyed/errored
before this block runs (e.g. EPIPE on a closed parent), the callback could be
delayed; force-exit must never hang. A short timeout on the child-side wait
(keeping the existingsetImmediateparent-side yield) closes the last edge. -
Windows console β
_handle.setBlockingis guarded bytypeof, good; worth
confirming win-x64 CI passes since console handles differ from pipes.
Otherwise LGTM from my side. This is the right fix for #64833.
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.