Conversation
With --watch --bail, the first failing run printed the bail banner and called Global::exit(1) from inside the per-test completion callback, terminating the watcher. The fixing edit never re-ran. Give the serial runner the same flag-based bail the parallel coordinator already has: Execution.aborted stops the per-file step loops and CommandLineReporter.bailed stops the per-file and --rerun-each loops, so control returns to TestCommand::exec. Under --watch that reaches run_event_loop_for_watch; otherwise the existing Global::exit(1) path is unchanged.
WalkthroughChangesWatch-mode bail handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:18 AM PT - Jul 22nd, 2026
❌ @robobun, your commit e16c003 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35094That installs a local version of the PR into your bun-35094 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
All three prior nits are now addressed (the fixture tests were renamed to "test #1"/"test #2", and the two bail sites now share the hoisted let watching = ... shape with summary → banner ordering preserved for non-watch). No new issues found. Deferring to a human for the control-flow change itself — flag-based unwind through Execution.aborted and CommandLineReporter.bailed across three step loops, the per-file loop, and the --rerun-each loop.
What was reviewed:
- Non-watch
--bailpath is byte-for-byte behavior-preserving at both sites (handle_test_completedand the module-load-rejection arm inTestCommand::run). abortedguards instep_group,step_group_one, andstep's per-group loop returnCompletebefore starting the next sequence — same effective skip set as the oldGlobal::exit(1).- New test polls stderr for observable conditions (no sleeps), asserts
test #2never ran, then edits the dep and awaits2 passfrom the re-run;await using proccovers cleanup. - The two unrelated CI failures (
test-gc-http-client-connaborted.js,test-net-connect-memleak.js) are pre-existing flakes on x64 Linux, not touched by this diff.
Extended reasoning...
Overview
The PR changes bun test --watch --bail so that hitting the bail threshold no longer calls Global::exit(1) and kills the watcher. Instead it sets two flags — CommandLineReporter.bailed (stops the per-file loop in run_all_tests and the --rerun-each repeat loop) and Execution.aborted (stops step_group, step_group_one, and the per-group loop in Execution::step) — so the run unwinds back to exec, which prints the summary and enters run_event_loop_for_watch. The non-watch path is unchanged: it still prints summary → banner → junit → Global::exit(1). The module-load-rejection bail arm in TestCommand::run gets the same treatment. A new test in test/cli/test/bun-test.test.ts spawns bun test --watch --bail, waits for the bail banner and the end-of-run summary, asserts the second test never ran, edits the failing dep, and waits for 2 pass from the re-run.
Security risks
None. This is test-runner control flow; no untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Moderate. The change is small (~60 lines of Rust across two files, plus a test) and follows the same flag-and-drain pattern the parallel coordinator already uses, but it threads a new early-exit through five loop sites in the serial runner. Getting the unwind order wrong could leave the watcher in an odd state or change what runs after bail. I traced each guard: aborted is set inside handle_test_completed (called from on_sequence_completed inside advance_sequence), and every loop that could start a next sequence/group checks it at the top before doing so, so no additional test bodies run — matching the old hard-exit's effective behavior. Under --watch the next file-change triggers reload_process (exec-in-place), so any skipped afterAll groups on the bailed run don't leak into the next run. That all looks correct, but it's the kind of multi-site control-flow change a maintainer should eyeball.
Other factors
I left three nits on earlier revisions (non-watch output-order regression at the module-load bail site, duplicated banner block, and a ~0.13% flake from not.toContain("t2") colliding with the mkdtemp suffix). All three are fixed in the current diff — the two bail sites now bind watching once and print summary before the banner when not watching, and the fixture tests are named "test #1"/"test #2" with not.toContain("test #2"), matching the sibling --bail tests. The 30s per-test timeout was flagged and ruled out (it's a spawned --watch subprocess awaiting a filesystem-triggered rerun; a deadline is appropriate and await using proc guarantees kill on failure). The two CI failures are unrelated Node-parallel GC/memleak flakes on Linux x64. Given this is a real control-flow change to the test runner rather than a mechanical edit, I'm deferring rather than approving.
|
Diff is green. The only red CI lanes across the last three builds (77612, 77626, 77648) are two unrelated Node.js GC-timing tests on Linux x64 that don't go through the test runner at all:
Both are The new |
There was a problem hiding this comment.
Beyond the inline nit, I traced a few things and they check out: the non-watch --bail path is behavior-identical (same summary→banner→Global::exit(1) order after f2d9e07); Execution.aborted's StepResult::Complete reaches Phase::Done via the same _advance path as natural completion; and neither bailed nor aborted leak across watch reruns (watch reloads via re-exec, and each file's Execution::init() starts aborted: false regardless).
Extended reasoning...
Verified the three earlier review comments were addressed in f2d9e07 and e16c003. Traced the new aborted checks in step_group/step_group_one/step — they're read-only early-returns at loop heads that funnel into the existing StepResult::Complete → _advance completion path, and the fresh .get() reborrow in step_group_one matches the file's existing stacked-borrows pattern. The run_all_tests per-file check and the --rerun-each break correctly unwind to exec, which prints the summary at line 2878 and enters run_event_loop_for_watch. The two CI failures (test-gc-http-client-connaborted.js, test-net-connect-memleak.js) are unrelated to this change.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-22, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#6453) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What
bun test --watch --bailexits the whole watch process on the first bail-out. The edit that would fix the failing test never re-runs. Without--bailthe same watcher survives and reruns on the next edit.Repro
Cause
CommandLineReporter::handle_test_completedcallsGlobal::exit(1)directly when the fail count reaches the bail threshold, inside the per-test completion callback. Under--watchthat kills the process before control reachesrun_event_loop_for_watch. The module-load-rejection bail path inTestCommand::runhas the same hard exit.The parallel coordinator already handles this correctly: it sets a
bailedflag and drains, returning normally.Fix
Give the serial runner the same flag-based bail:
Execution.abortedstops the step loops (step_group,step_group_one,step's per-group loop) so no further tests in the current file run.CommandLineReporter.bailedstops the per-file loop inrun_all_testsand the--rerun-eachrepeat loop inTestCommand::run.Under
--watchthe bail path now prints the banner, sets the flags, and lets the run unwind back toexec, which prints the full summary and entersrun_event_loop_for_watch. When the watched file changes,reload_processre-execs as before. When not watching, the existingGlobal::exit(1)is unchanged.Verification
The test spawns
bun test --watch --bailwith a failing test, waits for the bail banner and the end-of-run summary on stderr, then fixes the dep file and waits for2 passfrom the re-run. Skipped on Windows where--watchuses a respawning parent instead of exec-in-place (same as the existing--changed --watchtest).no test proof · iteration 2 · 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
Supersedes #19918 (same approach against the pre-Rust Zig sources, which no longer exist).
Fixes #20318
Fixes #6453