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

src/runtime/test_runner/bun_test.rs

@@ -815,31 +815,53 @@
         let [value] = callframe.arguments_as_array::<1>();
 
         let was_error = !value.is_empty_or_undefined_or_null();
+        // A second done() is a no-op, matching Bun 1.2.20.
+        // In Jest it is "Expected done to be called once, but it was called multiple times."
+        // Vitest does not support done callbacks.
         // SAFETY: `this` is the live `*mut DoneCallback` returned by `from_js`;
         // single-threaded JS VM, GC keeps the wrapper alive for the call frame.
-        if unsafe { (*this).called } {
-            // in Bun 1.2.20, this is a no-op
-            // in Jest, this is "Expected done to be called once, but it was called multiple times."
-            // Vitest does not support done callbacks
-        } else {
-            // error is only reported for the first done() call
-            if was_error {
-                let _ = global_this.bun_vm().as_mut().uncaught_exception(global_this, value, false);
-            }
-        }
+        let first_call = !unsafe { (*this).called };
         // SAFETY: see above — `this` is a live `*mut DoneCallback`.
         let ref_in = unsafe {
             (*this).called = true;
             (*this).r#ref.take()
         };
-        let Some(ref_in) = ref_in else {
-            return Ok(JSValue::UNDEFINED);
-        };
         // `this.ref` was already taken above.
         // RefPtr<T> currently has NO Drop impl, so decrement the
         // intrusive count explicitly at scope exit. Without this the
         // paired promise then/catch path never sees has_one_ref()==true and the RefData leaks.
-        let ref_in = scopeguard::guard(ref_in, |r: RefDataPtr| r.deref());
+        let ref_in = ref_in.map(|r| scopeguard::guard(r, |r: RefDataPtr| r.deref()));
+
+        // error is only reported for the first done() call
+        if first_call && was_error {
+            // `done(error)` is a failure of the entry that owns this callback,
+            // not a stray unhandled exception. Attribute it there (like the
+            // promise-catch path) so a failing hook fails its dependent tests.
+            let strong = match ref_in.as_ref() {
+                Some(r) => r.buntest_weak.upgrade(),
+                // done() ran synchronously inside the callback (`run_test_callback`
+                // attaches the ref after it returns), so the owner is the active entry.
+                None => clone_active_strong(),
+            };
+            match strong {
+                Some(strong) => {
+                    let phase = match ref_in.as_ref() {
+                        Some(r) => r.phase.clone(),
+                        None => strong.get().get_current_state_data(),
+                    };
+                    // `strong.get()` is re-derived; the `get_current_state_data` borrow ended above.
+                    strong.get().on_uncaught_exception(global_this, Some(value), false, &phase);
+                }
+                None => {
+                    // No live BunTest owns this callback; report it generically.
+                    let _ = global_this.bun_vm().as_mut().uncaught_exception(global_this, value, false);
+                }
+            }
+        }
+
+        let Some(ref_in) = ref_in else {
+            return Ok(JSValue::UNDEFINED);
+        };
 
         // dupe the ref and enqueue a task to call the done callback.
         // this makes it so if you do something else after calling done(), the next test doesn't start running until the next tick.

test/js/bun/test/test-error-code-done-callback.test.ts

@@ -1,5 +1,5 @@
-import { expect, test } from "bun:test";
-import { bunEnv, bunExe } from "harness";
+import { describe, expect, test } from "bun:test";
+import { bunEnv, bunExe, tempDir } from "harness";
 import path from "path";
 
 test("verify we print error messages passed to done callbacks", () => {
@@ -80,7 +80,6 @@ test("verify we print error messages passed to done callbacks", () => {
     ^
     error: you should see this(async)
     at <anonymous> (<dir>/test-error-done-callback-fixture.ts:42:14)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:37:3)
     (fail) error done callback (async)
     43 |   });
     44 | });
@@ -111,7 +110,6 @@ test("verify we print error messages passed to done callbacks", () => {
     ^
     error: you should see this(async, nextTick)
     at <anonymous> (<dir>/test-error-done-callback-fixture.ts:60:14)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:54:5)
     (fail) error done callback (async, nextTick)
     62 | });
     63 |
@@ -140,3 +138,57 @@ test("verify we print error messages passed to done callbacks", () => {
     "
   `);
 });
+
+// A `done(error)` in a lifecycle hook must fail the hook's dependent tests,
+// exactly like a synchronous throw in the same hook does. It used to be
+// surfaced as an "Unhandled error between tests" while every dependent test
+// was still counted as a pass. `node:test` routes every hook through the
+// done-callback form, so that module's `before()` was affected too.
+describe.concurrent("done(error) in a lifecycle hook", () => {
+  // One describe block containing 2 tests; expected counts match the
+  // synchronous-throw variant of each hook.
+  const expected = {
+    beforeAll: { pass: 0, fail: 1 },
+    beforeEach: { pass: 0, fail: 2 },
+    afterEach: { pass: 0, fail: 2 },
+    afterAll: { pass: 2, fail: 1 },
+  } as const;
+
+  test.each(Object.keys(expected) as (keyof typeof expected)[])(
+    "%s(done => done(err)) matches the synchronous-throw counts",
+    async hook => {
+      using dir = tempDir(`done-error-${hook}`, {
+        "hook.test.ts": `
+          import { describe, ${hook}, test } from "bun:test";
+          describe("suite", () => {
+            ${hook}(done => { done(new Error("hook failed")); });
+            test("t1", () => {});
+            test("t2", () => {});
+          });
+        `,
+      });
+      await using proc = Bun.spawn({
+        cmd: [bunExe(), "test", "./hook.test.ts"],
+        env: bunEnv,
+        cwd: String(dir),
+        stdout: "pipe",
+        stderr: "pipe",
+      });
+      const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+      const out = stdout + stderr;
+      // The hook's error is still reported, attributed to the failing entry.
+      expect(out).toContain("error: hook failed");
+      expect(summaryCounts(out)).toEqual(expected[hook]);
+      expect(exitCode).toBe(1);
+    },
+  );
+});
+
+/** `" 2 pass\n 0 fail\n 1 error\n"` -> `{ pass: 2, fail: 0, error: 1 }` */
+function summaryCounts(out: string): Record<string, number> {
+  const counts: Record<string, number> = {};
+  for (const [, n, label] of out.matchAll(/^ (\d+) (pass|fail|skip|todo|error)s?$/gm)) {
+    counts[label] = Number(n);
+  }
+  return counts;
+}

test/js/node/test_runner/fixtures/06-failing-before-hook.js

@@ -0,0 +1,10 @@
+const { describe, before, test } = require("node:test");
+
+describe("db suite", () => {
+  before(() => {
+    throw new Error("DB connection failed");
+  });
+  test("reads row", () => {
+    // would touch the DB; must not be reported as a pass
+  });
+});

test/js/node/test_runner/node-test.test.ts

@@ -52,6 +52,17 @@ describe("node:test", () => {
       stderr: expect.stringContaining("0 fail"),
     });
   });
+
+  test("should not report a test as passing when its before() hook threw", async () => {
+    const { exitCode, stderr } = await runTests(["06-failing-before-hook.js"]);
+    // node fails every test under a suite whose before() hook failed. Bun
+    // used to report the test as a pass and surface the hook error only as
+    // an "Unhandled error between tests".
+    expect(stderr).toContain("error: DB connection failed");
+    expect(stderr).toContain(" 0 pass\n 1 fail\n");
+    expect(stderr).not.toContain("Unhandled error between tests");
+    expect(exitCode).toBe(1);
+  });
 });
 
 async function runTests(filenames: string[]) {