bun test: name snapshots taken in beforeEach/afterEach after the running test

[robobun]

Problem

• toMatchSnapshot() / toThrowErrorMatchingSnapshot() called from beforeEach, afterEach, an afterEach registered inside a test, or onTestFinished is written as exports[`d (unnamed): hint 1`] instead of exports[`d t1: hint 1`] (repro and the full before/after key lists are in the details block below).
• Every hook snapshot in a describe shares that one key, so its counter runs across tests: adding, removing or reordering a test renumbers every later hook snapshot in the describe and they all stop matching.
• A hook declared in an outer describe only gets the outer describe as prefix (outer (unnamed): ...) even when the test lives in an inner describe, and hooks registered from inside a test body get no prefix at all ((unnamed): hint 1).
• Cause: Expect::get_snapshot_name (src/runtime/test_runner/expect.rs:626) builds the key from the execution entry that was active when expect() was called. While a hook runs that is the hook's own entry, which has no name, and whose parent is the scope the hook was declared in (none for hooks added during a test). Before the test runner rewrite (Rewrite test/describe, add test.concurrent #22534) the key came from the pending test; Jest keys these snapshots by currentTestName, which is set for the whole test including its hooks.

Fix

• New RefDataValue::snapshot_entry() (bun_test.rs, next to entry(), both built on a shared running_sequence() that keeps the existing validation) returns the sequence's test_entry, falling back to the active entry when the sequence has no test. get_snapshot_name calls it instead of entry() and is otherwise unchanged, so the concurrent-group error and the describe-chain walk are as before.
• Correct because an ExecutionSequence is exactly one test plus the beforeEach/afterEach/onTestFinished entries that run for it (Order.rs generate_order_test, and generic_hook_impl in bun_test.rs for hooks added while the test runs), so test_entry is the test a hook is running for, and its name and describe chain are what the test body's own snapshots are already keyed by. The result matches Jest (and vitest, apart from its > separator) for every case in the new test, including an inline snapshot in beforeEach taking number 1 of the test's unhinted counter.
• beforeAll / afterAll run in sequences without a test_entry (generate_all_order) and stay under their current d (unnamed): hint key, now counted among themselves only. Jest has no test name there either (currentTestName is undefined, or still the previous test's), that key is stable once the per-test hooks stop sharing it, and throwing (what bun did before the rewrite) would break files that work today.
• Behavior change: files that take snapshots inside beforeEach/afterEach/onTestFinished need one bun test -u after upgrading. Every such snapshot (file or inline, with or without hint) now counts against the test it runs for, which changes three kinds of existing .snap keys:
  • the hook entries themselves move from <describe> (unnamed)... to the test's keys;
  • the test body's own entries move up one number for each hook snapshot with the same hint (or none), inline ones included, and those leave no (unnamed) entry to grep for;
  • beforeAll/afterAll entries keep their (unnamed) key but are renumbered when a per-test hook in the same describe used the same hint.
• First run after upgrading: a moved key that lands on a stale entry fails as a value mismatch; otherwise the new keys fail in CI as "Snapshot creation is disabled" and are appended outside CI. Stale entries are never reported (bun has no obsolete-snapshot tracking); -u rewrites the file with only the current keys. Each shape was checked with a .snap written by bun 1.4.0 and then run with this branch (details block).
• Tests: test/js/bun/test/snapshot-tests/bun-snapshots.test.ts, new snapshots taken in hooks block, three tests that all fail on the released bun with the (unnamed) keys and pass with this change:
  • one fixture with a top-level, an outer and an inner beforeEach, beforeAll, afterEach, an afterEach registered inside the test, onTestFinished, afterAll and an inline snapshot in beforeEach, asserting the full .snap contents;
  • one where every hook and the body use the same hint, pinning the counter layout described above (d (unnamed): setup 1/2 for beforeAll/afterAll, d t1: setup 1/2/3 for beforeEach/body/afterEach);
  • one that adds a test in front of two tests with existing afterEach snapshots and checks the run still passes with the existing entries untouched.
• Also run against the debug build with no failures: test/js/bun/test/bun_test.test.ts, test/js/bun/test/ci-restrictions.test.ts, test/cli/test/rerun-each.test.ts (retry/repeat counter reset), test/js/bun/test/test-test.test.ts, test/js/bun/test/expect.test.js, and the rest of test/js/bun/test/snapshot-tests/ (its error snapshots case needs FORCE_COLOR=1, as on main; CI sets it).

Background

• Snapshot key: the enclosing describe names and the test name joined by spaces, : hint if a hint was given, then a counter. The counter is per key and is incremented by every file or inline snapshot taken under that key during the run (and reset when a test is retried or repeated), so a snapshot charged to the wrong key also shifts the numbering of the others.
• ExecutionEntry: one callback the runner executes, a test or a hook. Hooks have no name. base.parent is the describe scope the entry was declared in; hooks registered while a test is running have none.
• ExecutionSequence: what the runner executes as one unit. For a test it is the test's beforeEach entries, the test, and its afterEach/onTestFinished entries, with test_entry pointing at the test; each beforeAll/afterAll hook is a sequence of its own with no test_entry. active_entry is the entry currently running.
• expect() records the runner state it was called in (RefDataValue). RefDataValue::entry() returns the recorded entry if it is still the one running, and nothing in a concurrent group, where the runner cannot tell which sequence an expect() belongs to (hence the existing "not supported in concurrent tests" error, which this keeps). jest.rs already compares the active entry with the sequence's test_entry the same way to tell a hook failure from a test failure.

Repro and keys before/after

import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, onTestFinished, test } from "bun:test";
beforeEach(() => expect("top").toMatchSnapshot("top beforeEach"));
describe("outer", () => {
  beforeEach(() => expect("outer").toMatchSnapshot("outer beforeEach"));
  describe("inner", () => {
    beforeAll(() => expect("ba").toMatchSnapshot("beforeAll"));
    beforeEach(() => expect("inner").toMatchSnapshot("inner beforeEach"));
    beforeEach(() => expect("inline").toMatchInlineSnapshot('"inline"'));
    afterEach(() => expect("ae").toMatchSnapshot("afterEach"));
    afterAll(() => expect("aa").toMatchSnapshot("afterAll"));
    test("t1", () => {
      afterEach(() => expect("ae in test").toMatchSnapshot("afterEach in test"));
      onTestFinished(() => expect("otf").toMatchSnapshot("onTestFinished"));
      expect("body").toMatchSnapshot("body");
      expect("body").toMatchSnapshot();
    });
    test("t2", () => expect("body").toMatchSnapshot("body"));
  });
});

