test runner: fail dependent tests when a hook's done() receives an error

[robobun]

Problem

When a lifecycle hook fails through the done(error) callback, the error is printed as an Unhandled error between tests and every test that depends on the hook is still counted as a pass.

import { describe, beforeAll, test } from "bun:test";
describe("db suite", () => {
  beforeAll((done) => { done(new Error("DB connection failed")); });
  test("reads row", () => { /* would touch the DB */ });
});
// 1 pass, 0 fail, 1 error    <-- the test is reported as PASSING

A synchronous throw in the same hook reports 0 pass, 1 fail. Every hook type has the same divergence (2 tests in the describe block):

hook	() => { throw }	done => done(err)
beforeAll	0 pass, 1 fail	2 pass, 0 fail, 1 error
beforeEach	0 pass, 2 fail	2 pass, 0 fail, 2 errors
afterEach	0 pass, 2 fail	2 pass, 0 fail, 2 errors
afterAll	2 pass, 1 fail	2 pass, 0 fail, 1 error

node:test wraps every hook (before, beforeEach, after, afterEach) in the done-callback form, so a before() that throws under node:test hits this path too. A broken test environment (database down, fixture missing) produces green pass/fail counts in any reporter that consumes them instead of the exit code.

Cause

run_test_callback reports a hook's synchronous throw or promise rejection through BunTest::on_uncaught_exception with the hook's own RefDataValue, so Execution::handle_uncaught_exception finds the sequence and marks it failed.

bun_test_done_callback instead routed done(error) through the VM's generic uncaught_exception. Under bun test that lands in jest::on_unhandled_rejection, which deliberately demotes any error observed while a hook is running to RefDataValue::Start ("unhandled error between tests"), because a stray exception that happens to fire mid-hook may belong to anything. That demotion is wrong for done(error): the user explicitly handed the error to the hook's own completion callback, but the failure never reached the hook's sequence.

Fix

Report done(error) through BunTest::on_uncaught_exception with the done callback's own RefDataValue, the same way the promise-catch path (bun_test_then_or_catch) does. A stale RefDataValue (a done() called after the runner moved on) is still rejected by the existing validity checks in handle_uncaught_exception and falls back to the unhandled-error report.

run_test_callback attaches the ref only after the callback returns, so a first done(error) can arrive without one in two ways: it was called synchronously inside the callback, or the callback was orphaned because its body threw (the throw returns before the attach). run_test_callback now marks the orphan in the same exhaustive block that otherwise attaches the ref, and an orphaned done(error) falls back to the generic path so it is never blamed on whatever entry is active when it finally fires. An event-loop heuristic cannot make that distinction: a throwing body never reaches its own microtask drain, so a microtask orphan is drained inside the NEXT entry's callback, still inside the same runner step.

This also removes a synchronous re-entry: the old path advanced the runner from inside the done() call, while the non-error done() path (and the comment documenting the intent) defers the advance to the next tick. Two lines change in the existing test-error-code-done-callback.test.ts snapshot as a result: both were stack frames leaked from the previous test's done() call site into the next test's error stack, because the next test's body used to run from inside the previous test's done() frame.

One known pre-existing gap is intentionally left alone: a synchronous done(error) inside a describe.concurrent group with more than one sequence still resolves to an entry_data: None phase and is reported as an unhandled error, exactly as before. Closing that needs the ref attached before the callback runs, a reordering of run_test_callback better done on its own.

Verification

• test/js/bun/test/test-error-code-done-callback.test.ts: spawns bun test on a done(err) variant of each hook type and asserts the pass/fail counts match the synchronous-throw variant; a separate test pins that an orphaned late done(err) (a body that throws after handing done away) is not blamed on an unrelated test, for both a setTimeout and a microtask scheduling.
• test/js/node/test_runner/node-test.test.ts + fixtures/06-failing-before-hook.js: a node:test suite whose before() throws reports 0 pass, 1 fail.

The hook tests fail on the unfixed build with the 2 pass, 0 fail, N error(s) counts above. jest-hooks.test.ts, done-async.test.ts, bun_test.test.ts, bun-test.test.ts (both), test-timeout-behavior.test.ts, and the concurrent suites all pass with the fix.

[robobun]

^{Updated 1:32 PM PT - Jun 29th, 2026}

❌ @robobun, your commit 21e6a3f has 1 failures in Build #66884 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline

---

🧪   To try this PR locally:

bunx bun-pr 33089

That installs a local version of the PR into your bun-33089 executable, so you can run:

bun-33089 --bun

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f6823c76-b4fb-45db-b376-fd5aa54746fe

📥 Commits

Reviewing files that changed from the base of the PR and between 32770fd and 21e6a3f.

📒 Files selected for processing (2)

