Repository navigation
bun test: read an older Bun's entry beside the runner's own, and tell a Vitest .snap in key order by a snapshot with both keys - #48565
Conversation
…n key when the file's own entry differs An older Bun read and wrote only its own keys. In a file of Vitest or Jest it appended an entry under its key beside the entry that the runner had for the same snapshot. Bun now asks for the file's key first, and an entry there that does not match under Bun hid the one the older Bun wrote. key_of_older_bun is now the one place that builds Bun's key. The lookup for a missing entry and the new lookup for an entry that differs both use it, so neither runs with --update-snapshots and neither takes an entry that the run wrote. A mismatch now says which text an older Bun may have written: the saved one when its key is also Bun's key, or the entry under Bun's key. Only that text is compared with what an older Bun printed. An entry under a key that only the runner spells is compared in the file's format alone.
…y a snapshot with both keys Keys out of order were the only sign that an older Bun appended to a file of Vitest or Jest. Vitest sorts the keys again when it saves the file, and it saves whenever the file holds an entry that it did not ask for. What an older Bun appended for one describe is in order from the start. A Vitest file in key order is now also taken for one that an older Bun appended to when a snapshot has an entry under Vitest's key and one under the key that an older Bun had for it. The keys alone decide, when the file is read. A file of Jest in key order stays as it is: compared in Jest's format only. A file that is read with its keys in order is written with its keys in order. Only a file that is read out of order keeps that order.
|
Status: ready for review, on top of #48454 How I reproduced it, with Vitest 4.1.9 on Node 26, release 1.4.4 and
The pull request: #48565. Its second commit waits for a yes from a maintainer. The question is at the top of the Notes in the description. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/test_runner/snapshot.rs— Users who runbun test -uwith a name filter,.only, or a skipped test lose the older Bun's entry for every test that did not run, and those snapshots then fail in CI although this PR made them pass. keep_entries_of_tests (snapshot.rs:684) spells the kept names in Vitest's form, soouter > inner 1getsstaysbutouter inner 1does not and snapshot.rs:664 drops it, while Vitest's V8-text entry is left unrewritten. Fix: when a test did not run, keep its Bun-spelled entry too (add full_test_name with Bun's separator to the names set), or drop a Bun-key entry only after the file-key entry was rewritten this run; docs/test/snapshots.mdx:301 promises the rewrite.Why this was flagged
Trigger: a Vitest .snap that an older Bun appended to (
outer inner 1with JavaScriptCore text beside Vitest'souter > inner 1with V8 text), andbun test --update-snapshotsrun while that test istest.skip, filtered out by-t, or outside.only. remove_obsolete at snapshot.rs:652 proceeds because the skipped test's entries are unchecked. keep_entries_of_tests at :662 buildsnameswith full_test_name(test, " > ") at :684, so at :701-707outer > innermatches and gets stays=true whileouter inner 1does not; :664 retains only checked or stays entries and drops the older Bun's one. The Vitest entry is not rewritten because the test never ran. On the next full runouter > inner 1mismatches at :505, key_of_older_bun at :516 finds noouter inner 1and returns None at :470, and expect.rs:1359 fails the test. Before-uthis PR passed it at :520-522, so the PR's own remedy turns a green snapshot red for every test not run. docs/test/snapshots.mdx:301 states--update-snapshotsrewrites Vitest's entry and removes the second one without this condition.Verification: pre-existing. Trigger: a Vitest .snap holding an older Bun's entries and
bun test --update-snapshotsrun while the test owning the pair does not run. In src/runtime/test_runner/snapshot.rskeep_entries_of_testsbuildsnamesonly withfull_test_name(test, format.name_separator(), ...)(line 684), so line 664 dropsouter inner 1whileouter > inner 1keeps its V8 text unrewritten, and the snapshot that passed before-ufails in the next CI run.
| } | ||
| let mut key_of_bun = Self::name_of(expect, Format::Bun, hint)?; | ||
| // The count of a key comes after its last space. | ||
| key_of_bun.extend_from_slice(&key[strings::last_index_of_char(key, b' ').unwrap_or(0)..]); |
There was a problem hiding this comment.
🟡 (optional) Users whose Vitest file an older Bun appended to can get a silent false pass when two tests share one Bun-spelled name. key_of_older_bun at snapshot.rs:460 copies the count from the runner's key, so describe("a b")>test("c") and describe("a")>test("b c") both look up a b c 1. The new mismatch path at snapshot.rs:519-523 then passes the second test on the first test's older entry whenever the values coincide; the base never consulted a sibling on a mismatch. Fix: derive Bun's count from a per-Bun-name counter (as the older Bun did), or refuse an entry already matched by another snapshot this run. [also at: src/runtime/test_runner/snapshot.rs:516 - Users with beforeEach snapshots in a Vitest file an older Bun appended to can get every later test's hook snapshot passed against the first test's older entry. name_of at snapshot.rs:337 uses the active hook entry for Bun's name, and key_of_older_bun at snapshot.rs:460 reuses the count of the runner's key, so test B's outer > B 1 resolves to the same Bun key as test A's.]
Why this was flagged
Older Bun counted per its own key: for describe("a b")>test("c") and describe("a")>test("b c") it wrote a b c 1 and a b c 2. Vitest keys are a b > c 1 and a > b c 1, each with count 1. key_of_older_bun at snapshot.rs:458-460 builds Bun's name and appends the suffix after the last space of the runner's key, so both tests resolve to a b c 1. On the new mismatch path (snapshot.rs:516-523) the second test's Vitest entry differs (V8 text), the sibling a b c 1 holds the first test's JSC text, and format.matches passes the second test on it whenever the two tests print the same value. The base branch never looked up a sibling on a mismatch, so it failed loudly. Also reaches hooks, since name_of at snapshot.rs:337 uses the active hook entry for Bun's name. Remedy: keep a per-Bun-name count when computing Bun's key, or mark an entry consumed once one snapshot matched it.
Verification: key_of_older_bun (snapshot.rs:458-460) builds name_of(expect, Format::Bun, hint) = a b c and appends 1 from the runner's key, so both tests resolve to a b c 1 although 1.4.4 wrote a b c 1 and a b c 2. The new arm at snapshot.rs:516-523 then passes the second test on the first test's entry. On the base, a mismatch under the file's key returned Mismatch { saved } with leniency against saved only.
There was a problem hiding this comment.
The new is_key_of_a_test guard in key_of_older_bun only rejects Bun's key when it is also the file format's spelling of some test's name, so it covers the x y 1 top-level-vs-nested case but not two tests that share one Bun spelling. For describe("a b")>test("c") and describe("a")>test("b c"), Vitest's names are a b > c and a > b c, neither equals a b c, so the guard does not fire; line 469 still copies the count from the runner's key, and both tests still resolve to a b c 1 although the older Bun wrote a b c 1 and a b c 2. The mismatch arm in match_or_write therefore still passes the second test on the first test's older entry when the values coincide (and the same holds for beforeEach hook snapshots via name_of). Deriving the count from a per-Bun-name counter, or marking an entry consumed once a snapshot in this run has matched it, would close this.
…--update-snapshots keeps an older Bun's entry of a test that did not run
The lookup under Bun's key took any entry there. In a file of Vitest, Bun's
key of describe("x") > test("y") is also Vitest's key of a test "x y" at
the top level. A new nested test then got the entry of the test at the top
level in place of its own, and a nested test whose own entry differed passed
on the value of the other test. key_of_older_bun now leaves an entry alone
when its key is one that the file's format has for a test of the file.
With --update-snapshots, the entry that an older Bun left for a test that
was skipped, filtered out or failed went as an entry of no test, while the
runner's entry of that test stayed. The snapshot then failed in the next
run. keep_entries_of_tests now also knows the name that an older Bun had for
such a test, in a file that may hold its entries.
There was a problem hiding this comment.
I re-reviewed the new push (fe1d4e6) and found no new bugs; a maintainer's look is still worthwhile because commit 2's rule — reading an in-order Vitest file as appended to whenever one snapshot has both key spellings — is the design question the description leaves open, and it changes what passes in files no older Bun ever touched.
What was reviewed:
- The
is_key_of_a_testguard inkey_of_older_bunand the cachednames_of_tests: it now refuses the entry of a registered top-level test on both the missing-key and mismatch paths, and the two rewritten tests pin that. - The
keep_entries_of_tests/is_key_ofextraction against the old inline loop:staysdefaults to false, so the oldcontinueand the newfalseare the same; the two-format slice only widens what--update-snapshotskeeps when the file is marked. - The
expect.rsmismatch arm: Bun-format files and entries found under Bun's key still get the older-format comparison onsaved, as before; only unmarked Vitest keys likea > b 1lose it, which is the stated intent. - Not run here: the snapshot test suite and the 49-file corpus the description cites; those claims rest on the author's runs.
Extended reasoning...
The change touches bun test's snapshot lookup in src/runtime/test_runner/snapshot.rs and expect.rs (new OfOlderBun/KeyOfOlderBun enums, a names_of_tests cache on File, a keys_were_out_of_order split, a holds_a_key_in_both_spellings heuristic for in-order Vitest files), plus docs and about 350 lines of new tests with a checked-in older-Bun fixture corpus; it touches no security-sensitive surface. The latest commit adopts the fix from my prior red inline finding and tightens the previously vacuous stderr assertion. What decides defer over approve is that the PR itself asks a maintainer whether commit 2's heuristic is wanted, the lookup is heuristic logic that can pass a snapshot on a sibling entry in Vitest-only projects, and I could not run the suite to confirm the corpus results.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Stacked on #48454. Commit 2 needs a maintainer's yes (Notes).
Problem
.snapfile got a second entry for each snapshot under adescribe. Onmainthat test fails (error: expect(received).toMatchSnapshot(expected)) when Vitest's entry holds another engine's text.Snapshots::match_or_write(src/runtime/test_runner/snapshot.rs:412onmain) reads Bun's key only if the file's key has no entry and the keys are out of order (:378). Vitest sorts them again.Fix
test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts(35 new cases, bun test: do not sort a Jest or Vitest .snap that still holds an older Bun's entries, and write a carriage return as\r#48454 alone fails 21), thentest/js/bun/test/snapshot-tests/.Background
.snapfile in that tool's format. It promises that an older Bun's entries "keep passing".outer > inner 1, Bun and Jestouter inner 1.Downsides
describe, on its old Vitest entry.Notes
A question for a maintainer. #44833 wrote in
snapshot.rs: "Jest and Vitest keep their files sorted, so a file whose keys are in order has none." The second commit changes that for Vitest: a file in key order is taken for one that an older Bun appended to when one snapshot has an entry under Vitest's key and one under the key that an older Bun had for it. Is that wanted?\r#48454.bun test -uonce, or a changed first line.What the answer decides, in files that no older Bun touched: Vitest keeps the entry of a test that was moved into a
describe(outer inner 1besideouter > inner 1) untilvitest -u. With the second commit such a file is read as appended to. The moved test then also passes on the value of the old entry. A failure still shows Vitest's own entry, and a new key still goes to its place in the order. Both are pinned as cases.Repro (release 1.4.4 for step 2,
mainfor step 4, Vitest 4.1.9 on Node 26 for steps 1 and 3)vitest runwritesparser > refuses an open object 1with V8's text:[SyntaxError: Expected property name or '}' in JSON at position 1 (line 1 column 2)].CI=false bun testwith 1.4.4: 1 pass,+1 added. It appendsparser refuses an open object 1as"JSON Parse error: Expected '}'".CI=1 bun testwith 1.4.4 then passes.vitest run:1 obsolete. Vitest saves the file with every key in its order.CI=1 bun testonmain: 1 fail, against V8's text. It is the same before step 3.Where this comes from. No issue and no report: #44833 is in no release, so only a canary build fails. I found it with a run of 1.4.4 and then of
mainon one file. Such a file exists in public code:tests/__snapshots__/init.test.ts.snapofliorcodev/extreme-routerholds each of its 4 snapshots under both keys, in key order. Vitest's entries have aTypeErrormessage of V8, and Bun's entries have the message of JavaScriptCore. I read that file and did not run the project.Why the keys are in order.
@vitest/snapshot4.1.9 sorts all keys when it saves (saveSnapshotFile,dist/index.js:188), and it saves when the file holds a key that no test asked for (this._dirty || this._uncheckedKeys.size,:749). So after the next Vitest run an older Bun's entries are in key order. With Vitest 4 in CI that run also fails:Obsolete snapshots found when no snapshot update is expected. A file of onedescribewith its tests in the order of their names is in order from the start.The three commits
key_of_older_bunis the one place that builds Bun's key. The lookup for a missing entry (frommain, as bun test: do not sort a Jest or Vitest .snap that still holds an older Bun's entries, and write a carriage return as\r#48454 has it) and the new lookup for an entry that differs both call it. So neither runs with--update-snapshots, neither takes an entry that the run wrote, and both mark the entry forwrite_sorted.Outcome::Mismatchnow says which text an older Bun may have written: the saved text when its key is also Bun's key, or the entry under Bun's key beside it.Expect::snapshotcompares only that text with what an older Bun printed. Before, every entry of a marked file was compared that way, also under a key likea > b 1that no older Bun wrote. The failure shows the entry under the file's key, as onmain.format_ofmarks a Vitest file in key order when a key, spelled as an older Bun spelled it, is also a key: each>becomes a space, and the last one a space or:(a hint). The keys alone decide, when the file is read. A file that is read in key order is written in key order. Only a file that is read out of order keeps that order, which is the rule of bun test: do not sort a Jest or Vitest .snap that still holds an older Bun's entries, and write a carriage return as\r#48454.key_of_older_bundoes not take it, so both lookups leave it alone:x y 1is Vitest's key oftest("x y")and Bun's key ofdescribe("x")>test("y"). And--update-snapshotskeeps the entry that an older Bun left for a test that was skipped, filtered out or failed, as it keeps the runner's entry of that test. Before, it removed that entry, and the snapshot failed in the next run.Verdicts against 1.4.4. 49 generated files that 1.4.4 passes after it appended to them outside CI. Each build then runs
CI=1 bun test. No run changed a byte of a.snapfile. The table is from the second commit. The third commit changes a verdict only where Bun's key of a snapshot is also the file's key of another test, and no file that passes in the table has such a key.main98cda0dCI=false bun testof that buildbun test -uof that buildThe 12 more are the pairs: a
describe, a hint, adescribe("")in a Jest file with keys out of order. Of the 29 that stay red in CI, 19 need only the count that an older Bun kept for its own names (a snapshot in a hook, two tests with one Bun name, a matcher call that throws beforemaincounts it). No open PR has that count:key_of_older_bunis where it goes. 2 are cases that #44833 lists as not kept (test.eachtitles with%oand%p, two pending.resolves). 8 are files in key order with no snapshot under both keys: 3 Jest files, and 5 Vitest files with entries under Bun's key only, which oneCI=false bun testadds under Vitest's key.A second corpus is checked in:
test/js/bun/test/snapshot-tests/formats/older-bun. Vitest 4.1.9 on Node 26 wrote both files, Bun 1.4.4 appended to them, and Vitest ran once more on one of them. Its 14 tests: 1.4.4 passes 14,mainand #48454 pass 6, this PR passes 14.What a pair means. A snapshot with both entries passes on the value of either. The file's key is asked first. This is not what 1.4.4 did (it read Bun's key only) and not what
maindoes (the file's key only). One case for a project on both runners:vitest -urewrites Vitest's entry and removes Bun's, so from then on the file has one entry for each snapshot and this PR changes nothing in it. Nothing in the output says that an older entry decided, and the reader has no end date.Still red, or not changed here
maindoes. So thebeforeEachsnapshot of a second test is compared with the older entry of the first (outer (unnamed) 1). It passes if that entry also holds the value of the second test, and fails if not. A local run does not add an entry for it.--update-snapshotsdoes. Example with Vitest 4.1.9 and 1.4.4: abeforeEachsnapshot ofJSON.parse("{")and two tests. 1.4.4 passes 2, bun test: do not sort a Jest or Vitest .snap that still holds an older Bun's entries, and write a carriage return as\r#48454 passes 0, this PR passes 2, before and after Vitest sorts the file.src/runtime/test_runner/Execution.rs:763atbun-v1.4.4), and 1.3.12 did not. bun test: do not sort a Jest or Vitest .snap that still holds an older Bun's entries, and write a carriage return as\r#48454 had such a count and took it out in 4582bd3, because no one count follows every older Bun.test("outer inner")besidedescribe("outer")>test("inner")): the entry under that key is the top-level test's own, and the nested test does not get it. In a file that 1.4.4 wrote for both, the entry of the one that ran first is under that key. If that was the nested test,mainfinds it and this PR does not. The other test fails in that file on both.>, and a Jest file in key order, which stays strict. Both are pinned as not told.--update-snapshotsstill removes the entry that an older Bun left for a snapshot in a hook (d (unnamed) 1) when its tests did not run. bun test: --update-snapshots keeps the entries of tests that did not run to the end, in all three snapshot formats #47988 changes that.Measurements (linux x64, release builds of #48454 at 4582bd3 and of this PR at fe1d4e6,
BUN_JSC_useJIT=0, medians of 7 interleaved rounds)toMatchSnapshot()(perf_event_open,--rerun-each=12minus--rerun-each=2, 40,010 calls): Bun-format file 4949.4 to 4960.0, Vitest file in key order 4852.4 to 4861.6, Vitest file out of order with the entry under the file's key 4852.4 to 4861.4, with the entry under Bun's key 5967.5 to 6503.6. Two runs of the base differ by 0.3 at most. A call that finds its entry under the file's key has no new work in the source: the 9 come with a largerExpect::snapshot. A call that finds it under Bun's key now also asks whether that key is a test's own.mi_malloc/mi_realloc/mi_freeinside the 200th passing call (gdb): 4/2/5, 5/2/7, 5/2/7 and 6/4/9 for the same four files, in both builds.>: +123,921 instructions. 40,000 such keys: +5,077,466 (+2.4%). 1,000 keys with none and one with: +112,213. 1,000 keys with>and then one with none: +802,624. 40,000 and then one: +32,253,668 (+15.2% of a run that does little else than read the file).llvm-size:.text112,692,137 to 112,698,281. The binary: 143,018,256 to 143,026,448 bytes.format_of2305 to 4001,Expect::snapshot(holdsmatch_or_write) 3760 to 4103,inline_snapshot4860 to 4559. New and cold:key_of_older_bun2186,is_key_of1574..snapfiles that Vitest 5.0.3 wrote forformats/vitest: 5 have a key with no>, 0 are marked.Tests (
describe.each(["jest", "vitest"])for files with keys out of order, a group for Vitest files in key order, and the corpus)--update-snapshots -tkeeps it for a test that does not run (both formats)--update-snapshotskeeps the runner's entry of the two (both formats)describe: its old entry passes--update-snapshots>Also run: all of
test/js/bun/test/snapshot-tests/andtest/js/bun/test/ci-restrictions.test.ts. In this containererror snapshotsfails on every build,maintoo: it expects colors. On a debug build some tests of this file take more than their 5 s, as onmain(the corpus of 971 tests and others).Self-review. A review of the first form of this change (against
main, with a walk of the registered tests) said to build it on #48454, to decide the file from its keys, to tie the older print to the key, to keep the runner's entry as the expected text of a failure, to take the test ofcheckedout (a verdict depended on the order of the tests and on an earlier run), to add the cases above, to keep each comment to one true line, and to check a corpus in. All of that is done. The review also said to refuse an entry that is also the key of a registered test. I first left that out, and the review on the PR asked for it again: it is in the third commit.Other open PRs on this function. This PR is on top of #48454 and uses its
Entry.writtenandEntry.found_under_key_of_bun. #47988 rewritesremove_obsolete,keep_entries_of_testsand the writer. The third commit changeskeep_entries_of_teststoo: for a marked file it also takes the names of the tests that did not run to the end in Bun's spelling. #47988 has the same rule there ([file.format, Format::Bun]), and more: hook entries, and its own rule for a test that ran to the end. Where the two meet, the function of #47988 stands, andnames_of_testsandis_key_ofstay forkey_of_older_bun.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts