test: run process.on('exit') handlers in bun test (exposes vacuous tests) - #34443
test: run process.on('exit') handlers in bun test (exposes vacuous tests)#34443cirospaciari wants to merge 3 commits into
bun test (exposes vacuous tests)#34443Conversation
`bun run` calls vm.on_exit() before global_exit(), which dispatches the process 'exit' event and drains cleanup hooks. The test command only set exit_code and went straight to global_exit(), so exit handlers never ran under `bun test`. This silently weakened the vendored Node.js test suite. node's common.mustCall(fn, N) verifies its counts from a process 'exit' handler (runCallChecks), so every mustCall count was unchecked: a file calling a mustCall(3) callback once still reported success, where node exits 1 with "Mismatched function calls. Expected exactly 3, actual 1." Handlers run before the bun:test GC roots are released, since they are user JS and may touch still-live state.
|
Updated 9:17 PM PT - Aug 3rd, 2026
❌ @robobun, your commit 646e859 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 34443That installs a local version of the PR into your bun-34443 --bun |
WalkthroughChangesTest runner exit handlers
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits: the run_with_api_lock + raw-pointer reborrow is byte-identical to the two existing uses in this function (test_command.rs:2918-2923, 2963-2968), so no novel unsafe. global_exit() does not itself dispatch 'exit' or call on_exit(), so handlers run exactly once, and its debug_assert!(is_shutting_down()) is still satisfied via on_exit(). Leaving the blast-radius call (this now also runs cleanup hooks, profiler flush, and SQLite close under bun test) to a human as the author flagged.
Extended reasoning...
Checked the unsafe pattern against the two neighboring run_with_api_lock blocks in the same function — identical shape, so the SAFETY justification is copied convention rather than new reasoning. Confirmed global_exit() (VirtualMachine.rs:1528) does not dispatch the exit event or invoke on_exit(), ruling out double-dispatch. Confirmed on_exit() sets is_shutting_down before returning, so the debug_assert at the top of global_exit() remains satisfied even with the redundant line removed. Not approving because the author explicitly asks reviewers to weigh the blast radius, and on_exit() does more than fire 'exit' (profiler flush, Bun__closeAllSQLiteDatabasesForTermination, cleanup-hook drain) — all newly reachable from bun test.
| // unique mutable access on this single-threaded path. | ||
| vm.run_with_api_lock(|| unsafe { (*vm_ptr).on_exit() }); | ||
| } | ||
| vm.is_shutting_down = true; |
There was a problem hiding this comment.
🟡 nit: on_exit() already sets self.is_shutting_down = true (src/jsc/VirtualMachine.rs:1505), so this line is now a no-op re-assignment. Per REVIEW.md ("Delete dead code in the same PR that makes it dead … guards a new validator makes redundant"), this line can be dropped.
Extended reasoning...
What
The pre-existing line vm.is_shutting_down = true; at src/runtime/cli/test_command.rs:2950 is now dead. The new block immediately above it calls vm.on_exit(), and VirtualMachine::on_exit() unconditionally executes self.is_shutting_down = true; (src/jsc/VirtualMachine.rs:1505) after ExitHandler::dispatch_on_exit(self). So by the time control reaches line 2950, vm.is_shutting_down is already true, and this assignment re-writes the same value.
Code path
// test_command.rs:2943-2950
{
let vm_ptr: *mut VirtualMachine = vm;
vm.run_with_api_lock(|| unsafe { (*vm_ptr).on_exit() }); // sets is_shutting_down = true
}
vm.is_shutting_down = true; // <-- already true; no-opAnd in on_exit():
// VirtualMachine.rs:1483-1505
pub fn on_exit(&mut self) {
// ... CPU/heap profiler flush ...
ExitHandler::dispatch_on_exit(self);
self.is_shutting_down = true; // <-- unconditional
// ... cleanup hooks ...
}There is no early-return path in on_exit() that skips the assignment — the profiler branches are if let Some(...) guards that fall through, and dispatch_on_exit returns normally.
Why it wasn't dead before
Prior to this PR, test_command.rs never called on_exit(). The vm.is_shutting_down = true; line was load-bearing: it satisfied the debug_assert!(vm.is_shutting_down) in global_exit() and gated script_execution_status checks during teardown. This PR's on_exit() call now covers that responsibility, which is exactly the "guards a new validator makes redundant" case REVIEW.md calls out under Delete dead code in the same PR that makes it dead.
Step-by-step proof
- Execution reaches line 2948 →
run_with_api_lockinvokes the closure →(*vm_ptr).on_exit()runs. - Inside
on_exit(), afterdispatch_on_exit, line 1505 executesself.is_shutting_down = true.selfand*vm_ptrare the sameVirtualMachine(same live&mut). on_exit()returns; the closure returns;run_with_api_lockreturns.- Line 2950 executes
vm.is_shutting_down = true— the field is alreadytrue. No observable state changes.
Impact
Zero behavioral impact — a redundant boolean write. This is purely a cleanliness nit; nothing breaks if it merges as-is.
Fix
Delete line 2950 (vm.is_shutting_down = true;). The comment block above the GC-root release still reads correctly without it.
| // Run `process.on('exit')` handlers like `bun run` does. Node's test | ||
| // harness verifies mustCall() counts from one, so skipping them made | ||
| // those assertions silently pass. Must precede the GC-root release | ||
| // below: handlers are user JS and may touch still-live state. |
There was a problem hiding this comment.
🟡 This comment is 4 lines, exceeding the repo's 3-line max (CLAUDE.md rule 13), and its middle sentence ("Node's test harness verifies mustCall() counts from one, so skipping them made those assertions silently pass") is bug-history rationale that REVIEW.md says belongs in the PR description — where it already lives verbatim. Dropping that sentence leaves the durable content (run exit handlers like bun run; must precede the GC-root release since handlers are user JS) fitting cleanly in ≤3 lines.
Extended reasoning...
What the finding is
The new comment at src/runtime/cli/test_command.rs:2939-2942 spans four lines:
// Run `process.on('exit')` handlers like `bun run` does. Node's test
// harness verifies mustCall() counts from one, so skipping them made
// those assertions silently pass. Must precede the GC-root release
// below: handlers are user JS and may touch still-live state.Two repo rules apply here. CLAUDE.md rule 13 states: "Keep code comments to 3 lines max — Comments must be concise. If the code needs more explanation than that, it belongs in docs." And REVIEW.md's code-style section says: "Comments carry only durable non-obvious content: invariants, ownership/lifetime contracts, SAFETY justifications, deliberate deviations from upstream. No narrating what the code does, no bug history — that belongs in the PR description."
Why the middle sentence is the offender
The comment carries three distinct pieces of information:
- What — run
process.on('exit')handlers likebun rundoes (durable: describes the behavior and its precedent). - Why historically — "Node's test harness verifies mustCall() counts from one, so skipping them made those assertions silently pass" (bug history: describes the symptom that motivated the change).
- Invariant — must precede the GC-root release below because handlers are user JS and may touch still-live state (durable: an ordering constraint a future editor needs to preserve).
Piece (2) is precisely the kind of content REVIEW.md routes to the PR description — and indeed the PR description's "Why it matters" section already covers the common.mustCall / runCallChecks mechanism in far more detail than a one-line comment ever could. A reader who wants the motivation will git blame this block and find the PR; the in-source comment doesn't need to duplicate it.
Step-by-step: trimming to fit
Remove sentence (2) and re-wrap:
// Run `process.on('exit')` handlers like `bun run` does. Must precede
// the GC-root release below — handlers are user JS and may touch
// still-live state.That is 3 lines, keeps both durable facts (the bun run parity and the ordering invariant), and drops only the bug-history rationale. Nothing a future maintainer needs at the call site is lost.
Impact and severity
This is purely a comment-style violation of two explicit repo rules — no runtime behavior is affected, so it is a nit, not a merge blocker. Note there is a pre-existing 5-line comment a few lines below (2951-2955) that also exceeds the limit, but that is untouched by this PR and doesn't excuse a new violation in freshly-added code.
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); |
There was a problem hiding this comment.
🟡 Both new tests pipe stdout and stderr but only drain one of them: the first test sets stderr: "pipe" yet awaits only proc.stdout.text(), and the second leaves stdout at its default "pipe" yet awaits only proc.stderr.text(). Per REVIEW.md's "Subprocess tests: drain pipes concurrently" rule — and matching the scanner tests at the bottom of this same file — use the three-way Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]). The undrained streams here are only a few hundred bytes so it won't actually deadlock, but it diverges from the file-local convention.
Extended reasoning...
What the issue is
REVIEW.md's harness rules include an explicit line item: "Subprocess tests: drain pipes concurrently. Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) — an unread pipe fills the ~64KB OS buffer and deadlocks the child." Both new tests in this PR pipe both stdout and stderr from the child but only read one of them before awaiting proc.exited.
The two sites
Test 1 — "runs process.on('exit') handlers" (test/cli/test/bun-test.test.ts:1476):
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "exit.test.ts"],
...
stderr: "pipe", // stderr piped
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);stderr is explicitly set to "pipe" (and stdout defaults to "pipe" in Bun.spawn), but only proc.stdout.text() is awaited. The child's stderr — which receives the bun test summary (1 pass / 0 fail / Ran 1 test...) — is never drained.
Test 2 — "an exit handler can fail the run..." (test/cli/test/bun-test.test.ts:1497):
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "exit-code.test.ts"],
...
stderr: "pipe",
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);Here stdout defaults to "pipe" but only proc.stderr.text() is awaited — stdout is never drained.
Why it's flagged even though nothing prevents it from passing
The child in each case runs a single trivial test and emits well under 1KB on either stream — nowhere near the ~64KB OS pipe buffer — so these tests will not actually deadlock. However:
- REVIEW.md lists the three-way
Promise.allas an explicit harness convention ("Copy harness conventions exactly" / "Match the exact file's local conventions"). - The scanner tests at the bottom of this same file already follow the pattern verbatim:
so the new tests diverge from the file-local convention as well as the repo-wide one.
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
Step-by-step proof
Bun.spawndefaultsstdoutto"pipe"; test 1 additionally setsstderr: "pipe"→ both streams are open pipes.- The child (
bun test exit.test.ts) writes the test summary to stderr andconsole.log("exit handler ran")to stdout. - The parent awaits
Promise.all([proc.stdout.text(), proc.exited])— nothing ever callsproc.stderr.text(), so the stderr pipe sits with unread bytes until theawait usingdisposer closes it after the test body. - Symmetrically in test 2, stdout is piped by default and never read.
- If either child ever grew to emit >~64KB on the undrained stream (e.g. debug-build noise, ASAN output on stderr, or a future change to
bun testoutput), the child'swrite()would block on a full pipe andproc.exitedwould never resolve.
Fix
Match the neighboring scanner tests:
const [stdout, stderr, exitCode] = await Promise.all([
proc.stdout.text(),
proc.stderr.text(),
proc.exited,
]);in both tests (discarding whichever stream the assertions don't need). This is a two-line change per test with no behavioral risk.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
cirospaciari
changed the title
bun testbun test (exposes vacuous tests)
|
Moving this to draft — CI is red, and the failures are the point of the PR rather than regressions. Worth reading before this lands. Every failure so far has the same signature: the test reports That is Proof it's exposure, not regression — same file, same command, only the binary differs:
The file it spawns isn't vendored, so it has never actually run. Same story for the other two. What's red, and why
I vendored What I'd suggestThis change is only landable together with the cleanup it forces. Two of the three are already fixed by #32631, so the natural move is to rebase this onto that branch and handle Note CI is still running (~98/286 jobs at the time of writing), so more |
|
Root-caused the third failure ( It isn't about workers. It's the runner's process lifetime after a synchronous test returns. That test does: it('...should not affect zero-fill in other threads', () => { // sync — returns immediately
const w = new Worker(/* posts on setInterval */, { eval: true });
w.on('message', common.mustCallAtLeast(...)); // satisfied later, by the event loop
});Node's runner drains the event loop before exiting, so the messages arrive and the
So the test never had a chance to assert; this PR just makes that visible instead of printing That's a deliberate-looking design difference (a runner that waits for stray handles can hang), so I'm not going to change it unilaterally. It does mean the three failures split cleanly:
Two of three are handled by rebasing this onto #32631. The third needs your view on runner lifetime. Worth stating plainly: every one of these was green before this change while asserting nothing, and I found two more the same way ( |
…#34444) Implements `node:test`'s `run()` and raises the surrounding `node:test` behavior to Node v26.3.0. Stacked on #34443 (its first commit), which is landable on its own. Every behavior below was diffed against the real `node v26.3.0` binary, not read off the source. ## `run()` - Node-exact option validation in Node's order (`runner.js:731-909`), including the mutually-exclusive pairs: `forceExit`×`watch`, `shard`×`watch`, `globPatterns`×`files`, `env`×`isolation:'none'`. - `TestsStream`: a `Readable` in objectMode with Node's buffering, emitting each message as both an event and a stream chunk. - Each file runs in its own `bun test` child, spawned with `NODE_TEST_CONTEXT` (Node's own variable — upstream tests branch on it to tell parent from child). The child streams one JSON event per line; the parent republishes, aggregates, and emits a per-file and a run-level `test:summary`. - Node's recursion guard, so a file calling `run()` on itself doesn't fork forever. - Options that can't be honored yet (`watch`, `coverage`, `shard`, `isolation:'none'`, `globPatterns`, `globalSetupPath`) throw rather than being silently ignored. Event fidelity added in this round, each verified against Node: - The **file-level test node** that process isolation reports: `test:enqueue`/`test:dequeue` up front, `test:complete` at the end, and `test:fail` with `failureType: 'testCodeFailure'` when the file itself dies (a top-level throw, a missing file). When the file's *tests* fail instead, the completion carries `subtestsFailed` and no `test:fail` — which is what Node does. - **Skip and todo directive events**, which bun never emitted at all: `bun test` doesn't invoke those bodies, so nothing reported them. - `details.type`, suites counted only in `suites`, and `failureType` preserved across the child process boundary. ## `expectFailure` (xfail) Node v26's `expectFailure` was missing entirely. Adds the option parser (string label, function/RegExp validator, object form with `label`/`match`, and the empty-object rejection), the inverted verdict — a failing body is the expected outcome, a passing one fails with `failureType: 'expectedFailure'` — and the `expectFailure` field on the reported event. ## Two Node divergences reachable from plain `bun test` Not `run()`-specific; these affect anyone using `node:test` under bun today. - **A skipped suite ran its callback.** Node never invokes it, so its children are never declared and its side effects never happen. bun ran the body and registered the children. - **`{ skip: true, todo: true }` was treated as a todo.** Node checks `skip` first, for both tests and suites. ## Test-runner harness A vendored test that only *drives* `run()` is the parent of that run, not a test file — Node executes it as a plain script, and under `bun test` a file that registers no tests of its own exits before its `run()` finishes, delivering zero events. The runner now picks `bun run` for those, gated so that a file with any unindented registration of its own keeps `bun test` rather than silently "passing" having tested nothing. **4 of the 88 vendored `node:test` files change subcommand, all of them added by this PR.** ## Coverage Vendors 5 upstream tests — `test-runner-expect-error`, `test-runner-expect-error-but-pass`, `test-runner-todo-skip-tests`, `test-runner-filetest-location`, `test-runner-tags-experimental-warning` — taking the upstream `test_runner` suite from **20 to 26 of 81**. bun's own `node-test.test.ts` goes 40 → 44 (new fixtures cover `expectFailure` and the suite-skip semantics). All 20 previously-vendored `test-runner-*` files still pass. ## Still draft: the reporting seam A test whose failure is decided **after** its body resolves is still reported as passing by `run()`. Repro: `test/fixtures/test-runner/plan/timeout-basic.mjs` — run directly under `bun test` it is correctly 1 pass / 1 fail (bun's plan logic is fine), but the JS layer has already returned by the time the native runner attributes the async throw, so `run()` sees two passes. I confirmed the JS layer cannot close this: bun's `bun test` intercepts in-process throws natively and attributes them itself, so `process.on('uncaughtException')` — the mechanism Node's own `harness.js:255` uses — never fires for an in-test throw. A faithful `run()` needs a **reporter hook in the native runner**, emitting where bun actually decides pass/fail (`Execution::on_sequence_completed`, `src/runtime/test_runner/Execution.rs`, already the single fan-out point for the CLI reporter, the junit writer and the inspector agent). That's a maintainer design call, which is why this stays draft. Happy to implement it in this PR or a follow-up — it's also the unlock for `--test`-style CLI emulation and `node:test/reporters`, which together account for ~25 of the remaining 55 upstream test_runner files. ## Update (2026-07-17) Three follow-up commits landed on this branch after review: - **Event-loop drain for node tests** (`BUN_TEST_DRAIN_EVENT_LOOP=1`, set by the vendored-test runner; also on for run() children): a node test process only exits when its loop drains, and `common.mustCall()` verifies counts in `'exit'` handlers. This fixes the five vendored tests that started failing once exit handlers ran (`test-worker-arraybuffer-zerofill`, `test-file-write-stream5`, `test-tls-psk-alpn-callback-exception-handling`, and with the two fixes below, `test-events-add-abort-listener` and `test-net-server-async-dispose`). Off by default — bun suites keep exit-after-tests. - **`events.addAbortListener` now survives `stopImmediatePropagation`**: it registers a native abort algorithm (runs in `runAbortSteps()` before event dispatch) instead of an `'abort'` listener, since the native EventTarget drops node's `[kResistStopPropagation]` option. `node:events` had a duplicate inline implementation bypassing `internal/abort_listener`; it now delegates. - **`Server[Symbol.asyncDispose]` resolves when not listening** (node's `lib/net.js` guard); a second dispose used to reject with `ERR_SERVER_NOT_RUNNING` and re-emit `'close'`. Each verified byte-identical to the node v26.3.0 binary on minimal repros. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 26 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
…handlers # Conflicts: # src/runtime/cli/test_command.rs # test/cli/test/bun-test.test.ts
2 participants
What
bun testnever ranprocess.on('exit')handlers.bun runcallsvm.on_exit()beforeglobal_exit(), which dispatches the process'exit'event and drains cleanup hooks.test_command.rsonly setexit_codeand went straight toglobal_exit(), so handlers registered underbun testnever fired.Why it matters
This silently weakened the vendored Node.js test suite. Node's
common.mustCall(fn, N)— the most-used helper in that suite — verifies its counts from a process'exit'handler (runCallChecks,test/js/node/test/common/index.js). With handlers never dispatched, everymustCallcount went unchecked.A file that calls a
mustCall(3)callback once:0✅11 pass, 0 failMismatched <anonymous> function calls. Expected exactly 3, actual 1.After this change bun matches node exactly on both.
This is not hypothetical — it was masking real problems. Two vendored
node:testfiles were passing while asserting nothing:test-runner-xfail.js— spawns a child and matches its stdout against node's TAP reporter output, whichbun testdoes not emit; the parent also finished before the child'scloseevent, so themustCallwrapping those assertions never ran. (Already removed separately.)test-runner-plan.mjs— itsrun()-based assertions never fired; with this change it correctly reportsMismatched ... Expected exactly 1, actual 0.Ordering: handlers run before the
bun:testGC roots are released, since they are user JS and may touch still-live state.Testing
Two tests in
test/cli/test/bun-test.test.ts: one asserting an exit handler runs, one asserting an exit handler can set a failing exit code (thecommon.mustCallshape). Both fail withUSE_SYSTEM_BUN=1and pass with the built binary.Ran the vendored
test_runnersuite against this change: 20/20 still pass, so no test depended on handlers being skipped.test/cli/test/bun-test.test.tsis 83/83. Also spot-checkedexpect.test.js(406),event-emitter.test.ts(67),mock-fn.test.js(77),node-test.test.ts(39) — all green.Reviewers may want to consider the blast radius: this affects every
bun testrun, and any suite whose files registerprocess.on('exit')handlers will now execute them (which is what node and jest both do).