bun test: ignore completions from an earlier attempt of a retried test

src/runtime/test_runner/Execution.rs

@@ -93,7 +93,7 @@ pub struct Execution {
     /// around `run_test_callback` so code re-entered from a test body (e.g.
     /// spawnSync's wait loop) can read the calling entry's own deadline.
     pub(crate) on_stack_entry: core::cell::Cell<Option<NonNull<ExecutionEntry>>>,
-    /// The (group_index, sequence_index, entry, repeat) for `on_stack_entry`,
+    /// The (sequence_index, entry, generation) for `on_stack_entry`,
     /// set/restored alongside it. `get_current_state_data()` can't name a
     /// sequence inside a concurrent group; this can, for code re-entered from
     /// the microtask drain inside `run_test_callback` (node:test's runtime
@@ -159,6 +159,9 @@ pub struct ExecutionSequence {
     pub(crate) test_entry: Option<NonNull<ExecutionEntry>>,
     pub(crate) remaining_repeat_count: u32,
     pub(crate) remaining_retry_count: u32,
+    /// Bumped by every [`Execution::reset_sequence`] (retry or repeat). [`EntryData`] carries the
+    /// generation a callback started under, so a completion from an earlier attempt is stale.
+    pub(crate) generation: u32,
     pub(crate) result: Result,
     pub(crate) executing: bool,
     pub(crate) started_at: Timespec,
@@ -183,6 +186,7 @@ impl ExecutionSequence {
             remaining_repeat_count: repeat_count,
             remaining_retry_count: retry_count,
             // defaults:
+            generation: 0,
             result: Result::Pending,
             executing: false,
             started_at: Timespec::EPOCH,
@@ -475,9 +479,9 @@ impl Execution {
             return None;
         }
         let sequence = &mut self.sequences[seq_abs];
-        if i64::from(sequence.remaining_repeat_count) != entry_data.remaining_repeat_count {
+        if sequence.generation != entry_data.generation {
             group_log::log(format_args!(
-                "runOneCompleted: the data is for a previous repeat count (outdated)",
+                "runOneCompleted: the data is for a previous retry/repeat of the sequence (outdated)",
             ));
             return None;
         }
@@ -740,12 +744,14 @@ impl Execution {
         }
 
         // Preserve retry/repeat counts across reset
+        let generation = sequence.generation;
         *sequence = ExecutionSequence::init(
             sequence.first_entry,
             sequence.test_entry,
             sequence.remaining_retry_count,
             sequence.remaining_repeat_count,
         );
+        sequence.generation = generation.wrapping_add(1);
 
         // Snapshot counters are keyed by full test name and incremented on every
         // toMatchSnapshot() call. Without this reset, retries / repeats would
@@ -1004,7 +1010,7 @@ fn step_sequence_one(
         let entry_data = EntryData {
             sequence_index,
             entry: next_item_ptr.as_ptr() as *const (),
-            remaining_repeat_count: sequence.remaining_repeat_count as i64,
+            generation: sequence.generation,
         };
         let callback_data = RefDataValue::Execution {
             group_index: this.group_index,

src/runtime/test_runner/bun_test.rs

@@ -721,7 +721,7 @@ impl BunTest {
                     entry_data: Some(EntryData {
                         sequence_index: active_sequence_index,
                         entry: active_entry.as_ptr().cast::<()>(),
-                        remaining_repeat_count: sequence.remaining_repeat_count as i64,
+                        generation: sequence.generation,
                     }),
                 }
             }
@@ -819,31 +819,41 @@ impl BunTest {
         let [value] = callframe.arguments_as_array::<1>();
 
         let was_error = !value.is_empty_or_undefined_or_null();
+        // A second done() is a no-op (as in Bun 1.2.20; Jest reports it as an error).
         // 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);
-            }
-        }
-        // SAFETY: see above — `this` is a live `*mut DoneCallback`.
-        let ref_in = unsafe {
+        let (first_call, ref_in) = unsafe {
+            let first_call = !(*this).called;
             (*this).called = true;
-            (*this).r#ref.take()
+            (first_call, (*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 {
+            // Report against the entry the ref names, like `bun_test_then_or_catch`, so a late
+            // done(error) from a timed-out entry is not charged to whatever runs now. No ref means
+            // done() is still inside its own callback (the ref is attached after it returns).
+            let owner = ref_in
+                .as_ref()
+                .and_then(|r| Some((r.buntest_weak.upgrade()?, &r.phase)));
+            match owner {
+                Some((strong, phase)) => {
+                    // SAFETY: `&mut` derived via `UnsafeCell`; the borrow ends with this call.
+                    strong.get().on_uncaught_exception(global_this, Some(value), false, phase);
+                }
+                None => {
+                    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.
@@ -1426,7 +1436,8 @@ bun_jsc::jsc_host_abi! {
 pub struct EntryData {
     pub(crate) sequence_index: usize,
     pub(crate) entry: *const (),
-    pub(crate) remaining_repeat_count: i64,
+    /// `ExecutionSequence::generation` at the time the entry was started.
+    pub(crate) generation: u32,
 }
 
 // Clone: bitwise OK — `active_scope` is a non-owning borrow of a
@@ -1493,8 +1504,8 @@ impl fmt::Display for RefDataValue {
                 if let Some(ed) = entry_data {
                     write!(
                         f,
-                        "execution: group_index={},sequence_index={},entry_index={:x},remaining_repeat_count={}",
-                        group_index, ed.sequence_index, ed.entry as usize, ed.remaining_repeat_count
+                        "execution: group_index={},sequence_index={},entry_index={:x},generation={}",
+                        group_index, ed.sequence_index, ed.entry as usize, ed.generation
                     )
                 } else {
                     write!(f, "execution: group_index={}", group_index)

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 { 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,70 @@ test("verify we print error messages passed to done callbacks", () => {
     "
   `);
 });
+
+async function runDoneFixture(name: string, source: string) {
+  using dir = tempDir(name, { "done.test.ts": source });
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "test", "done.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]);
+  return { stdout, stderr, exitCode };
+}
+
+test.concurrent("done(error) from a test that already timed out is not charged to the test running now", async () => {
+  const { stdout, stderr, exitCode } = await runDoneFixture(
+    "done-error-after-timeout",
+    `
+      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 });
+    `,
+  );
+
+  // Same report as a timed-out test's promise rejecting during the next test.
+  expect(stdout).toContain("second body finished");
+  expect(stderr).toContain("(fail) first");
+  expect(stderr).toContain("Unhandled error between tests");
+  expect(stderr).toContain("error: late error from first");
+  expect(stderr).toContain("(pass) second");
+  expect(stderr).toContain("1 pass");
+  expect(stderr).toContain("1 fail");
+  expect(stderr).toContain("1 error");
+  expect(exitCode).toBe(1);
+});
+
+test.concurrent("done(error) called later from a concurrent test fails that test", async () => {
+  const { stderr, exitCode } = await runDoneFixture(
+    "done-error-concurrent",
+    `
+      import { test } from "bun:test";
+      test.concurrent("fails", done => {
+        setTimeout(() => done(new Error("reported through done")), 1);
+      });
+      test.concurrent("passes", done => {
+        setTimeout(() => done(), 1);
+      });
+    `,
+  );
+
+  expect(stderr).toContain("error: reported through done");
+  expect(stderr).toContain("(fail) fails");
+  expect(stderr).toContain("(pass) passes");
+  expect(stderr).not.toContain("Unhandled error between tests");
+  expect(stderr).toContain("1 pass");
+  expect(stderr).toContain("1 fail");
+  expect(exitCode).toBe(1);
+});