test: fix toMatchInlineSnapshot through a same-file tail-call helper - #34978
test: fix toMatchInlineSnapshot through a same-file tail-call helper#34978robobun wants to merge 7 commits into
Conversation
JSC's proper tail calls eliminate the helper's frame when the matcher is called as `return expect(v).toMatchInlineSnapshot()`, so the matcher's caller-frame source location points at the helper's caller (the test body) instead of the matcher call site, and writeback fails with "Could not find 'toMatchInlineSnapshot' here". Capture the `expect(...)` call site on the Expect object (that frame is still on the stack when expect() runs), and at writeback time, when the matcher-time location doesn't land on the matcher name, scan forward from the expect-time location for `.<matcher>(` and use that instead. The direct-call path is unchanged: the matcher-time location is exact there, so the fallback is never consulted and the existing decoy-string handling is preserved.
WalkthroughChangesInline snapshot handling now captures Inline snapshot location handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Updated 3:14 PM PT - Jul 21st, 2026
✅ @robobun, your commit 79c3bc73cf766db3a55ce152e55e7e6ea4ce7342 passed in 🧪 To try this PR locally: bunx bun-pr 34978That installs a local version of the PR into your bun-34978 --bun |
The last assertion snapshots a matcher error message that includes ANSI colour codes only when colours are enabled. CI sets FORCE_COLOR=1 so it passes there, but a plain 'bun bd test' run without FORCE_COLOR sees the plain-text message and fails the snapshot. Gate the snapshot form on Bun.enableANSIColors and fall back to a substring match otherwise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/snapshot.rs`:
- Around line 98-173: Add focused unit tests for find_matcher_call and
byte_offset_to_line_col. Verify matcher detection with whitespace around the
property dot and opening parenthesis, and verify line/column calculations for
CRLF sequences and non-BMP characters counted as UTF-16 surrogate pairs. Keep
the tests local to these helpers and cover both successful matching and expected
offsets.
🪄 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: 333bf800-1965-479e-96c2-24b3213886dd
📒 Files selected for processing (3)
src/runtime/test_runner/expect.rssrc/runtime/test_runner/snapshot.rstest/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…k scan
expect() hot path: capture the caller frame with a new
Bun__CallFrame__getCallerSrcLocUnmapped that skips the sourcemap remap
(two mutex acquisitions + hashmap lookup + VLQ search) and defer the
remap to inline_snapshot() via Bun__remapSrcLoc. The per-expect() cost
is now a single-frame StackVisitor step + computeLineAndColumn + one
WTFString ref.
Fallback scan: parse the expression at the expect() location and read
the matcher's EDot.name_loc off the AST instead of byte-scanning, so a
'.toMatchInlineSnapshot(' substring inside a string/template/comment
argument cannot be matched. Covered by a new decoy test case.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/test_runner/snapshot.rs (2)
475-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFallback resolution logic is correct and handles arbitrary helper-nesting depth.
Parsing always starts at the real
expect(...)token (captured before the tail call), so nested/chained helpers resolve identically to a single-level helper. Thedot.name == ils.kind+ byte-prefix re-validation give reasonable defense against a malformed/garbage parse. This matches the PR's stated coverage (tail-call helpers, nested helpers, decoy text) and is validated by the extensive e2e tests added insnapshot.test.ts.Nit: the comments at Lines 478-483 (6 lines) and Lines 513-519 (7 lines) exceed the 3-line guideline. Deferring to the consolidated comment.
🤖 Prompt for 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. In `@src/runtime/test_runner/snapshot.rs` around lines 475 - 602, The fallback resolution implementation is correct; only reduce the oversized comments in the fallback-resolution loop. Consolidate the introductory explanation and the parsing explanation into concise comments of no more than three lines each, preserving the key rationale and behavior without changing code.Source: Coding guidelines
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeveral newly-added comments exceed the repo's 3-line comment limit.
All six sites share the same root cause: this PR's explanatory comments (ownership contracts / design rationale for the tail-call fallback) are well-justified content-wise but run longer than the project's "Keep code comments to three lines or fewer" rule.
src/runtime/test_runner/snapshot.rs#L475-483: trim the "2a. resolve fallback locations…" block to ≤3 lines (currently 9 total, 6 newly added).src/jsc/bindings/bindings.cpp#L6150-6153: trim theBun__remapSrcLocownership comment from 4 to ≤3 lines.src/jsc/CallFrame.rs#L184-187: trim theget_caller_src_loc_unmappeddoc comment from 4 to ≤3 lines.src/runtime/test_runner/expect.rs#L735-741: trim the "Capture theexpect(...)call site…" comment (now 6 lines total) back to ≤3 lines.src/runtime/test_runner/expect.rs#L1171-1177: trim the "Fallback location…" comment (now 6 lines total) back to ≤3 lines.src/runtime/test_runner/snapshot.rs#L513-519: trim the "Parseexpect(<args>)…" comment from 7 to ≤3 lines.As per coding guidelines, "Keep code comments to three lines or fewer; put longer explanations in documentation only when explicitly requested."
🤖 Prompt for 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. In `@src/runtime/test_runner/snapshot.rs` at line 1, Reduce the six newly added explanatory comments to no more than three lines each while preserving their essential ownership, fallback, and call-site rationale. Update the comments at the identified sites near `Bun__remapSrcLoc`, `get_caller_src_loc_unmapped`, the snapshot fallback and argument parsing logic, and both `expect` fallback locations; make no code changes.Source: Coding guidelines
🤖 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/jsc/bindings/bindings.cpp`:
- Around line 6137-6170: Shorten the explanatory comment above
Bun__CallFrame__getCallerSrcLocUnmapped to three lines or fewer while preserving
that it skips sourcemap remapping for the expect() hot path and that remapping
is deferred to inline-snapshot writeback.
In `@src/jsc/CallFrame.rs`:
- Around line 184-188: Shorten the documentation comment for
get_caller_src_loc_unmapped to three lines or fewer while preserving the
essential description that it skips sourcemap remapping and is intended for hot
paths. Remove secondary implementation details or defer them to the consolidated
documentation.
In `@src/runtime/test_runner/expect.rs`:
- Around line 1171-1177: Shorten the comment immediately above the fallback_line
and fallback_col calculation to three lines or fewer, preserving only the
essential explanation of the fallback location and remap ownership behavior.
Remove redundant detail while leaving the implementation unchanged.
- Around line 735-741: Shorten the comment immediately above
get_caller_src_loc_unmapped in the expect call path to no more than three lines,
retaining only the essential explanation that the caller frame must be captured
before tail-position matchers run and that remapping is deferred for
inline_snapshot.
---
Outside diff comments:
In `@src/runtime/test_runner/snapshot.rs`:
- Around line 475-602: The fallback resolution implementation is correct; only
reduce the oversized comments in the fallback-resolution loop. Consolidate the
introductory explanation and the parsing explanation into concise comments of no
more than three lines each, preserving the key rationale and behavior without
changing code.
- Line 1: Reduce the six newly added explanatory comments to no more than three
lines each while preserving their essential ownership, fallback, and call-site
rationale. Update the comments at the identified sites near `Bun__remapSrcLoc`,
`get_caller_src_loc_unmapped`, the snapshot fallback and argument parsing logic,
and both `expect` fallback locations; make no code changes.
🪄 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: f6cf3f15-1fad-440c-9cde-8de1c10c3b9c
📒 Files selected for processing (5)
src/jsc/CallFrame.rssrc/jsc/bindings/bindings.cppsrc/runtime/test_runner/expect.rssrc/runtime/test_runner/snapshot.rstest/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts
There was a problem hiding this comment.
All three earlier findings (hot-path remap cost, byte-scan decoy, no-colour precision loss) are addressed and I found nothing new. Deferring because this still adds an unconditional StackVisitor::visit + computeLineAndColumn + toStringRef to every expect() call — a residual hot-path tradeoff a maintainer should sign off on — and the parser-driven fallback in snapshot.rs carries enough unsafe/MaybeUninit/FFI-ownership surface to warrant a human look.
What was reviewed:
Bun__remapSrcLocin/out +1 ownership vsremap_stack_frame_positions— early-return leaves the caller's ref intact;dupe_ref+OwnedStringguard ininline_snapshotreleases whichever ref comes back.- Fallback ordering:
(line, col)is rewritten before the sort, so mixed helper/direct calls stay monotone through the writeback loop. byte_offset_to_line_colround-trips with the existingline_col_to_byte_offset(CRLF, LS/PS, astral → 2 UTF-16 units).expect_src_file.deref()is wired intofinalize(); new struct fields don't leak.
Extended reasoning...
Overview
Fixes #2763: toMatchInlineSnapshot() called through a same-file helper in tail position couldn't locate its own call site because JSC's proper tail calls eliminate the helper's frame. The fix captures the expect(...) call site (un-remapped, via a new Bun__CallFrame__getCallerSrcLocUnmapped) at Expect construction time, stores it on the struct, and at writeback time — if the matcher-time location doesn't land on the matcher name — parses the expression at the expect-time location and reads the matcher's EDot.name_loc off the AST. Touches src/jsc/CallFrame.rs (new FFI wrappers), src/jsc/bindings/bindings.cpp (split getCallerSrcLoc into remapping/non-remapping variants + standalone Bun__remapSrcLoc), src/runtime/test_runner/expect.rs (three new Expect fields, capture in call(), remap+compare in inline_snapshot()), src/runtime/test_runner/snapshot.rs (~130-line pre-sort fallback resolution loop + byte_offset_to_line_col), and six new test cases in snapshot.test.ts.
Security risks
None identified. Input is the test file's own source text (already trusted — it was just executed) and a JSC-provided stack location. The fallback parses via the real JS parser rather than byte-scanning (my earlier decoy concern was addressed in 7ce651d), so a .toMatchInlineSnapshot( substring inside a string/comment argument can't misdirect the writeback.
Level of scrutiny
High. Two independent reasons:
- Residual hot-path cost. My earlier 🔴 finding was addressed by dropping the sourcemap remap (mutexes + VLQ search) from the per-
expect()path, but the unmapped capture still runsStackVisitor::visit,computeLineAndColumn, and oneBun::toStringRefon everyexpect()— the hottest native call inbun test. Whether that residual cost is acceptable, or whether the capture should be gated (e.g. only when the current file has inline snapshots), is a design call for a maintainer, not a bot. - Non-trivial memory/FFI surface. New in/out +1
BunStringownership acrossBun__remapSrcLoc; three new owned fields onExpectwith a.deref()infinalize(); aMaybeUninit<TSXParser>+scopeguard+ raw-pointerLogdance duplicated from the existing writeback loop. I traced each of these and they look correct, but the density ofunsafein a file-rewriting path is exactly where a second pair of eyes pays off.
Other factors
All prior review threads (mine and CodeRabbit's) are resolved. Test coverage is good — six targeted cases including the decoy-in-argument case, nested tail calls, mixed with direct calls, and the same-line conflict error — and the PR evidence shows the full 68-test file passing on both debug+ASAN and release. The unrelated error snapshots colour-branch now keeps full-precision inline snapshots on both arms (0dfefe1). The direct-call path is unchanged (fallback only engages when the primary location misses), so regression risk to existing behaviour is low.
|
CI on 79c3bc7 (build #77112): 193 jobs passed. The one red lane is |
|
#39111 fixes the expression-bodied callback shape of this problem ( |
Nested tail-calling helpers, a helper mixed with direct matcher calls, the matcher name appearing inside the expect() argument, and one helper called with two different values (same-line conflict error). All four fail on the released binary with "Could not find 'toMatchInlineSnapshot' here" at the helper's call site. Also make the "error snapshots" assertion hold with colours off, so the file passes under a plain `bun bd test` as well as under CI's FORCE_COLOR=1.
|
Closing in favour of #39111, which fixes the same tail-call problem for this shape (same-file helper) as well as for the expression-bodied callback shape ( |
2 participants
Fixes #2763.
Problem
The error is reported at the test body's line (where
snap(...)is called), not at the matcher call site inside the helper, and the snapshot can never be written. Jest 30 writes the snapshot at the helper's line.Cause
return expect(v).toMatchInlineSnapshot()is a tail call. JSC's proper tail calls eliminate the helper's stack frame, so whenBun__CallFrame__getCallerSrcLocwalks the stack from inside the native matcher it sees the helper's caller (the test body) as the first JS frame. V8 doesn't implement PTC, so Jest never hits this.The same helper without
return(so the matcher is not in tail position) already works:Fix
Capture the
expect(...)call site on theExpectobject at construction time, while the helper's frame is still on the stack (the subsequent.toMatchInlineSnapshot()is the tail call;expect()itself is not). The capture uses a newBun__CallFrame__getCallerSrcLocUnmappedthat skips the sourcemap remap so the per-expect()cost is a one-frameStackVisitorstep and a string ref bump; the remap is deferred to the inline-snapshot path viaBun__remapSrcLoc.At writeback time, if the matcher-time location doesn't land on the matcher name, parse the expression at the expect-time location and read the matcher's
EDot.name_locoff the AST. Parsing (rather than byte-scanning) keeps a.toMatchInlineSnapshot(substring inside a string, template, or comment argument from being matched. The resolved(line, col)then flows through the unchanged writeback loop.The direct-call path is unchanged: the matcher-time location is exact there, so the fallback is never consulted and the existing byte-safe writeback (decoy strings, CRLF/BOM, astral, two-on-one-line) is preserved.
Verification
New cases in
test/js/bun/test/snapshot-tests/snapshots/snapshot.test.tscover:toMatchInlineSnapshotandtoThrowErrorMatchingInlineSnapshot)return inner(v)).toMatchInlineSnapshot(inside a string and comment argumentAll six fail on the released binary and pass with the fix. Full
snapshot.test.ts(68 tests) andexpect.test.js(415 tests) pass.The unrelated
error snapshotsassertion in the same file is made colour-agnostic so the file passes withoutFORCE_COLOR=1(CI sets it; localbun bd testdoes not).[review] gate passed · iteration 2 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 2
evidence per changed file