CI=false bun test with bun 1.4.0-canary.1 writes:

outer inner (unnamed): beforeAll 1
(unnamed): top beforeEach 1
outer (unnamed): outer beforeEach 1
outer inner (unnamed): inner beforeEach 1
outer inner t1: body 1
outer inner t1 1
(unnamed): afterEach in test 1
outer inner (unnamed): afterEach 1
(unnamed): onTestFinished 1
(unnamed): top beforeEach 2
outer (unnamed): outer beforeEach 2
outer inner (unnamed): inner beforeEach 2
outer inner t2: body 1
outer inner (unnamed): afterEach 2
outer inner (unnamed): afterAll 1

With this change:

outer inner (unnamed): beforeAll 1
outer inner t1: top beforeEach 1
outer inner t1: outer beforeEach 1
outer inner t1: inner beforeEach 1
outer inner t1: body 1
outer inner t1 2
outer inner t1: afterEach in test 1
outer inner t1: afterEach 1
outer inner t1: onTestFinished 1
outer inner t2: top beforeEach 1
outer inner t2: outer beforeEach 1
outer inner t2: inner beforeEach 1
outer inner t2: body 1
outer inner t2: afterEach 1
outer inner (unnamed): afterAll 1

vitest 4.1.9 on the same file (with vitest imports) writes the same set of keys with > as the separator, including outer > inner > t1 2 for the unhinted body snapshot.

Upgrade path: .snap written by bun 1.4.0, then run with this branch

describe("d", () => {
  beforeAll(() => expect("ba").toMatchSnapshot("setup"));
  beforeEach(() => expect("be").toMatchSnapshot("setup"));
  afterEach(() => expect("ae").toMatchSnapshot("setup"));
  afterAll(() => expect("aa").toMatchSnapshot("setup"));
  test("t1", () => expect("body").toMatchSnapshot("setup"));
});

bun 1.4.0 writes:

d (unnamed): setup 1 = "ba"
d (unnamed): setup 2 = "be"
d t1: setup 1 = "body"
d (unnamed): setup 3 = "ae"
d (unnamed): setup 4 = "aa"

This branch against that file, without -u: the beforeEach fails (d t1: setup 1 now receives "be", stored "body"), so the body is skipped; the afterEach's d t1: setup 2 is appended (outside CI); the afterAll fails (d (unnamed): setup 2 now receives "aa", stored "be"). Summary: snapshots: 1 passed, 1 added, 2 failed, and the five old entries stay in the file.

After bun test -u on this branch:

