Skip to content

expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max - #32266

Merged
Jarred-Sumner merged 3 commits into
mainfrom
farm/3ed2d39f/fix-tobearrayofsize-large-array
Jul 16, 2026
Merged

Jarred-Sumner merged 3 commits into
mainfrom
farm/3ed2d39f/fix-tobearrayofsize-large-array

Conversation

@robobun

@robobun robobun commented Jun 15, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a panic in toBeArrayOfSize() and toHaveBeenCalledTimes() when the received array has a length that exceeds i32::MAX.

Repro

expect(new Array(3_000_000_000)).toBeArrayOfSize(5);
panic: called `Result::unwrap()` on an `Err` value: TryFromIntError(PosOverflow)

JS array length goes up to 2^32 - 1, so a cheap sparse array like new Array(3e9) makes i32::try_from(3_000_000_000) return Err and the .unwrap() crashes the whole test process.

The same pattern existed in toHaveBeenCalledTimes(), reachable via fn.mock.calls.length = 3_000_000_000; expect(fn).toHaveBeenCalledTimes(5) since fn.mock.calls is a mutable JSArray.

Fix

Compare the array length and the expected size as i64 instead of narrowing both to i32. get_length() already clamps its result to the i52 range, so the u64 -> i64 cast is lossless, and to_int64() handles the size argument (already guarded by is_any_int() / is_uint32_as_any_int()).

How did you verify your code works?

Added regression cases to the existing matcher tests:

  • test/js/bun/test/jest-extended.test.js (toBeArrayOfSize()): sparse arrays of length 3_000_000_000 and 2 ** 32 - 1.
  • test/js/bun/test/mock-fn.test.js (toHaveBeenCalledTimes): fn.mock.calls.length = 3_000_000_000.

Both tests panic on the released bun and pass with this change.

JS array length can be up to 2^32-1. Passing a sparse array like
new Array(3_000_000_000) to toBeArrayOfSize made i32::try_from(length)
fail and .unwrap() panic, crashing the test process.

Compare the length and expected size as i64 instead. get_length()
already clamps to the i52 range so the u64 -> i64 cast is lossless.
@coderabbitai

coderabbitai Bot commented Jun 15, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 10 minutes and 54 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3cf4e790-3c8b-4400-a289-7d327627590c

📥 Commits

Reviewing files that changed from the base of the PR and between e0acad3 and 1519b5c.

📒 Files selected for processing (4)
  • src/runtime/test_runner/expect/toBeArrayOfSize.rs
  • src/runtime/test_runner/expect/toHaveBeenCalledTimes.rs
  • test/js/bun/test/jest-extended.test.js
  • test/js/bun/test/mock-fn.test.js

Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 15, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:10 AM PT - Jun 15th, 2026

❌ @robobun, your commit 1519b5c has 1 failures in Build #62392 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32266

That installs a local version of the PR into your bun-32266 executable, so you can run:

bun-32266 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — one-line widening from i32 to i64 with regression tests; the inline note about toHaveBeenCalledTimes is a nice-to-have same-pattern follow-up, not a blocker.

Extended reasoning...

Overview

This PR changes a single line in src/runtime/test_runner/expect/toBeArrayOfSize.rs, replacing i32::try_from(value.get_length(global)?).unwrap() == size.to_int32() with value.get_length(global)? as i64 == size.to_int64(). It also adds three regression assertions to the existing toBeArrayOfSize() block in test/js/bun/test/jest-extended.test.js exercising sparse arrays with lengths above i32::MAX and at 2**32 - 1.

Correctness

The fix is straightforward and clearly correct: get_length() returns a u64 clamped to the i52 range, so the as i64 cast is lossless; size is already guarded by is_any_int() so to_int64() is the right coercion. This strictly widens the comparison domain — every input that passed before still passes, and the panic path for lengths in (i32::MAX, 2^32) is eliminated. The new tests directly cover the previously-panicking case and the upper bound.

Security risks

None. This is a numeric comparison in a test-runner matcher; there's no I/O, auth, allocation, or untrusted-input surface beyond what already existed.

Level of scrutiny

