Skip to content

test_runner: flush report stream before force exit - #64875

Open
Navaneethp007 wants to merge 1 commit into
nodejs:mainfrom
Navaneethp007:fix-test-runner-force-exit-verdict-loss
Open

test_runner: flush report stream before force exit#64875
Navaneethp007 wants to merge 1 commit into
nodejs:mainfrom
Navaneethp007:fix-test-runner-force-exit-verdict-loss

Conversation

@Navaneethp007

Copy link
Copy Markdown

Summary

Fixes: #64833

When --test-force-exit is 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 its stdout, which is a pipe. Writes to a pipe are asynchronous, so the process.exit() issued on force exit could quit the process while report frames were still buffered in the pipe. The trailing test:pass / test:fail events were silently dropped, yet the process still exited with code 0 — 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 no close(), 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 in lib/net.js and lib/tty.js — then end() the stream and await it, so any queued bytes are flushed to the OS before the process exits. Existing file destinations continue to use the close() path unchanged.

Additionally, yield once via setImmediate after tearing down the harness and before process.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-exit at --test-concurrency=16, then asserts that all 12000 verdicts are present in the reporter output and the process exits 0. The test fails on main (e.g. 11532 !== 12000) and passes with this change. Existing test-runner-force-exit* tests, including the one from #55099, continue to pass.

Signed-off-by: Navaneeth Prabha <nvps742@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_runner: --test-force-exit with concurrency silently loses test verdicts (parent reports fewer tests than ran, exit 0)

2 participants