d (unnamed): setup 1 = "ba"
d t1: setup 1 = "be"
d t1: setup 2 = "body"
d t1: setup 3 = "ae"
d (unnamed): setup 2 = "aa"

A beforeEach with only toMatchInlineSnapshot() behaves like the beforeEach above for numbering purposes: the body's unhinted toMatchSnapshot() entries move from d t1 1 to d t1 2, with no (unnamed) entry in the old file.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 32 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: 266e9abc-ed4d-49cb-af45-77462ba1f9ff

📥 Commits

Reviewing files that changed from the base of the PR and between a5c86ae and bc3d385.

📒 Files selected for processing (3)

• src/runtime/test_runner/bun_test.rs
• src/runtime/test_runner/expect.rs
• test/js/bun/test/snapshot-tests/bun-snapshots.test.ts

---

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

[robobun]

Status: ready for review at bc3d385.

CI (build 97523): 177 of 179 jobs passed, including the Linux, ASAN, Alpine and Windows runs of the new tests. The only two non-green jobs are the macOS 14 aarch64 test lanes, which never got an agent (one expired, one still queued); the previous build, 97156, was starved of the same lanes. The annotations on both builds are retried flaky tests unrelated to the test runner. Nothing in the diff is platform-specific. The expired job needs a retry from someone with write access to Buildkite; I am not going to push retrigger commits against a starved queue.

Reproduced with bun 1.4.0-canary.1: CI=false bun test on a file whose beforeEach/afterEach call toMatchSnapshot("hint") writes exports[`d (unnamed): hint 1`], exports[`d (unnamed): hint 2`], ... (one shared counter across tests) instead of exports[`d t1: hint 1`], exports[`d t2: hint 1`]. The snapshots taken in hooks tests in test/js/bun/test/snapshot-tests/bun-snapshots.test.ts fail the same way on the released bun and pass with the debug build of this branch.

[claude]

Beyond the inline nit, I also checked the raw-pointer round-trip in get_snapshot_name (capture NonNull from entry(), then re-borrow buntest for sequence()) — it mirrors the existing pattern at jest.rs:609-614, and the unwrap_or(active_entry) fallback preserves the old key when sequence() yields no test_entry (beforeAll/afterAll), so the concurrent-group error path and the describe-chain walk are unchanged.

Extended reasoning...

Verified the new unsafe deref follows the same split-borrow idiom already used for entry()/sequence() in jest.rs's uncaught-exception handler, and that test_entry: Option<NonNull<ExecutionEntry>> is Copy so no borrow outlives the sequence() call. The fallback to active_entry means every case where sequence() or test_entry is None degrades to exactly the pre-PR behavior. Not approving because this is a user-facing snapshot-key change with an unsafe block in the test runner — worth a maintainer glance.

[claude]

I re-reviewed after 79ef2eb and 5f1f700 and found no bugs — the earlier stdout-pipe nit is fixed and the running_sequence() extraction preserves entry()'s validation exactly. Because this re-keys existing hook .snap entries (a user-visible behavior change, even if it restores pre-#22534/Jest semantics) and adds new unsafe derefs in the test-runner core, a human sign-off would still be worthwhile.

Checked: running_sequence() is a pure extraction of the old entry() body (same phase check + get_current_and_valid_execution_sequence); snapshot_entry()'s test_entry.or(active_entry) fallback is total because the validator only returns Some when active_entry matches a non-null recorded entry; get_snapshot_name only reads base.name/base.parent, so the &ExecutionEntry return type suffices; the sole other .entry() caller in jest.rs is untouched.

Extended reasoning...

Overview

