Conversation
With --update-snapshots the .snap file was opened with O_TRUNC and every entry was written in test execution order. One changed value reordered the whole file when the tests were not declared in file order. Now the existing file is read and parsed first. A key that a test reaches keeps its position and gets the new value. New keys are appended. Keys no test reaches are dropped. The file is written at offset 0 and truncated to the new length. A file that does not parse is rewritten from scratch, as before.
|
Warning Review limit reached
On-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Or wait 25 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the non-update (plain bun test append) path, which shares the new pwrite_all(0) + ftruncate write: there file_buf always starts as the full old file contents (read via pread_all in get_snapshot_file), so the new buffer is never shorter than the old file and the result matches the previous sequential write_all — the removed Windows seek_to(0) was only needed for that sequential write. The splice at FILE_HEADER.len() is also safe in update mode since file_buf is reset to exactly the header before any entry is appended.
Extended reasoning...
The inline findings cover the non-atomic pwrite/ftruncate ordering, the silent parse-failure fallback, the dropped-unreached-keys behavior, and the test-file placement. Separately I traced the non-update append path through the shared write hunk in src/runtime/test_runner/snapshot.rs (write_snapshot_file, get_snapshot_file): in that mode file_buf is seeded with the entire old file via pread_all, then only grows, so pwrite_all at offset 0 followed by ftruncate to the buffer length yields the same bytes the old write_all (after the Windows seek_to(0)) produced. I also checked that the header-offset splice in update mode cannot land inside an entry, because file_buf is cleared and set to exactly FILE_HEADER before get_snapshot_value appends anything, and that duplicate keys in an old file leave one None slot that is dropped rather than emitting a stale entry.
|
Replied in each thread. No code change: one concern is a test file placement choice made to avoid conflicts with #38974, the other three are pre-existing behaviors of the old O_TRUNC path and are out of scope for the ordering fix. |
|
Closing in favor of #43043. It fixes the same root for #42969 (the |
1 participant
Fixes #42969
Problem
bun test --update-snapshotsrewrites the.snapfile in test execution order. When the tests are not declared in the order of the file, every unchanged entry moves. The reporter saw a 2900 line diff for one changed value.src/runtime/test_runner/snapshot.rs.get_snapshot_fileopens the file withO_TRUNCunder--update-snapshotsand never reads it.get_or_putthen appends every entry tofile_bufin call order.Fix
--update-snapshotsthe existing file is read and parsed first.parse_filerecords one slot per key, in file order. When a test reaches a key that exists,get_or_putfills its slot with the new entry. A new key is appended tofile_bufas before.write_snapshot_fileemits the header, the filled slots in file order, then the new keys. Empty slots (keys no test reached) are dropped, as the truncate did before. The file is written at offset 0 and truncated to the new length.added, as the docs show.test/js/bun/test/snapshot-tests/update-snapshots.test.ts(4 cases, stock bun fails 2). Alsotest/js/bun/test/snapshot-tests/,ci-restrictions.test.tsand the--parallelsnapshot test intest/cli/test/parallel.test.ts.Background
.snapfile is a CommonJS module ofexports[\name N`] = `value`lines.parse_file` parses it with bun's JS parser and keys each value by the hash of its name.--update-snapshotsthe runner already keeps the file as is and appends new keys. This PR gives--update-snapshotsthe same shape. Jest sorts keys on every write instead. Keeping file order causes no one time reorder in existing repos.O_TRUNC,pwrite_allat 0 plusftruncate) is the same as in bun test: stop truncating .snap files on open, write them per file and on --bail #38974. This fix needs it because it must read the file before it writes. Whichever PR lands first, the other rebases cleanly.Notes
--update-snapshotsare still dropped. This includes a run with-t,.onlyor.skip. That is the old behavior and is out of scope here.seek_to(0)after the read is gone. The write is positional now, so the file offset does not matter.test/js/bun/test/snapshot-tests/snapshots/snapshot.test.tshas one pre-existing failure (error snapshots) that depends on terminal colors. It fails the same way on main.-tdropping unreached keys, pre-existing.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
Under
--update-snapshots, the snapshot writer opened the.snapfile with truncation and never read the existing contents, so every entry was emitted in test execution order and a single changed value reordered the whole file. The fix reads and parses the existing file before writing, keeps each reached key at its original position with the regenerated value, appends keys that are new, and drops keys no test reached, then writes the buffer at offset zero and truncates to its length. A file that fails to parse is still rewritten from scratch, preserving the previous fallback behaviour.