Repository navigation
Conversation
The snapshot file was opened with O_TRUNC under --update-snapshots and only written when the next test file opened its own snapshot (or at process exit), so an afterAll() that read the .snap file saw zero bytes. Drop the eager truncate, track a needs_write dirty bit, and flush (seek + write + ftruncate) when execution reaches the root describe's afterAll groups. The end-of-run write_snapshot_file path now reuses the same flush, and a passing run with no new snapshots no longer rewrites an unchanged file. Fixes #12114
WalkthroughChangesThe test runner now tracks pending snapshot writes and flushes them when execution reaches the current scope’s Snapshot flush coordination
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 4:05 AM PT - Aug 1st, 2026
❌ @robobun, your commit 13576e4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36679That installs a local version of the PR into your bun-36679 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 874-879: Update the snapshot flush branch in step_group to
propagate the Result from runner.snapshots.flush_current_file() instead of
discarding it. Only set snapshot_flush_group to usize::MAX after the flush
succeeds; return the flush error immediately when it fails.
In `@test/js/bun/test/snapshot-tests/new-snapshot.test.ts`:
- Around line 75-85: The snapshot test currently uses identical output and
substring assertions, so it does not exercise truncation or detect stale
trailing bytes. Update the fixture and first run to create a longer snapshot,
then change it before the --update-snapshots run to produce a shorter one; in
the afterAll output assertions, require the exact framed length or full body,
and verify the final file exactly matches the shorter snapshot with no suffix
from the original.
🪄 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: a3083844-fb8c-446b-a681-65e6f7ab0f38
📒 Files selected for processing (6)
src/runtime/cli/test_command.rssrc/runtime/test_runner/Execution.rssrc/runtime/test_runner/Order.rssrc/runtime/test_runner/bun_test.rssrc/runtime/test_runner/snapshot.rstest/js/bun/test/snapshot-tests/new-snapshot.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… too Replaces the root-afterAll group index tracking with a simpler check in step_group: when a beforeAll/afterAll group (all sequences have test_entry == None) is about to start, flush pending snapshot writes. needs_write makes the beforeAll case a no-op. Order.rs and bun_test.rs are back to their original shape.
robobun
changed the title
There was a problem hiding this comment.
LGTM — both prior concerns are addressed and the reworked approach is simpler than the original.
What was reviewed:
flush_current_file's seek(0)+write_all+ftruncate against the O_TRUNC removal;needs_writeonly clears on full success so the end-of-runwrite_snapshot_file()?still surfaces I/O errors..all()on an empty/beforeAll group is a no-op vianeeds_write;Jest::runner()here matches the existingreset_sequencepattern in the same file.- Single
Snapshots { .. }construction site initializesneeds_write; parallel workers spawn subprocesses that go through the samestep_grouppath. get_end_posis fstat-based (no cursor move), so the non--uread-then-rewrite path is unchanged aside from now skipping the write when nothing was added.
Extended reasoning...
Overview
Fixes #12114: .snap files are now flushed to disk before beforeAll/afterAll hooks run, at any describe depth. Changes: snapshot.rs gains a needs_write flag and flush_current_file() (seek(0) + write_all + ftruncate, fd stays open), write_snapshot_file delegates to it, and O_TRUNC is dropped from the open flags so the on-disk file is never emptied mid-run. Execution.rs::step_group calls flush_current_file() before starting any group whose sequences all have test_entry == None. test_command.rs initializes the new field. Three new subprocess tests cover first-create, -u with a long→short value (exercising ftruncate), multi-file, and nested describes.
Prior review
My previous run flagged (1) the original root-scope-only snapshot_flush_group missing describe-level afterAll, and (2) the discarded I/O result needing a comment. Commit 13576e4 addressed both: the Order.rs/bun_test.rs index-tracking was dropped entirely in favor of a hook-only-group check directly in step_group (idempotent via needs_write), the best-effort comment was added, and a nested-describe test was added. CodeRabbit's two comments (error propagation, ftruncate test coverage) were also resolved.
Security risks
None. Local file I/O to __snapshots__/*.snap only; no user-controlled paths, network, or auth.
Level of scrutiny
Moderate — touches the test runner's step_group loop and snapshot-file write path. I traced: needs_write is only set in get_or_put and the -u branch of get_snapshot_file, and only cleared as the last line of flush_current_file after all three I/O ops succeed, so a mid-run flush failure leaves the retry armed for the end-of-run write_snapshot_file()?. bun_sys::ftruncate is cross-platform (libc / NtSetInformationFile) and already used by write_inline_snapshots in the same file. get_end_pos uses get_file_size (not lseek), so cursor semantics on the non--u read path are unchanged. The Jest::runner() call in step_group matches the existing pattern at reset_sequence in the same file. The parallel runner forwards --update-snapshots to worker subprocesses that run the same step_group, so no separate fix is needed there.
Other factors
The one intentional behavioral side effect — a passing run with no new snapshots no longer rewrites an unchanged .snap — is called out in the PR description. Tests follow harness conventions (tempDir, bunEnv spread, concurrent subprocess drain, exitCode asserted last) and the ftruncate case asserts exact-body equality between what afterAll observed and the final on-disk file. PR description reports the surrounding suites (bun_test.test.ts, snapshot-tests/, etc.) pass unchanged.
|
CI on 13576e4: the diff's own test file ( The red lanes are unrelated to this change:
Ready for review. |
|
Closing this PR because it has been inactive for more than 90 days. |
|
This PR stays closed.
The other part of this PR wrote the new contents before the |
Fixes #12114.
Repro
Cause
Snapshots::get_snapshot_fileopened the.snapfile withO_TRUNCunder--update-snapshots, andwrite_snapshot_fileonly ran when the next test file opened its own snapshot or at process exit. Between those two points (which includes everyafterAllin the file) the on-disk file was zero bytes. The first-create path had the same shape:O_CREATmade an empty file that wasn't written until later.Fix
src/runtime/test_runner/snapshot.rsO_TRUNCfrom the open flags; the old bytes stay on disk until we overwrite them.needs_write(set whenget_or_putappends a new entry, or under-uwhen the buffer is rebuilt from a clean header) and addflush_current_file, which doesseek(0) + write_all + ftruncate(len)and leaves the fd open so a latertoMatchSnapshotcan keep appending.write_snapshot_filenow callsflush_current_fileand then closes/clears. A passing run with no added snapshots no longer rewrites an unchanged file.src/runtime/test_runner/Execution.rsstep_groupflushes the snapshot file whenever a hook-only group (beforeAll/afterAll, any describe depth) is about to start.needs_writemakes thebeforeAllcase and repeated flushes no-ops. The flush is best-effort here; a persistent write error still surfaces via the end-of-runwrite_snapshot_file()?.Verification
New tests in
test/js/bun/test/snapshot-tests/new-snapshot.test.ts: the #12114 repro on first create,-uwith a shorter value than the existing snapshot (exercises theftruncatepath and checks the exact body seen byafterAllagainst the final file), a two-file-urun where each file'safterAllreads its own.snap, and a nesteddescribewhose inner and outerafterAllboth read the flushed file. All fail onmainand pass with this change.bun_test.test.ts,ci-restrictions.test.ts,test-filter-lifecycle-snapshot.test.ts, and the rest ofsnapshot-tests/pass unchanged.#34042 also touches this area as part of obsolete-snapshot detection; this PR is the narrow fix for the
afterAllvisibility bug and should rebase cleanly onto it in either direction.[review] gate passed · iteration 0 · 4 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