• test/js/bun/test/test-error-code-done-callback.test.ts
• test/js/node/test_runner/node-test.test.ts

Walkthrough

Adds an orphaned boolean field to DoneCallback that is set when the test body throws before run_test_callback can attach ref. Updates bun_test_done_callback to enforce first-call-only error handling and route done(error) to the owning BunTest or fall back to the VM's uncaught-exception handler. New tests cover lifecycle hook done(error) attribution and orphaned done(err) via macrotask/microtask. Docs receive formatting-only fixes.

Changes

done() callback error attribution

Layer / File(s)	Summary
DoneCallback orphaned flag src/runtime/test_runner/DoneCallback.rs	Adds pub orphaned: bool field with doc comment and initializes it to false in create_unbound.
done() error routing and orphan marking src/runtime/test_runner/bun_test.rs	run_test_callback sets orphaned = true when the test body threw before ref attachment. bun_test_done_callback enforces first-call-only handling and routes done(error) to the owning BunTest or falls back to vm.uncaught_exception when orphaned or no RefData is available.
done() callback tests test/js/bun/test/test-error-code-done-callback.test.ts	Updates existing inline snapshots for changed stack formatting; adds describe.concurrent suites for lifecycle hook done(error) attribution and orphaned done(err) via macrotask/microtask; adds summaryCounts helper.
node:test failing before hook test/js/node/test_runner/fixtures/06-failing-before-hook.js, test/js/node/test_runner/node-test.test.ts	Adds a fixture with a before() hook that throws and an integration test asserting zero passes, one failure, no "Unhandled error between tests" message, and exit code 1.

Docs formatting fixes

Layer / File(s)	Summary
Docs formatting cleanup docs/guides/util/base64.mdx, docs/runtime/web-apis.mdx	Wraps btoa/atob example in a TypeScript fenced code block; corrects web-apis table column spacing and ByteLengthQueuingStrategy link markup.

Suggested reviewers