Low. One-line logic fix in a jest-extended matcher, with a clear repro, clear root cause, and regression tests that fail before / pass after. No CODEOWNERS cover these paths.

Other factors

The bug-hunter inline comment points out that toHaveBeenCalledTimes.rs:28 has the identical i32::try_from(...get_length()).unwrap() pattern. I verified it's still present. That said, triggering it requires manually assigning fn.mock.calls.length = 3e9 — far more contrived than new Array(3e9) — and it's pre-existing, not introduced here. It's a reasonable same-class follow-up the author can fold in or defer, but it doesn't change the correctness of this PR.

Comment thread src/runtime/test_runner/expect/toBeArrayOfSize.rs
fn.mock.calls is a mutable JSArray whose length can be set up to
2^32-1 from userland, so the i32::try_from(calls.get_length()).unwrap()
pattern here hits the same TryFromIntError panic as toBeArrayOfSize.
@robobun robobun changed the title expect: fix panic in toBeArrayOfSize with array length > i32 max expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max Jun 15, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — thanks for applying the same widening to toHaveBeenCalledTimes.

Extended reasoning...

Overview

This PR fixes a process-crashing panic in two expect matchers — toBeArrayOfSize and toHaveBeenCalledTimes — by widening the array-length comparison from i32 to i64. Each is a one-line change replacing i32::try_from(get_length()).unwrap() == x.to_int32() with get_length() as i64 == x.to_int64(). Regression tests are added to jest-extended.test.js and mock-fn.test.js covering lengths beyond i32::MAX (3e9 and 2**32-1).

Security risks

None. This is test-runner matcher code with no auth, crypto, filesystem, or network surface. The change strictly removes an .unwrap() panic path; no new untrusted input is introduced.

Level of scrutiny

Low. The diff is two mechanical one-liners in non-critical test-infrastructure code. get_length() returns a u64 clamped to the i52 range, so the as i64 cast is lossless. to_int64() is infallible and both call sites already guard the argument with is_any_int() / is_uint32_as_any_int(), so the conversion is exact. The times variable in toHaveBeenCalledTimes is still used in the failure-message format_args!, and i64: Display covers that unchanged. A grep confirms no remaining i32::try_from(...get_length...) sites in the expect matchers.

Other factors

My earlier review asked for the sibling toHaveBeenCalledTimes fix; the author applied it in ef31006 along with a regression test, and the thread is resolved. The bug-hunting system found no issues on the updated revision. The added tests use sparse arrays (new Array(3e9)) so they're cheap and don't bloat the suite.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the toHaveBeenCalledTimes panic is now fixed alongside toBeArrayOfSize, both with regression tests; the remaining toHaveReturnedTimes note is a non-panicking nit and fine as a follow-up.

Extended reasoning...

Overview

This PR replaces two i32::try_from(….get_length(global)?).unwrap() narrowings with as i64 comparisons in the test-runner matchers toBeArrayOfSize and toHaveBeenCalledTimes, and switches the expected-count argument from coerce::<i32> / to_int32() to to_int64(). Regression cases are added to test/js/bun/test/jest-extended.test.js and test/js/bun/test/mock-fn.test.js covering array lengths of 3_000_000_000 and 2**32-1. Net diff is 4 lines of Rust and ~12 lines of test additions.

Correctness

get_length() returns a u64 already clamped to [0, 2^51-1] (verified at src/jsc/JSValue.rs:1404-1413), so the u64 → i64 cast is lossless. to_int64() is infallible and handles the double-encoded 3e9 case via saturating truncation (src/jsc/JSValue.rs:766-777); the arguments are already guarded by is_any_int() / is_uint32_as_any_int() so no behavior changes for valid inputs in the i32 range. The previous review's blocking concern (the identical panic in toHaveBeenCalledTimes) was addressed in ef31006 with a regression test, and that thread is resolved.

Security risks

None. This is purely an integer-width fix in test-assertion matchers; no parsing, auth, FS, or network surface is touched.

Level of scrutiny

Low. The change is mechanical (widen i32 → i64), localized to two matcher files, removes an .unwrap() panic site, and is covered by new tests that fail on the prior build. No CODEOWNERS paths are touched.

