Skip to content

test_runner: gate event-loop drain and on_exit to node:test, drop BUN_TEST_DRAIN_EVENT_LOOP - #35401

Open
robobun wants to merge 5 commits into
mainfrom
farm/46ad8752/node-test-drain-event-loop
Open

test_runner: gate event-loop drain and on_exit to node:test, drop BUN_TEST_DRAIN_EVENT_LOOP#35401
robobun wants to merge 5 commits into
mainfrom
farm/46ad8752/node-test-drain-event-loop

Propagate drain-surfaced uncaught errors to the exit code

fd5856e
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jul 24, 2026 in 23m 0s

Code review found 2 potential issues

Found 2 candidates, confirmed 2. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 2
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/runtime/cli/test_command.rs:3065-3077 Exit handlers receive stale code=0 for drain-surfaced errors
🟡 Nit test/cli/test/bun-test.test.ts:1531-1536 drain-throw fixture's setTimeout(500) adds unnecessary 500ms to every run

Annotations

Check warning on line 3077 in src/runtime/cli/test_command.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

Exit handlers receive stale code=0 for drain-surfaced errors

The `unhandled_error_counter > prev_unhandled` check at line 3075 runs *after* `on_exit()` has already dispatched `process.on('exit')` handlers, so a drain-surfaced throw makes handlers see `code === 0` even though the process will exit 1. Node's `common.mustCall()` checker starts with `if (exitCode !== 0) return;`, so this prints spurious 'Mismatched' noise on top of the real error. Move the check inside the closure, between `handle_rejected_promises()` and `(*vm_ptr).on_exit()` (the post-closu

Check warning on line 1536 in test/cli/test/bun-test.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

drain-throw fixture's setTimeout(500) adds unnecessary 500ms to every run

The `setTimeout(..., 500)` in the drain-throw fixture gives this test a hard 500ms wall-clock floor — the spawned `bun test` must spin `on_before_exit()` until the timer fires before it can exit. The 500ms was carried over verbatim from the ad-hoc repro; since the fixture's only test is a synchronous no-op and timers don't fire during collection/`tick_immediate_tasks`, a much smaller delay (~20ms — the reviewer's own analysis used 10ms) exercises the identical end-of-run drain path. REVIEW.md: '