bun:test: stop treating a returned Error as thrown in toThrow by robobun · Pull Request #34392 · oven-sh/bun

robobun · 2026-07-16T18:27:21Z

What does this PR do?

expect(fn).toThrow() passed when fn returned an Error instead of throwing it.

test("returned error treated as thrown", () => {
  expect(() => new TypeError("boom")).toThrow();             // passes, should fail
  expect(() => new TypeError("boom")).toThrow(TypeError);    // passes, should fail
  expect(() => new TypeError("boom")).not.toThrow();         // fails, should pass
});

Jest 30 and Vitest 4 both fail the first two with "Received function did not throw" and pass the third. Bun passed all three toThrow() forms (no-arg, string, regexp, constructor, Error instance, asymmetric matcher) and failed .not.toThrow(), so a test written to assert "this API throws rather than returning an Error" could not fail for exactly the bug class it was written to catch.

This surfaced while writing regression tests for #33522 (the DH constructor used to return a created error instead of throwing; a toThrow-based test passed on both the fixed and the unfixed binary) and affects the tests in #31708 for the same reason.

Cause

Expect::get_value_as_to_throw in src/runtime/test_runner/expect.rs stores the call result in one variable regardless of whether the call threw or returned normally, then runs .to_error() on it. JSValue::to_error() yields Some for any ErrorInstance, so a returned Error was indistinguishable from a thrown one.

Fix

Record whether the call threw synchronously (and whether an unhandled rejection was captured during it). When neither happened and the return value is not a Promise, report "did not throw" regardless of the return value's type. The Promise-returning path (expect(async () => { throw e }).toThrow(), a deliberate Bun extension used by ~30 existing tests) and the .resolves/.rejects path are untouched. toThrowErrorMatchingSnapshot and toThrowErrorMatchingInlineSnapshot share the same helper and are fixed by the same change.

Why this is correct

Jest's toThrow sets thrown only inside the catch block of its try { received() } catch (e) { ... }; the return value is never consulted. Vitest's toThrow (chai-based) does the same. The new test was run against Jest 30.4.1 and Vitest 4.1.10 and passes on both.

How did you verify your code works?

Added toThrow does not treat a returned Error as thrown to test/js/bun/test/expect.test.js covering every expected-value shape (no-arg, string, regexp, constructor, Error instance, expect.any, expect.objectContaining), the two snapshot matchers, .not.toThrow(), Error subclasses, a plain {message} object, throwing non-Error values, and the Bun-only async extension. The non-Bun assertions were run green against Jest 30.4.1 and Vitest 4.1.10.

$ bun bd test test/js/bun/test/expect.test.js -t toThrow
(pass) expect() > toThrow asymmetric matchers
(pass) expect() > toThrow
(pass) expect() > toThrow does not treat a returned Error as thrown
(pass) expect() > toThrow to return undefined
(pass) expect() > toThrowErrorMatchingInlineSnapshot to return undefined
(pass) expect() > toThrowErrorMatchingSnapshot to return undefined
 6 pass  0 fail

Fails on unfixed bun (USE_SYSTEM_BUN=1), passes with this change. The full expect.test.js is 407 pass / 10 todo / 0 fail, clean under BUN_JSC_validateExceptionChecks=1. Grepped the test suite for arrow functions that return an Error into .toThrow(); none exist, so no existing tests rely on the old behavior.

test/js/bun/ffi/ffi-viewSource-non-object.test.ts was written as .toThrow("Expected an object") but Bun.FFI.viewSource actually returns the TypeError rather than throwing it, so that assertion only passed because of this bug. Rewrote it to assert on the returned value so the crash regression it covers is still caught; making viewSource throw is tracked separately.

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi-viewSource-non-object.test.ts

Expect.get_value_as_to_throw folded a synchronous throw and a normal return into the same variable, then called to_error() on the result. When the function body was `() => new TypeError(...)` the return value is an ErrorInstance, to_error() returned Some, and toThrow() passed even though nothing was thrown. Jest and Vitest both fail here with "Received function did not throw". Track whether the call actually threw (or a rejection was captured) and report "did not throw" for every non-Promise normal return, regardless of the return value's type. The Promise-returning path (Bun's expect(async () => { throw e }).toThrow() extension) and the .resolves/.rejects path are unchanged.

robobun · 2026-07-16T18:27:32Z

^{Updated 1:32 PM PT - Jul 16th, 2026}

❌ @robobun, your commit 4b6222b has 2 failures in Build #74033 (All Failures):

test/cli/run/require-cache.test.ts - code 1 on 🍎 14 aarch64
test/cli/run/no-orphans.test.ts - code 1 on 🍎 26 aarch64

🧪 To try this PR locally:

bunx bun-pr 34392

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

bun-34392 --bun

coderabbitai · 2026-07-16T18:27:46Z

Walkthrough

Changes