Other factors

The one open finding — toHaveReturnedTimes still using is_uint32_as_any_int() + coerce::<i32> — is a wrap-around (wrong comparison result with a negative number printed), not a panic, and only reachable with a contrived expected count > 2^31. It's flagged as a 🟡 nit and explicitly noted as fine for a follow-up; it does not block this fix.

Comment thread src/runtime/test_runner/expect/toHaveBeenCalledTimes.rs
@robobun

robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

The diff is ready. Both regression tests (test/js/bun/test/jest-extended.test.js and test/js/bun/test/mock-fn.test.js) pass locally and did not fail in CI.

The remaining CI failures are unrelated flake on lanes this change does not touch:

  • build 62374: streams-leak.test.ts, html-rewriter-leak.test.ts, napi.test.ts, bun-install.test.ts, update_interactive_install.test.ts
  • build 62392 (retrigger): hot.test.ts, streams-leak.test.ts (all warning style, passed on retry)

None of these exercise expect matchers. Needs a maintainer to merge.

@Jarred-Sumner
Jarred-Sumner merged commit adbaf41 into main Jul 16, 2026
77 of 78 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/3ed2d39f/fix-tobearrayofsize-large-array branch July 16, 2026 07:38
hughescr added a commit to hughescr/bun that referenced this pull request Jul 16, 2026
* upstream/main: (57 commits)
  node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488)
  expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266)
  lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253)
  Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289)
  test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297)
  worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278)
  buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273)
  fs.promises.watch: yield events with a null prototype (oven-sh#34279)
  child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268)
  Fix asString assertion when passing String objects as signals (oven-sh#34265)
  Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274)
  test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294)
  tty: track raw mode per handle instead of per process (oven-sh#33527)
  test: expect the bumped mimalloc SHA in process.versions
  Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181)
  Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131)
  test: update block-scoped enum lowering expectations to let (oven-sh#34287)
  Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259)
  js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246)
  js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245)
  ...
hughescr added a commit to hughescr/bun that referenced this pull request Jul 16, 2026
* upstream/main: (70 commits)
  node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488)
  expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266)
  lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253)
  Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289)
  test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297)
  worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278)
  buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273)
  fs.promises.watch: yield events with a null prototype (oven-sh#34279)
  child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268)
  Fix asString assertion when passing String objects as signals (oven-sh#34265)
  Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274)
  test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294)
  tty: track raw mode per handle instead of per process (oven-sh#33527)
  test: expect the bumped mimalloc SHA in process.versions
  Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181)
  Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131)
  test: update block-scoped enum lowering expectations to let (oven-sh#34287)
  Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259)
  js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246)
  js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245)
  ...
hughescr added a commit to hughescr/bun that referenced this pull request Jul 16, 2026
* upstream/main: (52 commits)
  node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488)
  expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266)
  lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253)
  Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289)
  test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297)
  worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278)
  buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273)
  fs.promises.watch: yield events with a null prototype (oven-sh#34279)
  child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268)
  Fix asString assertion when passing String objects as signals (oven-sh#34265)
  Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274)
  test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294)
  tty: track raw mode per handle instead of per process (oven-sh#33527)
  test: expect the bumped mimalloc SHA in process.versions
  Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181)
  Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131)
  test: update block-scoped enum lowering expectations to let (oven-sh#34287)
  Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259)
  js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246)
  js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245)
  ...
hughescr added a commit to hughescr/bun that referenced this pull request Jul 16, 2026
* upstream/main: (52 commits)
  node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488)
  expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266)
  lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253)
  Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289)
  test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297)
  worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278)
  buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273)
  fs.promises.watch: yield events with a null prototype (oven-sh#34279)
  child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268)
  Fix asString assertion when passing String objects as signals (oven-sh#34265)
  Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274)
  test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294)
  tty: track raw mode per handle instead of per process (oven-sh#33527)
  test: expect the bumped mimalloc SHA in process.versions
  Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181)
  Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131)
  test: update block-scoped enum lowering expectations to let (oven-sh#34287)
  Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259)
  js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246)
  js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245)
  ...

# Conflicts:
#	test/js/bun/websocket/websocket-server.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants