Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/runtime/test_runner/expect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@
pub flags: Cell<Flags>,
pub parent: Option<bun_test::RefDataPtr>,
pub custom_label: bun_core::String,
// Source location of the `expect(...)` call itself. Captured here because a
// matcher invoked in tail position (`return expect(v).toMatchInlineSnapshot()`)
// has its JS caller frame eliminated by JSC's proper tail calls, so the
// matcher's own `get_caller_src_loc` sees the *helper's caller* instead.
pub expect_src_file: bun_core::String,
pub expect_src_line: core::ffi::c_uint,
pub expect_src_col: core::ffi::c_uint,
}


Expand Down Expand Up @@ -688,6 +695,7 @@
#[allow(clippy::boxed_local)]
pub fn finalize(mut self: Box<Self>) {
self.custom_label.deref();
self.expect_src_file.deref();
// RefDataPtr = RefPtr<RefData> has NO `Drop` impl (src/ptr/ref_count.rs)
// so the Box drop below would leak the +1 — release explicitly.
if let Some(parent) = self.parent.take() {
Expand Down Expand Up @@ -724,10 +732,18 @@
// error path between ref creation and the wrapper taking ownership; from
// then on `Expect::finalize` derefs `parent` (RefDataPtr has no Drop).

// Capture the `expect(...)` call site now, while the caller's frame is
// still on the stack. A matcher called in tail position cannot recover
// this frame later (see the `Expect` struct comment).
let expect_srcloc = callframe.get_caller_src_loc(global_this);

let expect = Expect {
flags: Cell::new(Flags::default()),
custom_label,
parent: active_execution_entry_ref,
expect_src_file: expect_srcloc.str,
expect_src_line: expect_srcloc.line,
expect_src_col: expect_srcloc.column,

Check failure on line 746 in src/runtime/test_runner/expect.rs

View check run for this annotation

Claude / Claude Code Review

Performance regression: stack walk + sourcemap remap on every expect() call

This adds a stack walk, `computeLineAndColumn`, sourcemap remap (two mutex acquisitions in `remap_stack_frame_positions`), and a `BunString` allocation to **every** `expect()` call — the hottest native call in `bun test` — even though the captured location is only ever read by the inline-snapshot writeback path. REVIEW.md's Performance section is explicit: "Never fix a rare-case bug by adding cost to the hot path" / "The common case pays zero for rare features." Consider capturing only the raw u
Comment thread
robobun marked this conversation as resolved.
Outdated
};
// `JsClass::to_js` boxes `self` and hands the pointer to `${T}__create`.
let expect_js_value = expect.to_js(global_this);
Expand Down Expand Up @@ -1149,10 +1165,24 @@
);
}

// Fallback location: where `expect(...)` itself was called. Only
// usable when that call happened in the same file we write back to.
let (fallback_line, fallback_col) =
if this.expect_src_file.eql_utf8(fget_source_path_text) {
(
core::ffi::c_ulong::from(this.expect_src_line),
core::ffi::c_ulong::from(this.expect_src_col),
)
} else {
(0, 0)
};

// 2. save to write later
runner.snapshots.add_inline_snapshot_to_write(file_id, super::snapshot::InlineSnapshotToWrite {
line: core::ffi::c_ulong::from(srcloc.line),
col: core::ffi::c_ulong::from(srcloc.column),
fallback_line,
fallback_col,
value: core::mem::take(&mut pretty_value).into_boxed_slice(),
has_matchers: property_matchers.is_some(),
is_added: result.is_none(),
Expand Down
155 changes: 143 additions & 12 deletions src/runtime/test_runner/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@
pub struct InlineSnapshotToWrite {
pub line: c_ulong,
pub col: c_ulong,
/// Location of the `expect(...)` call that produced this matcher; used as
/// a fallback starting point when `(line, col)` doesn't land on the
/// matcher name (JSC tail-call elimination can report the helper's caller
/// instead of the matcher call site). `(0, 0)` means no usable fallback.
pub fallback_line: c_ulong,
pub fallback_col: c_ulong,
/// owned (was: owned by Snapshots.allocator)
pub value: Box<[u8]>,
pub has_matchers: bool,
Expand Down Expand Up @@ -89,6 +95,82 @@
}
}

/// Scan `text[from..]` for the matcher identifier `fn_name` as the target of a
/// property call (`.<fn_name>(`), returning its byte offset. Whitespace is
/// permitted around `.` and `(`. Used when JSC tail-call elimination loses the
/// exact matcher-call column and we only have the `expect(...)` location.
fn find_matcher_call(text: &[u8], from: usize, fn_name: &[u8]) -> Option<usize> {
let mut cursor = from;
while let Some(rel) = strings::index_of(&text[cursor..], fn_name) {
let pos = cursor + rel;
let after = pos + fn_name.len();
// Require `.` immediately (modulo JS whitespace) before the name.
let mut b = pos;
while b > from {
let c = text[b - 1];
if c == b' ' || c == b'\t' || c == b'\n' || c == b'\r' {
b -= 1;
} else {
break;
}
}
let dot_ok = b > from && text[b - 1] == b'.';
// Require `(` immediately (modulo JS whitespace) after the name.
let mut a = after;
while a < text.len() {
let c = text[a];
if c == b' ' || c == b'\t' || c == b'\n' || c == b'\r' {
a += 1;
} else {
break;
}
}
let paren_ok = a < text.len() && text[a] == b'(';
if dot_ok && paren_ok {
return Some(pos);
}

Check warning on line 131 in src/runtime/test_runner/snapshot.rs

View check run for this annotation

Claude / Claude Code Review

find_matcher_call heuristic can match decoy inside expect() argument

`find_matcher_call()` scans for `.<fn_name>(` with `strings::index_of` plus a byte-adjacency check, so a decoy occurrence inside the `expect()` argument — e.g. `return expect(".toMatchInlineSnapshot(").toMatchInlineSnapshot()` or a block comment containing `.toMatchInlineSnapshot(` — is matched first and the writeback lexer is seeded inside the string/comment. This only fires on the new tail-call fallback path so it's not a regression of anything that worked before, but REVIEW.md's "use real par
Comment thread
robobun marked this conversation as resolved.
Outdated
cursor = after;
}
None
}

/// Compute the 1-based (line, column) of byte offset `target` in `text`,
/// counting columns in UTF-16 code units to match JSC / source-map semantics.
fn byte_offset_to_line_col(text: &[u8], target: usize) -> (c_ulong, c_ulong) {
use bun_core::strings::{CodepointIterator, Cursor};
let iter_ = CodepointIterator::init(&text[..target.min(text.len())]);
let mut iter = Cursor::default();
let mut line: c_ulong = 1;
let mut col: c_ulong = 1;
let mut prev_cr = false;
while iter_.next(&mut iter) {
match iter.c {
0x0A => {
if !prev_cr {
line += 1;
}
col = 1;
prev_cr = false;
}
0x0D => {
line += 1;
col = 1;
prev_cr = true;
}
0x2028 | 0x2029 => {
line += 1;
col = 1;
prev_cr = false;
}
_ => {
col += if iter.c > 0xFFFF { 2 } else { 1 };
prev_cr = false;
}
}
}
(line, col)
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
pub struct File {
pub id: FileId,
pub file: bun_sys::File,
Expand Down Expand Up @@ -389,18 +471,7 @@
}
});

// 1. sort ils_info by row, col
ils_info.sort_by(|a, b| {
if InlineSnapshotToWrite::less_than_fn(a, b) {
core::cmp::Ordering::Less
} else if InlineSnapshotToWrite::less_than_fn(b, a) {
core::cmp::Ordering::Greater
} else {
core::cmp::Ordering::Equal
}
});

// 2. load file text
// 1. load file text
// avoid `Jest::runner()` (would alias `&mut TestRunner` over the live
// `&mut self` / `ils_info` borrow of `runner.snapshots`). See comment in `parse_file`.
// SAFETY: see `parse_file` — raw-pointer projection to disjoint `.files` field.
Expand Down Expand Up @@ -440,6 +511,66 @@
let source =
bun_ast::Source::init_path_string(test_filename_z.as_bytes(), file_text.as_slice());

// 2a. resolve fallback locations: when the matcher was called in
// tail position, JSC's proper tail calls eliminate the helper's
// frame and `(line, col)` points at the helper's *caller* instead
// of the matcher call site. Scan forward from the `expect(...)`
// location (captured before the tail call) to find the real
// `.<fn_name>(` and fix up `(line, col)` before sorting.
for ils in ils_info.iter_mut() {
// c_ulong is u32 on Windows (LLP64); widen explicitly.
#[allow(clippy::useless_conversion)]
let primary = bun_ast::Source::line_col_to_byte_offset(
&file_text,
1,
1,
u64::from(ils.line),
u64::from(ils.col),
);
if let Some(p) = primary {
if strings::starts_with(&file_text[p..], ils.kind) {
continue;
}
}
if ils.fallback_line == 0 {
continue;
}
#[allow(clippy::useless_conversion)]
let Some(fallback) = bun_ast::Source::line_col_to_byte_offset(
&file_text,
1,
1,
u64::from(ils.fallback_line),
u64::from(ils.fallback_col),
) else {
continue;
};
if let Some(found) = find_matcher_call(&file_text, fallback, ils.kind) {
let (line, col) = byte_offset_to_line_col(&file_text, found);
bun_core::scoped_log!(
inline_snapshot,
"Fallback resolved {}/{} -> {}/{}",
ils.line,
ils.col,
line,
col
);
ils.line = line;
ils.col = col;
}
}

// 2b. sort ils_info by row, col
ils_info.sort_by(|a, b| {
if InlineSnapshotToWrite::less_than_fn(a, b) {
core::cmp::Ordering::Less
} else if InlineSnapshotToWrite::less_than_fn(b, a) {
core::cmp::Ordering::Greater
} else {
core::cmp::Ordering::Equal
}
});

let mut result_text: Vec<u8> = Vec::new();

// 3. start looping, finding bytes from line/col
Expand Down
85 changes: 82 additions & 3 deletions test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,76 @@ Date)
`,
);
});
// #2763: `return expect(v).toMatchInlineSnapshot()` is a tail call, so JSC
// eliminates the helper's frame and the matcher's caller frame points at
// the helper's call site instead of the matcher call site.
it("same-file helper in tail position", async () => {
await tester.test(
v => /*js*/ `
function snap(v) {
return expect(v).toMatchInlineSnapshot(${v("", bad, '`"hello"`')});
}
test("cases", () => {
snap("hello");
});
`,
);
});
it("same-file helper in tail position (toThrowErrorMatchingInlineSnapshot)", async () => {
await tester.test(
v => /*js*/ `
function snap(fn) {
return expect(fn).toThrowErrorMatchingInlineSnapshot(${v("", bad, '`"boom"`')});
}
test("cases", () => {
snap(() => { throw new Error("boom") });
});
`,
);
});
it("same-file helper in tail position with direct calls mixed", async () => {
await tester.test(
v => /*js*/ `
function snap(v) {
return expect(v).toMatchInlineSnapshot(${v("", bad, '`"helper"`')});
}
test("cases", () => {
expect("before").toMatchInlineSnapshot(${v("", bad, '`"before"`')});
snap("helper");
expect("after").toMatchInlineSnapshot(${v("", bad, '`"after"`')});
});
`,
);
});
it("same-file helper in tail position, nested", async () => {
await tester.test(
v => /*js*/ `
function inner(v) {
return expect(v).toMatchInlineSnapshot(${v("", bad, '`"nested"`')});
}
function outer(v) { return inner(v); }
test("cases", () => {
outer("nested");
});
`,
);
});
it("same-file helper, different values at same call site", async () => {
await tester.testError(
{
msg: "error: Failed to update inline snapshot: Multiple inline snapshots on the same line must all have the same value",
},
/*js*/ `
function snap(v) {
return expect(v).toMatchInlineSnapshot();
}
test("cases", () => {
snap("a");
snap("b");
});
`,
);
});
it("indentation", async () => {
await tester.test(
// prettier-ignore
Expand Down Expand Up @@ -890,14 +960,23 @@ test("error snapshots", () => {
expect(() => {
throw undefined; // this one doesn't work in jest because it doesn't think the function threw
}).toThrowErrorMatchingInlineSnapshot(`undefined`);
expect(() => {
expect(() => {}).toThrowErrorMatchingInlineSnapshot(`undefined`);
}).toThrowErrorMatchingInlineSnapshot(`
// The matcher-error message includes ANSI colour codes only when colours are
// enabled (CI sets FORCE_COLOR=1); keep the snapshot check but don't fail on
// the colour-stripped form.
if (Bun.enableANSIColors) {
expect(() => {
expect(() => {}).toThrowErrorMatchingInlineSnapshot(`undefined`);
}).toThrowErrorMatchingInlineSnapshot(`
"\x1B[2mexpect(\x1B[0m\x1B[31mreceived\x1B[0m\x1B[2m).\x1B[0mtoThrowErrorMatchingInlineSnapshot\x1B[2m(\x1B[0m\x1B[2m)\x1B[0m

\x1B[1mMatcher error\x1B[0m: Received function did not throw
"
`);
} else {
expect(() => {
expect(() => {}).toThrowErrorMatchingInlineSnapshot(`undefined`);
}).toThrow("Received function did not throw");
}
Comment thread
robobun marked this conversation as resolved.
});
test("error inline snapshots", () => {
expect(() => {
Expand Down
Loading