Conversation
…as added Snapshots loaded from the .snap file but never matched during a run were previously invisible: a plain run gave no signal and -u silently rebuilt the file while booking every rewritten entry through the "added" counter. Snapshots now tracks an unchecked_keys set populated at parse time and drained on each match. Remaining entries at file close are tallied as obsolete (plain run) or removed (-u). Under -u the old file is read before the rebuild so a key that already existed counts as passed rather than added, and the file is truncated after write instead of at open. Skipped, todo, and -t filtered tests mark their keys checked so they are not reported as obsolete; --only suppresses the obsolete line entirely since those tests never reach the reporter.
|
Updated 11:09 AM PT - Jul 12th, 2026
❌ @robobun, your commit a024016 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34042That installs a local version of the PR into your bun-34042 --bun |
snapshots/snapshot.test.ts has an env-dependent pre-existing failure (error snapshots needs FORCE_COLOR) that would mask the fail-before check; keep the new coverage in its own file.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughChangesThe snapshot runner now tracks unchecked snapshot keys to classify matched, obsolete, and removed entries. Skipped tests are marked checked, CLI output reports new counters, and snapshot tests cover update, filtering, and skip behavior. Snapshot obsolete tracking
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/Execution.rs`:
- Around line 711-722: Update snapshot bookkeeping for tests excluded by .only()
in addition to the existing skipped, todo, and label-filtered ExecutionSequence
results. Ensure each filtered-out sibling’s snapshot name is passed to Jest’s
mark_snapshots_as_checked_for_test before snapshot cleanup, even when no
ExecutionSequence is created, while preserving the existing handling for
sequences that do exist.
In `@src/runtime/test_runner/snapshot.rs`:
- Around line 359-362: Handle the return value of bun_sys::ftruncate in the
update_snapshots branch instead of discarding it. Propagate the failure using
the same fatal error behavior as write_inline_snapshots, ensuring the snapshot
rewrite cannot return success while stale trailing data remains; keep the
subsequent file.file.close flow unchanged for successful truncation.
In `@test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts`:
- Around line 957-1026: Add a snapshot test alongside the obsolete/update cases
that combines test.only(...) with the -u option, including a selected test and a
sibling test with existing snapshots. Assert the run succeeds and the rewritten
snapshot file still contains the sibling snapshot, while preserving the selected
test’s update behavior. Use the existing temp-directory, snapFixture, run, and
snapshot-file assertions.
- Around line 936-1027: Make the “obsolete snapshot detection” suite concurrent
by changing its describe declaration to describe.concurrent, allowing all
contained subprocess and file-I/O tests to run in parallel. Preserve the
existing test bodies, fixtures, assertions, and isolation behavior.
🪄 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: c86e2eef-6f76-4ef8-b3ff-45304cdb9cb9
📒 Files selected for processing (4)
src/runtime/cli/test_command.rssrc/runtime/test_runner/Execution.rssrc/runtime/test_runner/snapshot.rstest/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts
…ftruncate error Skipped/todo/filtered test names are now recorded with their file id and reconciled against unchecked_keys in write_snapshot_file, so a test.skip declared before the first toMatchSnapshot call is still excluded from the obsolete count. Under -u the reconciliation is skipped so the removed tally matches what is physically dropped from the .snap file. ftruncate failure after the -u rewrite now propagates as FailedToWriteSnapshotFile instead of returning Ok with stale trailing bytes. Tests: skip-before/after-first-match ordering, -u + skip removal tally, describe made concurrent.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/cli/test_command.rs:2855-2859— The obsolete-count suppressionif reporter.jest.only { 0 }only covers the CLI--onlyflag — in-sourcetest.only()setsscope.base.only = Only::Containson the describe tree and never touchesreporter.jest.only, and non-only entries are dropped atOrder.rs:103before ever getting anExecutionSequence, so their snapshot keys stay inunchecked_keysand are reported as obsolete. Every developer who temporarily focuses a test with.only()will see a spurious "N obsolete" plus a hint to runbun test -u, which then physically deletes the non-only tests' still-valid snapshots. Fix by also suppressing when the file's root scope ended upOnly::Contains(or set a "had in-source only" flag on the runner/snapshots that the summary check reads).Extended reasoning...
What the bug is
The PR suppresses the obsolete-snapshot count when
--onlyis in effect, because non-only tests never run and their snapshot keys would otherwise all show up as obsolete:let obsolete = if reporter.jest.only { 0 } else { reporter.jest.snapshots.obsolete };
But
reporter.jest.only(TestRunner.only, jest.rs:116) reflects only the CLI--onlyflag. It is initialized fromctx.test_options.onlyand the sole write to it anywhere insrc/runtime/is the scopeguard restore at test_command.rs:3121, which puts it back to the CLI value after each module. In-sourcetest.only()goes through a completely separate path:BaseScopeCfg { self_only: true }→base.only = Only::Yes(bun_test.rs:1701) →mark_contains_only()walks the parent chain settingscope.base.only = Only::Contains(bun_test.rs:1758–1769). Nothing on that path ever setsrunner.only.Why the mark-as-checked hook doesn't help here
The PR's new
mark_snapshots_as_checked_for_testhook fires fromon_sequence_completedforSkip/Todo/SkippedBecauseLabelresults. But when a file uses in-source.only(), non-only entries never get that far:Order::generate_orderreadsscope_only = current.base.onlyand, when it isOnly::Contains,continues straight past every entry whoseonly == Only::No(Order.rs:101–105). Those entries are never handed togenerate_order_sub, so noExecutionSequenceis created for them,on_sequence_completednever fires, and their snapshot keys are never removed fromunchecked_keys.This is a different mechanism from the already-reported ordering bug on this PR: that one is about skipped/filtered tests that are sequenced but complete before the
.snapfile is lazily opened. This one is about tests that are never sequenced at all, so no amount of deferring the mark-as-checked call will reach them.It also differs from
-tfiltering:-tsetsScopeMode::FilteredOut, which still creates a sequence and completes withResult::SkippedBecauseLabel, so the hook fires..only()drops entries at order-generation time.Step-by-step proof
Given
snap.test.ts:test.only("a", () => expect(1).toMatchSnapshot()); test("b", () => expect(2).toMatchSnapshot());
and
__snapshots__/snap.test.ts.snapcontaining both`a 1`and`b 1`, runningbun test(no--onlyflag):- Collection:
test.only("a", …)sets its ownonly = Only::Yesand callsmark_contains_only()on its parent chain → root describe'sbase.only = Only::Contains.test("b", …)hasonly = Only::No.runner.onlystaysfalse(CLI flag not passed). Order::generate_orderon the root scope:scope_only == Only::Contains, so at Order.rs:103 the entry forb(only == Only::No) hitscontinue— no sequence is generated for it.- Execution: only
aruns.toMatchSnapshot()→get_snapshot_fileopens the.snap,parse_filepopulatesunchecked_keys = {"a 1", "b 1"}.get_or_putremoves"a 1"."b 1"remains. - No sequence for
bexists, soon_sequence_completednever fires for it andmark_snapshots_as_checked_for_test("b")is never called. write_snapshot_file:unchecked_keys.len() == 1→self.obsolete += 1.- Summary at test_command.rs:2855:
reporter.jest.onlyisfalse(it was never set — and even if something per-module had set it, the scopeguard at 3121 restored it to the CLI value before this point). The suppression does not fire. Output:snapshots: 1 passed, 1 obsoleteplusTo remove obsolete snapshots, run bun test -u.
Following that hint runs the same order-generation drop,
bnever writes into the rebuiltfile_buf, andwrite_snapshot_filetruncates —`b 1`is physically deleted from disk and reported as1 removed.Impact
test.only()is one of the most common iteration workflows — temporarily focusing on one test while editing. With this PR every such run prints a false-positive obsolete warning for every other snapshot-using test in the file, and the accompanying hint steers the user toward-u, which then destroys those tests' snapshots. When.only()is later removed, all of them come back asaddedwith no diff against their previous stored values. The PR author clearly intended to suppress this case (the--onlyguard is there for exactly this reason) but only wired it to the CLI flag.The new test file covers
test.skipand-tbut not in-sourcetest.only(), so this is unexercised.How to fix
The cleanest option is to suppress per-file at the point the count is produced: when a file's root describe ends up
Only::Containsbecause of in-source.only(), skip theself.obsolete += uncheckedtally inwrite_snapshot_filefor that file (e.g. set ahad_in_source_only: boolonSnapshotsfrommark_contains_only, or check the root scope'sonlystate before callingwrite_snapshot_file). Alternatively, keep the summary-time check but read a flag that is set whenever any file used in-source.only(), not just the CLI flag.Add a test alongside the existing
test.skip/-tcases that usestest.only("keeps", …)+test("skipped", …)and asserts the summary does not containobsolete, plus a-uvariant assertingskipped 1survives on disk. - Collection:
note_skipped_test is now called for every non-Pass sequence result (Jest calls markSnapshotsAsCheckedForTest on test_done for pass and fail), so a test that throws before reaching its toMatchSnapshot is not flagged obsolete. The root describe's Only state is checked per file so both CLI --only and in-source test.only() suppress the obsolete tally for that file, replacing the summary-time reporter.jest.only check which only covered the CLI flag.
|
Addressed the additional findings in 703e806:
10 tests in |
…pped-names vec
file_was_partial replaces had_only_in_file and is also set on any failing
sequence (including hook-only sequences), so tests jumped over by a
beforeAll failure or a throwing describe callback do not produce a false
obsolete hint on an already-red run.
mark_snapshots_as_checked_for_test also matches '{test_name}: {hint}'
prefixes so a skipped test with a hinted toMatchSnapshot is not flagged.
get_snapshot_file prunes skipped_test_names to the file being opened so
entries from files with no snapshot calls do not accumulate.
|
CI is red on |
Dropping the '!= Pass' guard means a test.failing() body that throws (result is Pass with maybe_skip) still has its key excluded from the obsolete tally; noting a fully-passed test is a no-op since get_or_put already removed its keys. partial_file_id replaces the bool so a file with .only() or a failure that never opens a .snap file cannot suppress the next file's obsolete tally.
A throwing describe callback is handled during collection and never produces a failing ExecutionSequence, so the per-file partial_file_id check does not catch it. The run is already red via unhandled_errors_between_tests, so gate the obsolete hint on that counter being zero.
…cted; share name builder Noting a cleanly passing test's name caused mark_snapshots_as_checked_for_test to strip that test's higher-numbered keys (e.g. 'foo 2' when only 'foo 1' was matched), defeating the headline use case of obsolete detection. Restore the '!= Pass' guard and add a ScopeMode::Failing arm so a test.failing body that throws is still noted. Expect::build_snapshot_name is extracted from get_snapshot_name and reused by on_sequence_completed so the key format has a single source of truth.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/test_runner/Execution.rs:717-719— 🟡partial_file_idis a singleOption<FileId>written by plain assignment, so it can be overwritten cross-file: if file A setsSome(A)(via.only()or a fail) and then file B's first sequence completes (fail or.only()) before B's firsttoMatchSnapshot(), B writesSome(B)while_current_fileis stillSome(A); A's deferred flush then seesSome(B) != Some(A)and falsely tallies A's.only()-sibling key as obsolete. This is the reverse of the false-negative 8057d85 fixed — B no longer suppresses A, but B can now clobber A's own suppression. Reporting-only and a narrow multi-file ordering, so not blocking; a smallVec<FileId>(or a per-file-boundarywrite_snapshot_file()) would close it.Extended reasoning...
What the bug is
Commit 8057d85 replaced
file_was_partial: boolwithpartial_file_id: Option<FileId>so a later file B setting the flag can't suppress an earlier file A's obsolete tally (the false-negative direction). But it's still a single slot written by plain assignment at Execution.rs:718 —runner.snapshots.partial_file_id = Some(file_id)— so the reverse case is now broken: if A wroteSome(A)for its own suppression, B's first completing sequence overwrites it toSome(B), and A's deferred flush loses the suppression it needed.Code path
There is no per-file-boundary
write_snapshot_file()in the sequential loop — its three call sites are the lazy switch insideget_snapshot_file(snapshot.rs:~908), end-of-run (test_command.rs:~2645), and parallel-worker teardown (runner.rs:~731, whose comment reads "Snapshots flush lazily when the next file opens its snapshot file"). So file B'son_sequence_completedruns while_current_fileis stillSome(A). The flush check at snapshot.rs:~361 iselse if self.partial_file_id != Some(file.id)— withpartial_file_id == Some(B)andfile.id == Athat'strue, so the obsolete branch runs for A.Note that
skipped_test_namesalready avoids this class by carrying(FileId, _)per entry and filtering on*id == file.id;partial_file_idhas no such per-file protection because it holds only one id.Step-by-step proof
Two files, sequential run, no
-u:- a.test.ts:
test.only("keeps", () => expect({a:1}).toMatchSnapshot()); test("sibling", () => expect({b:2}).toMatchSnapshot());—__snapshots__/a.test.ts.snapcontains bothkeeps 1andsibling 1. - b.test.ts:
test("first", () => { throw new Error("boom") }); test("second", () => expect({c:3}).toMatchSnapshot());
- A's
keepsruns →get_snapshot_file(A)opens A's.snap,unchecked_keys = {"keeps 1", "sibling 1"}, removes"keeps 1"._current_file = Some(A). - A's
keepscompletes →on_sequence_completed:root_only == Only::Contains→partial_file_id = Some(A). (siblingwas dropped inOrder::generate_orderbefore sequencing, sonote_skipped_testnever fires for it.) - File A finishes; file B starts. No flush —
_current_fileis stillSome(A). - B's
firstthrows →on_sequence_completed:sequence.result.is_fail()→partial_file_id = Some(B)(overwrites A). - B's
secondcallstoMatchSnapshot()→get_snapshot_file(B)→write_snapshot_file()for A.self.partial_file_id (== Some(B)) != Some(file.id == A)→ true → obsolete branch runs.skipped_test_nameshas no entry matchingsibling(it was never sequenced), so"sibling 1"stays inunchecked_keys→self.obsolete += 1.
Output:
snapshots: … 1 obsolete+To remove obsolete snapshots, run bun test -u— for A's.only()-sibling snapshot, exactly the false positivepartial_file_idwas meant to prevent.There's also a green-run variant: if B uses
test.only()on a test that completes before B's firsttoMatchSnapshot()(instead of a failure), the same overwrite happens on an exit-0 run, and neither theunhandled_errors_between_testssummary suppression (test_command.rs:~2861) nor a red exit code masks the false positive.Why existing code doesn't prevent it
robobun's reply resolving the earlier backward-leak comment said "a later file setting Some(B) does not suppress file A since the ids do not match" — correct for the false-negative direction (B ≠ A ⇒ A's obsolete branch runs). But that same inequality is why A's own suppression is lost when A had set
Some(A)first: the single slot can't remember that both A and B were partial. There is no reset between files, but a reset wouldn't help either — the write and the read both happen after B's overwrite.Impact
Reporting-only false positive in the new obsolete counter. Narrow trigger: requires a specific multi-file ordering (A partial + opened its
.snap; B's first completing sequence is fail/.only()and precedes B's firsttoMatchSnapshot()). In the failure variant the run is already red, so users are unlikely to blindly follow the-uhint; the.only()-in-both-files variant can hit on a green run but is a dev-loop state. No on-disk change without-u; following the hint under-udeletes the sibling entry, but that's pre-existing-u+.only()behaviour. Not merge-blocking.How to fix
Either make it a small set — e.g.
partial_file_ids: Vec<FileId>(or a bit on the per-file struct), pushed at Execution.rs:718 and checked with.contains(&file.id)at flush — or add an explicitwrite_snapshot_file()at the per-file boundary in the sequential loop so A is flushed before B's sequences start writing shared state. TheVecis the smaller change and mirrors howskipped_test_namesalready handles the same lazy-flush window. - a.test.ts:
…d errors partial_file_ids: Vec<FileId> replaces the single Option slot so a later file cannot overwrite an earlier file's own suppression across the lazy flush window. mark_file_partial is also called where unhandled_errors_between_tests is incremented (describe-callback throws and errors between tests), so the suppression is per-file instead of the previous run-wide summary gate. The hint-prefix arm gains a comment noting the key-format ambiguity trade-off.
|
Addressed in a024016:
|
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-12, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#12114) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
bun testnow reports snapshot keys that exist in a.snapfile but were never matched during the run, andbun test -ustops mislabeling rewritten entries as "added".Fixes #12114
Problem
Jest prints "1 snapshot obsolete" on the first run and "1 snapshot removed" on the second. Bun had no way to represent "in the file but never matched":
Snapshotscarried onlyadded/passed/failed, and under-uthe file was opened withO_TRUNCso every entry looked brand new. The early truncate also left the.snapfile empty for anything that read it duringafterAll()(#12114).Fix
src/runtime/test_runner/snapshot.rsSnapshotsgainsobsolete,removedcounters and anunchecked_keys: StringHashMap<()>populated byparse_fileand drained on each match inget_or_put.-u,get_snapshot_filereads and parses the old file before clearing the buffer so a key that already existed counts aspassedrather thanadded;write_snapshot_filetruncates after write instead of relying onO_TRUNCat open. A malformed old file is ignored under-uso the existing "replace unparseable file" behaviour is preserved.mark_snapshots_as_checked_for_testremoves every{testName} Nkey for a given test (Jest'skeyToTestNamerule) so skipped/todo/filtered tests are not counted as obsolete.src/runtime/test_runner/Execution.rson_sequence_completedcalls the new hook forSkip/Todo/SkippedBecauseLabelresults, building the name identically toExpect::get_snapshot_name.src/runtime/cli/test_command.rsN removed/N obsolete(with abun test -uhint). The obsolete line is suppressed under--onlysince non-only tests never reach the reporter there.Verification
New coverage in
test/js/bun/test/snapshot-tests/obsolete-snapshots.test.ts: plain-run obsolete count,-uremoved vs added,test.skipand-tnot producing false positives, a truly new key still reportingadded, and the #12114 repro (.snapfile readable inafterAllunder-u). All six fail on current main and pass with this change. Existingsnapshot-tests/,ci-restrictions.test.ts, andbun_test.test.tspass unchanged.[review] gate passed · iteration 3 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 0 rejected · iteration 3
evidence per changed file