toThrow now distinguishes synchronous exceptions and captured promise rejections from functions that return error-like values. Tests cover returned errors, actual throws, matcher variants, snapshots, async cases, and FFI error results.

toThrow error distinction

Layer / File(s)	Summary
Invocation state and early return `src/runtime/test_runner/expect.rs`	Tracks synchronous throws and captured rejections separately, returning ordinary function results without entering promise handling.
Returned versus thrown error tests `test/js/bun/test/expect.test.js`	Verifies returned errors fail `toThrow`, while thrown errors, non-`Error` values, matcher variants, snapshots, and promise cases remain supported.
FFI error result expectations `test/js/bun/ffi/ffi-viewSource-non-object.test.ts`	Verifies non-object FFI descriptors return a `TypeError` containing “Expected an object” instead of throwing.

Possibly related PRs

oven-sh/bun#34396: Changes bun:ffi viewSource throwing behavior and updates the same non-object descriptor test.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and specifically describes the main change to bun:test to reject returned Errors in toThrow.
Description check	✅ Passed	The description includes both required sections and gives a detailed summary plus verification steps.

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

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/expect.rs`:
- Around line 845-849: In the rejection-capture flow around
handle_rejected_promises(), move restoration of
vm.unhandled_pending_rejection_to_capture until after rejected promises are
drained and captured_rejection is computed. Keep the capture slot pointed at
return_value while handle_rejected_promises() runs, then restore
prev_unhandled_pending_rejection_to_capture.

🪄 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: a450ef5c-9a28-4a59-a659-b8a597dea1fb

📥 Commits

Reviewing files that changed from the base of the PR and between 6ad467f and b52f688.

📒 Files selected for processing (2)

src/runtime/test_runner/expect.rs
test/js/bun/test/expect.test.js

The test relied on toThrow() accepting a returned Error; assert on the returned value instead so the crash regression it covers is still caught.

claude

LGTM — the earlier comment-length nit was addressed in 4b6222b.

Checked that take_exception returns the JSC::Exception wrapper, so throw 42 still yields Some from to_error() via the Exception cell-type branch and did_throw stays true for non-Error throws. Traced both callers of get_value_as_to_throw (toThrow.rs and the two snapshot matchers via fn_to_err_string_or_undefined) — both treat None as "did not throw" and match the new test's failure-message regex. Confirmed the capture-pointer/drain ordering CodeRabbit flagged is unchanged from main and the returned-rejected-Promise case flows through the as_any_promise() branch instead. The FFI test rewrite still guards the original crash regression (asserts on the returned TypeError).

Extended reasoning...

Overview

Changes Expect::get_value_as_to_throw in src/runtime/test_runner/expect.rs (~10 lines) to record whether the received function actually threw (threw_sync) or captured an unhandled rejection (captured_rejection), and returns (None, ...) when neither happened and the return value is not a Promise. Adds a comprehensive test to test/js/bun/test/expect.test.js covering every toThrow argument shape, both snapshot matchers, .not, Error subclasses, plain {message} objects, throwing non-Error values, and the Bun-only async-function extension. Rewrites test/js/bun/ffi/ffi-viewSource-non-object.test.ts, the one existing test that only passed because of the bug being fixed.

Security risks

None. This is test-runner matcher logic; no I/O, auth, parsing of untrusted input, or FFI surface changes.

Level of scrutiny

Medium. toThrow is one of the most-used matchers, so a semantic change here touches a lot of surface area. But the change is (a) small and easy to trace, (b) a correction toward Jest 30 / Vitest 4 semantics (author verified against both), (c) only affects the pathological case where a function returns an Error rather than throwing — which no correct test should rely on. I traced the control flow: take_exception returns the JSC::Exception cell (bindings.cpp:6010), so to_error() on it hits the inherits<JSC::Exception> branch (bindings.cpp:4668) and returns Some(exception->value()) — meaning throw <non-Error> still produces did_throw = true. Returned Promises exit early via as_any_promise() before reaching the new guard. The captured_rejection check preserves the pre-existing capture-slot path unchanged.

Other factors

The new test is thorough and was cross-checked against Jest 30.4.1 and Vitest 4.1.10 per the PR description; full expect.test.js reported green under BUN_JSC_validateExceptionChecks=1.
My earlier inline nit about the 6-line comment in the FFI test was addressed in 4b6222b — the comment is now 2 lines and drops the PR-history rationale.
CodeRabbit's ordering concern was correctly rebutted (the restore-before-drain sequence is unchanged from main; returned rejected Promises use the as_any_promise() path, not the capture slot) and withdrawn.
The FFI test rewrite still exercises the original crash repro (calling viewSource with non-object descriptors) and asserts on the returned TypeError, so the regression guard is preserved.

robobun · 2026-07-16T19:55:11Z

CI status on 4b6222b (build #74033): the remaining failures are all unrelated to this diff.

test/cli/run/no-orphans.test.ts timed out on darwin-aarch64 only (process-orphan reaping, does not use toThrow; already tracked separately).
The rest are marked flaky: password.test.ts (RSS leak threshold on windows-x64-baseline), spawn.test.ts (timeout on windows-x64-baseline), in-process-cron.test.ts (hot-reload timeout on debian-x64-asan), napi.test.ts, webview-chrome.test.ts, test-fs-promises-file-handle-readFile.js, bun-security-scanner-matrix-without-node-modules.test.ts, 20144.test.ts.

The ffi-viewSource-non-object.test.ts failure from the previous build is gone after 817aa2a. The files this PR touches (expect.test.js, ffi-viewSource-non-object.test.ts) pass on every lane. Ready to merge.

…ad of returning them (#34396) ## What does this PR do? `viewSource` from `bun:ffi` returned a `TypeError` instead of throwing it when the descriptor was invalid: ```js import { viewSource } from "bun:ffi"; const result = viewSource({ myFunc: 42 }); result instanceof TypeError; // true, returned not thrown result.message; // 'Expected an object for key "myFunc"' ``` `JSCallback` has the same bug: its constructor destructured the returned error, so validation failures produced a useless instance instead of throwing: ```js import { JSCallback } from "bun:ffi"; const cb = new JSCallback(() => {}, 42); // options must be an object cb.ptr; // undefined, no error surfaced ``` Both happen for every validation failure the native implementation reports: non-object descriptors, unknown FFI type names, a non-object options argument, and (for `JSCallback`) a non-callable callback. ### Cause The native host functions on `Bun.FFI` return an `Error` instance as their value when validation fails; the `bun:ffi` module is expected to turn that into a throw. `dlopen`, `cc` and `linkSymbols` already do this with `if (Error.isError(result)) throw result;`, but `viewSource` was exported as a direct alias of the native function and `JSCallback` destructured the native result without checking it. ### Fix Wrap `viewSource` and guard the `JSCallback` constructor the same way as the three existing siblings. The declared return type of `viewSource` in `packages/bun-types/ffi.d.ts` is `string[] | string`, which this now matches. ### Why this is correct Matching `dlopen`/`cc`/`linkSymbols` keeps the `bun:ffi` module internally consistent, and it matches the typed contract. The low-level `Bun.FFI` object is left unchanged (it is not part of the typed public surface and `Bun.FFI.dlopen`/`Bun.FFI.linkSymbols` also return errors). ### How did you verify your code works? Rewrote `test/js/bun/ffi/ffi-viewSource-non-object.test.ts` to assert the error is actually thrown (the old assertions only passed because `expect().toThrow()` currently accepts a returned Error, which #34392 tightens). Added `test.each` cases for `viewSource` (non-object descriptor, unknown type, non-object options, callback descriptor, valid input) and `JSCallback` (non-object options, non-callable callback, unknown type, valid input). ``` $ bun bd test test/js/bun/ffi/ffi-viewSource-non-object.test.ts 24 pass 0 fail ``` 22 of the 24 cases fail on an unfixed build (the two valid-input cases pass as expected). The full `test/js/bun/ffi/` suite is green and clean under `BUN_JSC_validateExceptionChecks=1`. ### Related #34392 changes `toThrow()` to stop accepting a returned Error and rewrites this test to assert on the returned value as a stopgap. This PR is the actual fix; whichever lands second will need the test file reconciled. #33200 touches `FFI::print`/`FFI::callback` for a different bug (propagating exceptions from descriptor getters) and still returns the validation error as a value, so the wrappers here cover both before and after that change.  --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi-viewSource-non-object.test.ts

github-actions Bot added the claude label Jul 16, 2026

[autofix.ci] apply automated fixes

b52f688

coderabbitai Bot reviewed Jul 16, 2026

View reviewed changes

Comment thread src/runtime/test_runner/expect.rs

test: FFI.viewSource returns its validation TypeError, not throws it

817aa2a

The test relied on toThrow() accepting a returned Error; assert on the returned value instead so the crash regression it covers is still caught.

robobun mentioned this pull request Jul 16, 2026

bun:ffi: throw validation errors from viewSource and JSCallback instead of returning them #34396

Merged

claude Bot reviewed Jul 16, 2026

View reviewed changes

Comment thread test/js/bun/ffi/ffi-viewSource-non-object.test.ts Outdated

test: trim comment in ffi-viewSource-non-object

4b6222b

claude Bot reviewed Jul 16, 2026

View reviewed changes

Navigation Menu

Uh oh!

bun:test: stop treating a returned Error as thrown in toThrow - #34392

bun:test: stop treating a returned Error as thrown in toThrow#34392
robobun wants to merge 4 commits into
mainfrom
farm/30a09ec2/tothrow-returned-error

robobun commented Jul 16, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 16, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Cause

Fix

Why this is correct

How did you verify your code works?

Related

Uh oh!

robobun commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jul 16, 2026 •

edited

Loading

robobun commented Jul 16, 2026 •

edited

Loading

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading