bun test: number snapshots per test file regardless of which files ran before

[robobun]

Problem

• The key a toMatchSnapshot() writes or looks up depends on which test files ran earlier in the same bun test process. With a.test.ts taking one file snapshot and b.test.ts taking an inline snapshot followed by a file snapshot in test b:
  • bun test ./b.test.ts writes exports[`b 2`] (the inline snapshot is number 1, as in Jest)
  • bun test ./a.test.ts ./b.test.ts writes exports[`b 1`]
  • so a .snap committed from one shape fails under the other; in CI the second shape fails with Snapshot creation is disabled in CI environments ... Snapshot name: "b 1".
• The opposite direction too: a file that only uses inline snapshots leaves its counts behind, and the next file with a test of the same name starts numbering at 2 (same name 2 instead of same name 1).
• --parallel has the same problem for every file after the first one a worker runs (6 identical files on 2 workers end up as 2 x t 2 and 4 x t 1).
• Cause: the counters (Snapshots.counts, src/runtime/test_runner/snapshot.rs) lived on the process-wide Snapshots and were only cleared inside write_snapshot_file, which runs when a file snapshot is taken for a different test file than the .snap currently open. Expect::inline_snapshot (src/runtime/test_runner/expect.rs) bumps the counters without opening a .snap, so the bump either landed before the switch and was wiped, or (for a file that never opens a .snap) was never cleared at all. The --rerun-each fix for Snapshots are broken with --rerun-each #23705 had already needed a second hand-placed reset of the same map.

Fix

• The counters move to BunTest::snapshot_counts (src/runtime/test_runner/bun_test.rs). enter_file builds a fresh BunTest for every file run, including each --rerun-each pass, so the per-file lifetime comes from the object itself: Snapshots::reset_counts, the --rerun-each reset in TestCommand::run, and the clear in write_snapshot_file are all deleted.
• Snapshots::add_count bumps the counter on the BunTest the expect() belongs to, which is the same BunTest whose file_id already selects the .snap file, so the numbering and the file it applies to can no longer disagree.
• The per-test retry/repeat reset (Execution::reset_sequence, same behaviour as before, matching Jest's SnapshotState.clear() on retry) now takes the BunTest and clears that file's map instead of reaching the process-wide one through Jest::runner().
• Why this is right: snapshot numbering is a per-test-file concept (Jest creates a fresh SnapshotState, counters included, per test file), and BunTest is bun's per-test-file object; the .snap file is opened lazily by the first file snapshot, so its lifetime was never a usable proxy for the test file's. Single-file runs already numbered the way Jest does; this makes multi-file and --parallel runs agree with them.
• Verified with three new tests in test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts (snapshot numbering across test files): inline snapshot before the file's first file snapshot, .snap files written by single-file runs passing under CI=true when the files run together, and an inline-only file followed by a file with the same test name; plus one in test/cli/test/parallel.test.ts where two workers run six files each committed from a single-file run. All four fail on the current release (b 1, Snapshot name: "b 1", same name 2, and 4 of 6 files failing with "t 1") and pass with this change.
• Also ran test/js/bun/test/snapshot-tests/ (only the pre-existing colour-dependent error snapshots failure, which test: make the error snapshots test pass with colors disabled #38833 addresses), test/cli/test/rerun-each.test.ts (the Snapshots are broken with --rerun-each #23705 --rerun-each and retry/repeat numbering guards, which now exercise the fresh-BunTest-per-pass path and the new reset_sequence), test/cli/test/retry-flag.test.ts, test/js/bun/test/test-retry-repeats-basic.test.ts, test/js/bun/test/ci-restrictions.test.ts, test/cli/test/test-filter-lifecycle-snapshot.test.ts, test/js/bun/test/bun_test.test.ts, test/js/bun/test/test-test.test.ts, and test/cli/test/parallel.test.ts.
• Out of scope, found while looking at this: --bail exits without flushing the open .snap (with -u it leaves the existing file truncated to 0 bytes). That is the lazy flush of the .snap contents, independent of the counters, and is being tracked separately alongside bun test: flush .snap file before afterAll runs #36679 and bun test: detect obsolete snapshots and fix -u "added" mislabel #34042.

Background

• Snapshot key: the name under which a value is stored in a .snap file, exports[`<describe names> <test name> <N>`]. N counts the snapshot matchers the test has run so far, and inline snapshots consume a number too (Jest behaviour, which bun already matched within a single file), which is why the example above is b 2.
• Snapshots (snapshot.rs) is one object for the whole process, owned by the TestRunner. It holds the summary counters (total/added/passed/failed) and the currently open .snap file (_current_file, its parsed entries in values, the bytes to write back in file_buf). The .snap is opened lazily by the first file snapshot in a test file and is flushed and swapped when a file snapshot from a different test file arrives, or at the end of the run.
• BunTest (bun_test.rs) is the per-test-file object: BunTestRoot::enter_file creates one when a file starts running and exit_file drops it when the file is done. Each expect() holds a weak reference to the BunTest it was created under, which is how add_count and the .snap selection in get_or_put find the file a snapshot belongs to.
• TestCommand::run (test_command.rs) runs one test file (looping for --rerun-each, calling enter_file on every pass); the serial runner calls it per file, and each --parallel worker process calls it for every file it is handed.

First revision of this PR

The first push kept the map on Snapshots and cleared it at the top of every file pass in TestCommand::run (deleting the clear in write_snapshot_file). That fixed the same cases, but it was the second time the reset point of this map had to be moved by hand (#29375 added the first two for #23705), so the current revision moves the map onto the per-file object instead, which removes the reset calls rather than relocating them.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 3 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: 559b90ac-3195-4d0c-bcc0-4355b8b540a0

📥 Commits

Reviewing files that changed from the base of the PR and between b44b2c4 and c51d4da.

📒 Files selected for processing (6)

• src/runtime/cli/test_command.rs
• src/runtime/test_runner/Execution.rs
• src/runtime/test_runner/bun_test.rs
• src/runtime/test_runner/snapshot.rs
• test/cli/test/parallel.test.ts
• test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts

---

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

[robobun]

Status: reproduced on the current release with two test files in one directory (bun test ./b.test.ts writes exports[`b 2`], bun test ./a.test.ts ./b.test.ts writes exports[`b 1`]; an inline-only first file makes the next file start at same name 2; under --parallel every file after a worker's first gets the wrong number). The fix in this PR moves the counters onto the per-file BunTest. The three new tests in test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts and the new one in test/cli/test/parallel.test.ts fail on the release build and pass with the change.

[robobun]

^{Updated 7:05 AM PT - Aug 15th, 2026}

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

---

🧪   To try this PR locally:

bunx bun-pr 38874

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

bun-38874 --bun

[claude]

LGTM — moves the snapshot counter reset to the test-file boundary where it belongs.

What was reviewed:

• Traced both failure modes: inline-before-first-file-snapshot (counts wiped by write_snapshot_file on .snap switch) and inline-only-file (counts never cleared) — the new reset_counts() at the top of the per-file loop fixes both, and removing the clear from write_snapshot_file is safe because .snap switches only at file boundaries.
• reset_counts changing from zero-in-place to .clear() is equivalent for the retry/repeat caller in Execution::reset_sequence (add_count's get-or-put treats absent and zero identically).
• Checked the other write_snapshot_file callers (end-of-run in test_command.rs:2729, parallel runner) — neither needs the counts afterward.
• Tests cover all three described scenarios with exact .snap contents, use describe.concurrent, drain pipes, and set CI explicitly.

Extended reasoning...

Overview

This PR fixes snapshot key numbering so it is deterministic per test file, independent of which files ran earlier in the same bun test process. The change touches four files: test_command.rs (moves reset_counts() from the repeat_index > 0 branch to run unconditionally at the top of every per-file iteration), snapshot.rs (removes counts.clear() from write_snapshot_file, changes reset_counts to .clear() the map, adds a doc comment on the counts field), Execution.rs (comment-only wording tweak), and three new subprocess tests in snapshot.test.ts.

Security risks

None. This is test-runner bookkeeping state (a StringHashMap<usize> of test-name → snapshot-matcher count) with no user-controlled input beyond test names, which were already handled.

Level of scrutiny

Medium-low. The mechanism is well-explained in the PR description and matches the code: counts was previously cleared only inside write_snapshot_file, which runs lazily on .snap file switch, so inline snapshots taken before a file's first file-snapshot were either wiped (if a previous file had a .snap open) or leaked into the next file (if no .snap was ever opened). Moving the reset to TestCommand::run's per-file loop is the correct layer — it's the test-file boundary that both serial and --parallel workers go through.

I verified that removing counts.clear() from write_snapshot_file cannot lose a needed reset: within a single test file the file_id never changes, so get_snapshot_file only calls write_snapshot_file at file boundaries, exactly where the new reset_counts() now runs. The two other write_snapshot_file callers (end-of-run flush and parallel-runner flush) don't need counts afterward.

The reset_counts change from zero-in-place to .clear() is behaviorally equivalent for its other caller (Execution::reset_sequence on retry/repeat): add_count uses get_or_put and treats a missing key the same as a zero value. Clearing also stops the map from accumulating every test name across the run, which is a small win.

Other factors

The three new tests directly encode the failure scenarios from the PR description, assert exact .snap file bytes (not just "passes"), spread bunEnv while overriding CI, drain stdout/stderr/exited concurrently, and run under describe.concurrent since they're independent subprocess spawns. The PR description lists the existing test files that were re-run (rerun-each.test.ts, ci-restrictions.test.ts, parallel.test.ts, etc.) covering the paths whose behavior could have shifted. The --rerun-each reset that was previously guarded by repeat_index > 0 is subsumed by the unconditional call, so no coverage is lost.

[claude]

I reviewed this PR and didn't find any bugs. The fix is well-reasoned — moving the counters onto BunTest gives them the right per-file lifetime by construction — but it touches test-runner state management with new unsafe derefs (reset_sequence, add_count), so a human look at the aliasing reasoning and the CI status on the latest commit would be worthwhile.

Checked: add_count's buntest_strong.get() doesn't overlap the earlier bun_test borrow in get_or_put (last use is file_id, dead under NLL); reset_sequence's raw deref of snapshot_counts is disjoint from the live sequence borrow into execution.sequences, matching the discard_junit_failure pattern just above it. The removed --rerun-each reset is covered because enter_file builds a fresh BunTest per pass. The comment-cop note on bun_test.rs looks addressed by c51d4da (field doc is now one line).

Extended reasoning...

Overview

This PR fixes snapshot key numbering when multiple test files run in one bun test process. It moves the per-snapshot-name counter map from the process-wide Snapshots struct to the per-test-file BunTest struct, deleting three hand-placed reset calls (reset_counts in --rerun-each, the clear in write_snapshot_file, and the Jest::runner() reach-through in reset_sequence) in favour of the map dropping with BunTest on exit_file. Four files touched in src/runtime/ plus four new tests across two test files.

Security risks

None. This is internal test-runner bookkeeping; no user input parsing, no I/O path changes, no auth/crypto.

Level of scrutiny

Medium-high. The change is architecturally the right one (REVIEW.md: "Store state on the object whose lifetime matches it"), and the diff is small, but it introduces a new unsafe { (*buntest.as_ptr()).snapshot_counts.clear() } in reset_sequence and a new BunTestCell::get() call inside Snapshots::add_count while the caller get_or_put already holds an Rc to the same cell. I traced both: the get_or_put borrow's last use is bun_test.file_id before add_count is called, so no aliased &mut; the reset_sequence deref touches a field disjoint from execution.sequences (where sequence points), and mirrors the existing discard_junit_failure pattern immediately above it. Still, this is exactly the class of change where a maintainer should confirm the Stacked-Borrows reasoning.

Other factors

• The PR description is thorough and the four new tests each pin a distinct failure mode (inline-before-file-snapshot, CI read-back, inline-only carryover, --parallel); the author confirmed each fails on the release build.
• The --rerun-each and retry/repeat guards from #23705 are covered by rerun-each.test.ts per the description.
• robobun reported CI failures on commit 978474f; two commits landed after that (7a1a268, c51d4da) and the current CI status isn't visible in the timeline.
• The github-actions comment-cop flagged a long comment on bun_test.rs; c51d4da shortened the field doc to one line, so that looks addressed.

Given the unsafe-code surface and unclear CI status on HEAD, deferring rather than approving.

[robobun]

Re-verified against current main (367d939). This branch is 1,013 commits behind it and merges without a conflict. All results below are from a debug ASAN build of the merge result.

• The four new tests pass: three in snapshot.test.ts (snapshot numbering across test files) and one in parallel.test.ts. A debug build of main alone fails all four.
• These suites pass: test/cli/test/rerun-each.test.ts, test/cli/test/retry-flag.test.ts, test/js/bun/test/test-retry-repeats-basic.test.ts, test/js/bun/test/ci-restrictions.test.ts, test/cli/test/test-filter-lifecycle-snapshot.test.ts, test/js/bun/test/bun_test.test.ts, test/js/bun/test/test-test.test.ts.
• test/js/bun/test/snapshot-tests/ as one bun test process (7 files): 78 pass, 1 fail. No committed .snap file changes. The failure is error snapshots. It fails the same way without this change when the output has no colour (test: make the error snapshots test pass with colors disabled #38833).
• test/cli/test/parallel.test.ts: 43 pass, 2 fail. Both failures depend on timing, not on snapshots. each worker has a unique JEST_WORKER_ID also fails 2 of 3 runs on a debug build of main alone. lazily scales workers based on file duration passes when it runs alone.
• ASAN reported nothing in these runs.