Repository navigation
Conversation
…s failed A preload `beforeAll` or `aroundAll` set-up runs with the first test file. Its failure lived only in that file's execution order, so every later file ran its tests. `BunTestRoot` now records how those hooks ended. `exit_file` writes the record from the file that ran the hooks, and `enter_file` reads it for each file that follows: - After a failure, the file's root scope is failed from the start, as for a file that fails to load. No `describe()` callback, hook or test of it runs. - After a failed `aroundAll` set-up, the preload `afterAll` hooks do not run with the last file. They are inside the `aroundAll`. A file that runs the hooks is never kept from running by the record: its own order skips past its tests when they fail. So `--isolate` and `--parallel` workers behave as before. `--rerun-each` runs the hooks again with each repeat of the first file. A repeat that passes them is not undone by one that fails them, because what it set up is still there. A hook that the runner gives up on (no timeout, and nothing left that can settle it) is not a failed hook here. The later files run, as before.
…r file from running - `test/regression/issue/12782.test.ts`: the snapshot pinned `(pass) bar > should not run`. It now pins that no test of the second file runs. - `test/cli/test/bun-test.test.ts`: each way the hook fails, each driver, a first file with no test to run, and the repeats of `--rerun-each`. One test pins that a later file is still loaded. - `test/js/bun/test/vitest-flavor.test.ts`: a preload `aroundAll` that fails before `runSuite()`, and a `beforeAll` that fails inside one.
Say in the lifecycle page and in the JSDoc of `beforeAll` and `aroundAll` what a hook of a `--preload` script guards when it fails, and that it runs for each file with `--isolate` and on a `--parallel` worker.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 17 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
|
Updated 7:56 PM PT - Nov 8th, 2026
✅ @robobun, your commit 64e0d9d965b17ff39b36252a9b2e1aa36f1ccbfd passed in 🧪 To try this PR locally: bunx bun-pr 48272That installs a local version of the PR into your bun-48272 --bun |
|
Status Reproduced on a debug build of main 2596ac5 and on a 1.4.3 canary build (367d939): cat > preload.ts <<'X'
import { beforeAll } from "bun:test";
beforeAll(() => { throw new Error("preload beforeAll failed"); });
X
for f in a b; do echo "import { it } from \"bun:test\"; it(\"in $f\", () => console.log(\"ran: $f\"));" > $f.test.ts; done
bun test --preload ./preload.ts ./a.test.ts ./b.test.ts
CI passes on 64e0d9d (build 138947). The diff is ready for a maintainer review. PR: #48272 |
There was a problem hiding this comment.
Beyond the inline findings, I also checked the --bail path through deinit_for_exit (it now skips recording the set-up outcome, but the process exits before anything reads BunTestRoot::set_up), the set_up_outcome slice bounds (set_up_hooks stays (0, 0) until load_from_order fills the leading groups, so a failed-root or isolated file reads nothing), and the --rerun-each sticky Passed rule, which matches the PR's stated intent. The REVIEW.md checks that need a run (USE_SYSTEM_BUN=1 / bun bd test on the new tests) were not performed in this review.
Extended reasoning...
The change adds a SetUp record on BunTestRoot and a per-file set_up_hooks count in src/runtime/test_runner/bun_test.rs so a failed preload beforeAll/aroundAll fails the root scope of every later serial file, plus docs, .d.ts JSDoc and tests. It touches no security-sensitive surface. Four inline findings on the later-file behaviour (timings, never-settling hooks, unreleased module-level resources, silent empty-suite reporting) already signal that a human should look; this note only records what else was examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
… preload's set-up failed With `--timings` and `--update-timings`, the serial run recorded how long each file took. After a failed preload set-up no file runs a test, so that time says nothing about the file. A time that is too short starts the file late in the next `--parallel` or `--shard` run. Such a file now keeps the time that the table has, like a file whose worker ended in the middle of it.
There was a problem hiding this comment.
cdec3df covers the --timings/--update-timings point from my earlier review (later files now go through Timings::keep, and the new test-shard test pins it). Beyond the inline nit, I also checked the --parallel worker path: BunTestRoot::set_up is never reset between a worker's files without --isolate, so it can stay Failed, but every reader (failed_set_up_before, the afterAll gate in advance_inner) is keyed on first_last.first, which runner.rs:750 always sets, and set_up_failed() is only read by the serial record_timing path, so a stale value has no effect there.
Extended reasoning...
The latest push adds a CommandLineReporter::record_timing helper in src/runtime/cli/test_command.rs that routes a file blocked by a failed preload set-up through the existing Timings::keep, plus a serial test in test/cli/test/test-shard.test.ts; the runner changes in src/runtime/test_runner/bun_test.rs are otherwise unchanged since the prior review. No security-sensitive surface is touched. Earlier inline threads on later-file afterAll hooks and on silent empty-suite reporting are not changed by this push, so the PR still needs a human decision on those and is not approved.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (1):
- 🔴
src/runtime/test_runner/bun_test.rs:671—After a preload set-up failure, every later file still executes its module body but none of its own afterAll/afterEach…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
…imings test concurrently The timings test compares no durations, so it does not need the serial block.
A file that ran no test after the preload's set-up failed keeps its time too.
Related to #12782
Problem
--preloadscript'sbeforeAllfails,bun teststill runs the later files' tests:(pass) bar > should not run,2 pass, 1 fail.beforeAlldoes not stop tests from running #12782). Since run beforeAll before the first file and afterAll after the last file #23113 (1.3.0) the hook runs with the first file only, and its failure stays in that file's order (BunTest::advance_inner,src/runtime/test_runner/bun_test.rs:1269).Fix
BunTestRootrecords how the preload'sbeforeAllandaroundAllset-ups ended.enter_filefails the root scope of each later file, as for a file that fails to load.--isolateand--parallelrun the same tests as before.test/regression/issue/12782.test.ts,test/cli/test/bun-test.test.ts(11 of 22 new tests fail on main),test/js/bun/test/vitest-flavor.test.ts(4 of 4),test/cli/test/test-shard.test.ts(1 of 1). More in Notes.suite.Background
BunTestRoot::hook_scope. OnlyBunTestRootoutlives a test file.beforeAllmakes the runner jump within its file's order, never to the next file.hook_scope.failed(cannot name the failed stage).Downsides
--isolate: its module body runs, its hooks and tests do not, and its tests are not listed.BunTestgrows from 440 to 456 bytes per test file.Notes
Decisions
.claude/docs/landing-prs.mdasks for a flag when a default changes. This change is the return of the throwing in a global (preload)beforeAlldoes not stop tests from running #12782 fix (Rewrite test/describe, add test.concurrent #22534, 1.2.23). run beforeAll before the first file and afterAll after the last file #23113 removed that fix as a side effect, and its description does not mention failed hooks. A hook that catches its own error keeps every file running.--isolateand on a--parallelworker, the module body of each file runs too, and its hooks and tests do not. The shapes that this PR does not build are "start no later file" (Jest and VitestglobalSetup) and "report a failure for each file" (1.2.23,--isolate,--parallel). One test pins the decision:a file after the failure is still loaded, and reports nothing of its own but a failure to load.Open PRs on the same code
docs/test/lifecycle.mdx, thebeforeAllJSDoc andtest-error-code-done-callback.test.ts. The one that lands second must change those three places. After bun test: fail the hook whose done() received an error #48142, a hook that fails throughdone(err)is a failed hook, and this PR then covers it with no more code.--rerun-eachloop. It merges with this branch with no text conflict, but its test expects[before, "test b", after]after a failed preloadbeforeAll. With both PRs, reruns 2 to N of the first file are later runs and run no test.advance_inner. The second one to land can read the first one's record.12782.test.ts.Infinitymeans no timeout in test(), hooks, setDefaultTimeout() and jest.setTimeout() #48225 and bun test: do not charge a test or hook for a promise rejected before it started #47731 decide how the runner reports a hook that can never settle. See "Not covered".What each build does
Debug builds of main (2596ac5) and of this branch. The preload has a
beforeAllthat throws, unless the row says otherwise.bun test --preload ./preload.ts a b c2 pass, 1 fail, b and c run0 pass, 1 fail, b and c print a header only--isolate a b,--parallel=1 a b, one--parallelworker0 pass, 2 fail--rerun-each=2 a b2 pass, 2 fail0 pass, 2 fail--rerun-each=2 a b, the hook fails on its first call only3 pass, 1 fail--rerun-each=2 a b, the hook passes on its first call only3 pass, 1 fail--bail=1 a baroundAllthat throws beforerunSuite(), 3 filesafterAllrunsafterAlldoes not run, the outeraroundAllfinishesbeforeAllwith no timeout that never settlesThe preload
afterAllhooks still run with the last file after a failedbeforeAll, as before.Reference runs
setupFilesAfterEnvfile with abeforeAllthat throws, 2 files,--runInBand: the hook runs for each file,Tests: 4 failed, 4 total, no test body runs. AglobalSetupthat throws: no test file runs.setupFilesfile with the same hook:Test Files 2 failed,Tests 4 skipped, no test body runs. AglobalSetupthat throws: no test file runs.The rule
aroundAllorbeforeAll).exit_filewrites it from the file that ran the hooks. A hook fails when it throws, rejects, fails anexpect()or times out.--rerun-eachruns the preload hooks again with each repeat of the first file (main does too, bun test --rerun-each: run a preload's beforeAll, afterAll and aroundAll once around the reruns #48226 changes that). A repeat that fails them does not undo a repeat that passed: what it set up is still there. A set-up that cannot run twice, such as a server on a fixed port, keeps the result of main.aroundAllset-up, the preloadafterAllhooks do not run with the last file. They are inside thearoundAll, and a run of one file already skips them. The teardown of an outeraroundAllstill runs.--timingsand--update-timings, a file that ran no test after the failed set-up keeps the time that the table has (Timings::keep, as for a file whose worker crashed). That includes the file that ran the failed hook, with or without--isolate: main writes a time with no test in it there. The coordinator of a--parallelrun still writes that time, as on main.Not covered
unhandledRejectionlistener, that shape turned an exit code of 1 intoRan 0 tests, exit 0. One test pins that this does not happen. bun test:Infinitymeans no timeout in test(), hooks, setDefaultTimeout() and jest.setTimeout() #48225 and bun test: do not charge a test or hook for a promise rejected before it started #47731 carry the rule for such a file.0 pass, 3 fail, 3 errorsfor 3 files), as on main.--bailexits before the preloadafterAll, as on main.--rerun-each, a preloadaroundAllstarts once for each repeat of the first file and finishes once, as on main. bun test --rerun-each: run a preload's beforeAll, afterAll and aroundAll once around the reruns #48226 owns that.beforeAllguards are not listed, in the first file or in a later one. bun test: fix inline snapshot conflict, beforeAll skip, and test.each title reporting #32817 lists them for the first file.Considered and not built
afterAlland thearoundAllteardown are scheduled with the last file, so they need a new home first. The review sized it at about 30 lines intest_command.rs. It changes what a failing run loads, and no other mode of the runner skips the load of a file, so this PR does not do it. Only the test named under Decisions asserts that a later file is loaded.fs.writeSyncor through a child process then printed under the header of the first file, andGITHUB_ACTIONS=trueprinted an::endgroup::with no::group::. The commit is gone. A file with no tests, or with a-tthat matches nothing, prints the same bare header.afterAllhooks of a later file, so that what its module body opens is closed. They would run without thebeforeAllhooks of that file, and a hook such asafterAll(() => db.close())then throws. The file that runs the failed hook does not run its ownafterAllhooks either, on main and here. Three files that each listen on one fixed port at module scope give0 pass, 3 fail, 2 errorson both builds.beforeAllsets and a preloadbeforeEachchecks. It works on main, but it reports every test as a failure, it cannot stop the hooks of the later files, and the documented default stays wrong.--aotwith a first file that the build refused). AOT: front end, test runs and tests for the rewritten compiler #47942 removed that path while this branch was in progress.Measurements
Release builds of main (2596ac5) and of this branch (cdec3df, the last commit that changes code), same build directory.
size -A:.textis 108,257,833 bytes on main and 108,257,577 bytes on this branch (256 fewer)..rodatais 26,780,652 bytes in both. The stripped binary is 138,119,440 bytes in both.llvm-nm -Sandllvm-objdump -donbun-profile, main to this PR:BunTest::run12,402 to 12,421 bytes (2,453 to 2,474 instructions),TestCommand::run13,490 to 13,464 bytes (3,081 to 3,071), the drop ofEnteredFile85 to 261 bytes (25 to 63), the closure ofrun_all_tests1,297 to 1,164 bytes (282 to 247). When a test file exits, it runs 8 of the new instructions. The file that ran the preload's hooks runs 20, and 11 more for each hook.lldb, debug builds:BunTestRoot56 to 56 bytes (one per process),BunTest440 to 456 bytes (one per test file).Debug (ASAN) builds,
test/js/bun/test/scheduling/multi-file(a preload and 2 files, all pass).gdb,nexticount of the own frames ofenter_file,BunTest::init,advance_inner,exit_fileand the three new functions, for one test file: 1,763 on main, 2,208 (first file) and 2,247 (last file) on this branch. Two runs of each build gave the same counts. Nothing is added per test or per group.--rerun-each=2,--isolate) on both builds. JS callbacks in those runs: 8, 16 and 10 on both.WANTS_LOUD=1) of the passing run with no flag,--isolate,--rerun-each=2and--parallel=1 --no-isolate: 0 differing lines between the builds, after line numbers and addresses are removed.Execution.rs,Order.rs,Collection.rsandjest.rs: 0 lines changed.test_command.rschanges only the two places that record a file's time.step_group,step_sequence_oneandgenerate_order_testare 3,716, 14,856 and 4,799 bytes in both builds.beforeAllhooks: 21 JS callbacks on main, 3 on this branch. Test bodies after the failed hook: 4 and 0.valgrind,perf,straceandbloatyare not installed here, so the instruction counts come fromgdbandllvm-objdump.Tests
12782.test.ts: the snapshot no longer has(pass) bar > should not run, and the test now also checks that no test body logs.bun-test.test.ts,a preload whose beforeAll fails: 22 tests. 11 fail on main. The other 11 pin what must not change:--isolate,--parallelworkers,--bail, a hook that fails or passes on its first call only, a first file that fails or hangs in a hook of its own, and a hook that the runner gives up on.vitest-flavor.test.ts: a preloadaroundAllthat throws, returns or times out beforerunSuite(), and abeforeAllthat fails inside a preloadaroundAll.test-shard.test.ts:--update-timingskeeps the time of each file after thebeforeAllof a preload failed. Main writes 115, 8 and 7 ms there.aroundAllstage (3), the hook that the runner gives up on (1), the count of the leading set-up hooks (1).Suites run with
bun bd teston this branch:bun-test,vitest-flavor,bun_test,isolation,failure-skip,rerun-each,test-test,jest-hooks,expect-deferred,vi-utils,parallel,junit,node-test,test-shard, andtest/internal/source-lints. The failures are timeouts of the debug build that a debug build of main has too:instances of a class nested too deeply to print are a RangeError,no other builtin module patches a global class when it loads,each worker has a unique JEST_WORKER_ID and BUN_TEST_WORKER_ID, and two source-lint tests.no test proof · iteration 1 · 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