• dylan-conway

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the core change: dependent tests now fail when a hook's done() callback receives an error.
Description check	✅ Passed	The description covers the problem, cause, fix, and verification, even though it uses custom headings instead of the template labels.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
bun	🟢 Ready	View Preview	Jun 29, 2026, 5:04 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/test_runner/bun_test.rs`:
- Around line 840-856: The synchronous done(err) fallback in DoneCallback::call
loses the original callback cfg_data when ref_in is None because it uses
get_current_state_data(), which can return entry_data: None during concurrent
sequences. Update the DoneCallback path in bun_test.rs so the callback
phase/cfg_data is preserved for this case—either store the callback phase on
DoneCallback itself or attach the ref before invoking the callback—so the strong
branch uses the same cfg_data as the throw/rejection path.

In `@test/js/bun/test/test-error-code-done-callback.test.ts`:
- Around line 142-146: Shorten the comments in the test file to the invariant
the test is asserting, and remove the bug-history explanation and old-behavior
context. Keep the remaining comment tied to the relevant test helpers (the
done-callback lifecycle hook cases and the node:test before() coverage) and make
it fit within the 3-line limit while preserving only what the test protects.
- Line 196: The ordering test currently relies on a wall-clock timer via the a
setTimeout case, which can make the assertion flaky. Update the test data in
test-error-code-done-callback.test.ts to use an event-loop turn primitive
instead of setTimeout(fire, 5), so the existing ordering checks still exercise
macrotask behavior without depending on elapsed time. Keep the change localized
to the relevant test case and preserve the surrounding ordering semantics.

In `@test/js/node/test_runner/node-test.test.ts`:
- Around line 58-60: Remove the historical behavior context from the comment in
node-test.test.ts so it only states the invariant being verified. Update the
comment near the affected test to describe the current expected behavior without
mentioning Bun’s prior behavior or the “Unhandled error between tests” history;
keep that background out of the test and rely on the test name/assertions plus
the PR description instead.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b93356fd-8983-4698-ace8-872dd75b46a7

📥 Commits

Reviewing files that changed from the base of the PR and between fb24aac and 32770fd.

📒 Files selected for processing (7)

• docs/guides/util/base64.mdx
• docs/runtime/web-apis.mdx
• src/runtime/test_runner/DoneCallback.rs
• src/runtime/test_runner/bun_test.rs
• test/js/bun/test/test-error-code-done-callback.test.ts
• test/js/node/test_runner/fixtures/06-failing-before-hook.js
• test/js/node/test_runner/node-test.test.ts

[robobun]

CI status for this PR as of build 66884 (commit 21e6a3f): 281 jobs passed and the diff is green on every lane that ran it. The three red jobs are unrelated to this change:

• darwin 26 aarch64 - test-bun: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'. No test ran. The same agent pool has failed the same way, before any test started, on every one of my builds today.
• alpine 3.23 x64 - test-bun and alpine 3.23 x64-baseline - test-bun: test/js/node/test/parallel/test-net-connect-memleak.js asserting collected === true after globalThis.gc(). That file is not run under bun test, so it executes none of the code this PR changes, and the identical failure annotation appears on every recent PR build I checked across unrelated branches and subsystems (66904, 66900, 66896, 66868, 66867, 66809). It is an environment-wide alpine failure right now, not something specific to this diff.

I already re-ran CI once for the darwin infra failure and do not want to keep pushing empty commits, so this needs a maintainer to retry those lanes or merge over them. All review feedback is addressed and every thread is resolved.

[robobun]

Related: #38876 makes get_current_and_valid_execution_sequence reject data from an earlier retry/repeat attempt of the same sequence (a generation counter bumped in reset_sequence). Today done(error) bypasses that check because bun_test_done_callback reports through vm.uncaught_exception, so a late done(error) from an attempt that timed out is charged to the retry that is running (and with retry: 2 starts attempt 3 while attempt 2's body is still running, ending 1 pass, exit 0). The ref-present branch in this PR fixes that as well once it is rebased on #38876, so it would be worth carrying these cases here; an earlier revision of #38876 had the same rerouting and they passed unchanged with it. They all fail on current main.

Test cases (each is a `bun test` fixture run in a subprocess)

Retry (test/js/bun/test/test-retry-repeats-basic.test.ts has a runRetryFixture helper after #38876):

import { test } from "bun:test";
let firstDone: (err?: unknown) => void;
let attempt = 0;
test("retry", done => {
  attempt++;
  if (attempt === 1) {
    firstDone = done; // times out
    return;
  }
  firstDone(new Error("late error from attempt 1"));
  setTimeout(() => {
    console.log("attempt 2 body finished");
    done();
  }, 1);
}, { retry: 2, timeout: 500 });

Expected: stdout contains attempt 2 body finished; stderr contains Unhandled error between tests, error: late error from attempt 1, (pass) retry (attempt 2), 1 pass, 0 fail, 1 error, and not (attempt 3); exit code 1.

Same thing without retry (the late error currently fails second):

import { test } from "bun:test";
let firstDone: (err?: unknown) => void;
test("first", done => {
  firstDone = done;
}, { timeout: 100 });
test("second", done => {
  firstDone(new Error("late error from first"));
  setTimeout(() => {
    console.log("second body finished");
    done();
  }, 1);
}, { timeout: 500 });

Expected: (fail) first, Unhandled error between tests, error: late error from first, (pass) second, 1 pass, 1 fail, 1 error; exit code 1.

Concurrent (currently fails passes and the error is printed between tests):

import { test } from "bun:test";
test.concurrent("fails", done => {
  setTimeout(() => done(new Error("reported through done")), 1);
});
test.concurrent("passes", done => {
  setTimeout(() => done(), 1);
});

Expected: error: reported through done, (fail) fails, (pass) passes, 1 pass, 1 fail, no Unhandled error between tests; exit code 1.

[robobun]

Heads-up: #39112 overlaps with this PR. It also routes done(err) through on_uncaught_exception with the entry's own RefDataValue (so the hook cases here are covered there too, with a beforeEach/afterEach test), but it stamps the owner on the DoneCallback at creation instead of falling back to get_current_state_data() when r#ref is not set yet. That fallback still cannot name a sequence inside a concurrent group, so a synchronous or microtask done(err) in an it.concurrent test, and every node:test failure under bun test --concurrent, are still reported as passing with this branch; #39112 adds tests for those. If this one lands first, #39112 rebases to the remaining delta.

[robobun]

Closing in favor of #39112, which carries the same change (reporting done(err) through BunTest::on_uncaught_exception with the owning entry's RefDataValue instead of the generic uncaught_exception path) and also covers the cases this PR left open: done(err) called before the ref is attached inside a concurrent group (synchronous or from the microtask drain), and node:test failures under bun test --concurrent, since it stamps the owner on the DoneCallback at creation rather than falling back to get_current_state_data().

Checked against a build of #39112: the four hook cases from this PR (beforeAll 0 pass / 1 fail, beforeEach and afterEach 0 pass / 2 fail, afterAll 2 pass / 1 fail, matching the throwing variants), the two orphaned late done(err) cases, and the node:test before() fixture all pass there; main still reports them as passing with unhandled errors. #39112's own tests already pin the late done(err) cases, so the coverage that was only here has been added to that branch: beforeAll / afterAll done(err) in its fixture (run serially, under --concurrent, and with describe.concurrent), and a node:test fixture whose suite-level before() / after() throw.