Three files: bun_test.rs extracts a private RefDataValue::running_sequence() from the existing entry() body and adds a sibling snapshot_entry() that returns sequence.test_entry.or(sequence.active_entry); expect.rs swaps .entry() for .snapshot_entry() at the one call site in get_snapshot_name; the test file adds a two-test describe covering top-level/outer/inner beforeEach, beforeAll/afterAll, in-test afterEach, onTestFinished, and an inline snapshot in beforeEach, plus a stability test (adding a test doesn't renumber existing hook snapshots).

Security risks

None. No untrusted input, no auth/crypto/permissions surface. The new unsafe blocks deref NonNull<ExecutionEntry> fields under the same invariant the pre-existing entry() and every test_entry deref in Execution.rs already relies on (intrusive nodes valid while the sequence is live, lifetime tied to &'a mut BunTest).

Level of scrutiny

Moderate-to-high. The Rust delta is small and pattern-matching, but (a) it lives in the test runner's execution core with unsafe pointer derefs, and (b) it changes the observable snapshot key for any user who currently calls toMatchSnapshot from a per-test hook — their next run outside CI writes new keys and leaves the old ones obsolete, and their next CI run reports new snapshots. The PR description argues this well (regression from #22534, matches Jest's currentTestName and vitest, keys are unstable today), but that's a product call a maintainer should ratify.

Other factors

Prior feedback addressed: my stdout: "pipe" nit (79ef2eb → "ignore") and comment-cop's paragraph-comment flag (5f1f700 moved the lookup into snapshot_entry() so get_snapshot_name is a one-token change). I confirmed get_current_and_valid_execution_sequence requires entry_data: Some and matches active_entry against a non-null recorded pointer, so running_sequence() returning Some implies active_entry.is_some() — the .or(active_entry) fallback for beforeAll/afterAll never yields None after ?. The refactored entry() derives &mut ExecutionEntry from a NonNull field of an immutably-borrowed &ExecutionSequence; the entry is a separate heap box (in DescribeScope::entries/hook vecs/cloned_hook_entries), so no aliasing. Tests follow harness conventions (tempDir, bunEnv, test.concurrent, exact-array assertions, stderr before exitCode). No CODEOWNERS on the touched paths.

[robobun]

^{Updated 4:56 AM PT - Aug 15th, 2026}

❌ @robobun, your commit bc3d385 has some failures in Build #97523 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38820

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

bun-38820 --bun

[robobun]

^{Updated 9:05 PM PT - Aug 14th, 2026}

@robobun, your commit 5f1f700 is building: #97156

[claude]

I reviewed this PR and didn't find any bugs (the earlier stdout: "pipe" nit was addressed in 79ef2eb). Because it changes user-facing snapshot key generation — existing .snap files with hook snapshots need a one-time bun test -u after upgrading — a human sign-off on that compat trade-off would be worthwhile.

What was reviewed:

• running_sequence() refactor preserves entry() behavior exactly; snapshot_entry() returns None on the same paths, so the concurrent-group error in get_snapshot_name is unchanged.
• test_entry deref safety matches the existing active_entry pattern; both point into sequences validated by get_current_and_valid_execution_sequence.
• The describe-chain walk in get_snapshot_name now uses the test's base.parent, so outer-declared hooks get the inner describe prefix — checked against the fixture's expected keys.

Extended reasoning...

Overview

The PR changes which ExecutionEntry is used to build snapshot keys when toMatchSnapshot() is called from a per-test hook. In bun_test.rs, RefDataValue::entry() is refactored onto a new private running_sequence() helper (behavior-preserving), and a new snapshot_entry() sibling returns the sequence's test_entry (falling back to active_entry for beforeAll/afterAll). expect.rs swaps one accessor call in get_snapshot_name. Three subprocess tests are added to bun-snapshots.test.ts covering the full key layout, the shared-counter behavior, and the stability property (adding a test does not renumber existing hook snapshots).

Security risks

None. This is test-runner-internal snapshot naming; no untrusted input, no I/O beyond the existing .snap file writes.

Level of scrutiny

Medium-high. The Rust diff is small (~20 lines) and mechanically parallels existing code — the new test_entry deref uses the same intrusive-node safety justification as the adjacent active_entry deref, and running_sequence() is a pure extraction with the same guards (RefDataValue::Execution match, Phase::Execution check, get_current_and_valid_execution_sequence). The tests are hermetic (tempDir, bunEnv, CI: "false"), use test.concurrent, drain stderr concurrently with proc.exited, and assert exact .snap contents plus pass/fail counts before exit code.

However, this is a user-visible behavior change: any project taking snapshots inside beforeEach/afterEach/onTestFinished will see those snapshots re-keyed and need bun test -u. The PR body documents this thoroughly and shows the change aligns with Jest and vitest, but whether that trade-off is acceptable (and whether it needs a changelog note) is a maintainer call.

Other factors

• The one nit from my earlier review (stdout: "pipe" unread) was fixed in 79ef2eb; the comment-cop flag on expect.rs was addressed by moving the lookup into snapshot_entry() in 5f1f700. All inline threads are resolved.
• CI on 5f1f700 was 177/179 green with unrelated flakes per the status comment; bc3d385 only adds a third test.
• I checked that snapshot_entry() returning &ExecutionEntry (vs entry()'s &mut) is fine — get_snapshot_name only reads base.name and walks base.parent.
• The .or(active_entry) fallback keeps beforeAll/afterAll on their existing (unnamed) key rather than throwing, which the PR body justifies (Jest has no currentTestName there either, and throwing would break files that work today).