Repository navigation
Conversation
WalkthroughAdds thenable and callable support to Changesresolves/rejects thenable & callable support
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:10 PM PT - Jun 28th, 2026
✅ @robobun, your commit a8d4edf22c62d858fd8b2f24bd318e0b6f52d58b passed in 🧪 To try this PR locally: bunx bun-pr 32944That installs a local version of the PR into your bun-32944 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
I didn't find any bugs and the thenable/function handling looks correct and well-tested, but since this changes .resolves/.rejects semantics for every bun:test user and also shrinks an unrelated stress test's iteration counts, it's worth a quick human look.
Extended reasoning...
Overview
The PR modifies Expect::process_promise in src/runtime/test_runner/expect.rs (~25 lines) to accept thenables (objects with a callable .then) for both .resolves and .rejects, and to call the received value first when .rejects is given a function — matching Jest's makeRejectMatcher contract. It wraps thenables via JSPromise::create + spec resolve() so the existing wait_for_promise path works unchanged, and updates the .rejects usage error wording. Two new test blocks in expect.test.js cover sync/async-settling thenables, delegating thenables, functions returning promises/thenables, async-throwing functions, and the negative cases. Separately, it shrinks the deepEquals Set/Map stress test iteration counts (150→25 elements, 2000→500 / 1000→250 iterations) to keep it under timeout on debug+ASAN.
Security risks
None. This is test-runner assertion plumbing; the only new user-controlled call is invoking the function passed to expect(fn).rejects, which is the user's own test code.
Level of scrutiny
Moderate. The Rust change is small, clearly commented, and the GC-rooting note about awaited staying on the stack across wait_for_promise is correct. The ? on value.call(...) means a synchronously throwing function propagates rather than being treated as a rejection — but that matches Jest's typeof actual === 'function' ? actual() : actual (no try/catch), so parity holds. Tests are thorough and run under Jest/Vitest as a parity check. However, this is a user-visible behavior change to a core bun:test API used across the entire ecosystem, which I'd rather not rubber-stamp.
Other factors
The stress-test reduction is orthogonal to the fix. The justification (190s under debug+ASAN vs a 5s default timeout) is reasonable and the reduced counts still exercise thousands of iterator allocations for #14256, but weakening a regression stress test is the kind of side change a maintainer should explicitly ack rather than have slip in alongside a feature. No CODEOWNERS coverage on either file; no prior human review yet.
|
Status, for whoever picks this up. The diff is complete and, as far as I can tell, merge-ready. What it fixes: The one semantic question, settled: an earlier revision called a received function for Review: all review threads are resolved; the last three reviewer passes found nothing further and only asked for a human sign-off on the semantics, which the paragraph above is meant to answer. CI (build 66461, commit a8d4edf): 284 jobs passed, 0 failed. The only jobs not finished are two |
There was a problem hiding this comment.
Thanks for addressing all three points in eeb0f4b — the settled-value formatting, the RETURN_IF_EXCEPTION guard, and the comment trim all look right. I'm deferring final sign-off since this is a behavioral change to process_promise that runs user JS across the Rust↔JSC boundary (and bundles the unrelated stress-test shrink), so a maintainer should give it a look.
Extended reasoning...
Overview
This PR widens expect().resolves/.rejects in bun:test to accept thenables (objects with a callable .then) and, for .rejects, functions returning a promise — matching Jest/Vitest semantics. It touches three files: src/runtime/test_runner/expect.rs (~25 new lines in process_promise plus two error-message fixes), src/jsc/bindings/bindings.cpp (one RETURN_IF_EXCEPTION guard in matchAsymmetricMatcher), and test/js/bun/test/expect.test.js (~120 new test lines plus a workload reduction in an unrelated stress test).
Review history
My earlier review raised three issues: (1) the wrong-settlement error messages formatted the original value instead of the settled new_value, (2) the new user-JS calls on the silent asymmetric-matcher path could leave a pending VM exception that the C++ side would flip FAIL→PASS under .not, and (3) a 5-line comment exceeded the repo's 3-line cap. Commit eeb0f4b addresses all three — both error sites now format new_value, matchAsymmetricMatcher checks for a pending exception before applying the NOT flip, the comment is condensed to three lines, and regression tests were added for (1) and (2). The current bug-hunting pass found nothing further.
Security risks
None. This is test-runner matcher logic; no auth, crypto, network, filesystem, or untrusted-input parsing is involved. The new code runs user-supplied JS (calling a received function, reading a .then property), but that's the explicit contract of these matchers and is already exercised by the existing promise-await path.
Level of scrutiny
Medium-high. The change is conceptually small but sits on a subtle boundary: it introduces new points where user JS executes inside process_promise (including on the silent=true asymmetric-matcher path used from inside deepEquals), wraps thenables via JSPromise::create + spec resolve(), and relies on the wrapper staying rooted across wait_for_promise. The first revision had two real (if minor) issues at this boundary, which is evidence the area deserves a maintainer's eyes rather than bot-only approval.
Other factors
The PR also bundles an unrelated change — shrinking the deepEquals Set/Map stress test workload so the file passes under debug+ASAN — which the author has explicitly offered to split out. That's a maintainer call. Test coverage for the new behavior is thorough (sync/async thenables, delegation to real promises, .not, functions returning promises/thenables, negative cases, and the asymmetric resolvesTo/rejectsTo matchers). Given the cross-language scope and the bundled unrelated change, deferring rather than approving.
Jest accepts any object with a callable .then for .resolves/.rejects, and for .rejects it also accepts a function returning a promise. Bun only accepted real Promise instances, so awaiting a query builder or ORM result with expect(...).resolves failed with "Expected promise". In process_promise, wrap a thenable in a promise via the spec resolve algorithm so wait_for_promise can await it, and for .rejects call the received function first. The error message for .rejects now mentions functions, matching Jest. Also shrink the deepEquals Set/Map stress test so this file's other tests stay within the default per-test timeout on a debug + ASAN build; the workload it exercises (thousands of Set/Map iterator allocations, #14256) is unchanged. Fixes #18857
The asymmetric matchers route through the same process_promise helper, so they were also rejecting non-Promise thenables. The fix already covers them; add the missing assertions.
…r exceptions The wrong-settlement errors in process_promise formatted the original received value; now that functions and bare thenables reach those branches that printed [Function] instead of what the promise settled to. Format new_value there; the usage error still reports the original. matchAsymmetricMatcherAndGetFlags can now run user JS (a .rejects function, a then getter), so matchAsymmetricMatcher checks the exception before flipping the result for .not, matching the RETURN_IF_EXCEPTION sites inside it. Also condense the stress-test comment to three lines. New tests: the settled value appears in the settlement error for both .resolves and .rejects, a synchronously throwing function given to .rejects propagates its error, and rejectsTo given a throwing function propagates that error with and without .not.
main now gives that test an explicit 480s timeout, so the shrink this PR carried to keep the file runnable under debug+ASAN is obsolete, and it was out of scope here.
eeb0f4b to
d32d76a
Compare
There was a problem hiding this comment.
Thanks — all three of my earlier points are addressed (settled-value in the error messages, the RETURN_IF_EXCEPTION chokepoint, and the unrelated stress-test change is now dropped). No further issues from me; leaving the final call to a maintainer since this changes .resolves/.rejects acceptance semantics in the core expect path.
Extended reasoning...
Overview
This PR widens expect(...).resolves / .rejects in bun:test to accept thenables (objects with a callable then) and, for .rejects only, a function returning a promise — matching Jest 30 / Vitest behavior. It touches three files: src/runtime/test_runner/expect.rs (the process_promise helper, ~25 lines of new logic plus two error-message fixes), src/jsc/bindings/bindings.cpp (one RETURN_IF_EXCEPTION line in matchAsymmetricMatcher), and test/js/bun/test/expect.test.js (~110 lines of new tests). The change is shared by all four entry points that use this helper: .resolves, .rejects, custom matchers, and the expect.resolvesTo/rejectsTo asymmetric matchers.
Prior review status
I left three inline comments on an earlier revision; all are now resolved in the current diff:
- The wrong-settlement error messages now format
new_value(the settled value) instead of the original received function/thenable, with a covering test. - A
RETURN_IF_EXCEPTIONwas added at the single chokepoint inmatchAsymmetricMatcherbefore the.notflip, so a pending exception from the new user-JS calls (function call,.thengetter, spec resolve) propagates instead of being flipped to PASS. The author was candid that this is defense-in-depth — the outer Rust host-call wrapper already re-raises pending exceptions — and added a test for the sync-throw-under-rejectsTocase. - The unrelated
deepEquals Set/Map stress testworkload shrink (and its 5-line comment) was dropped entirely in d32d76a, so the PR is now scoped purely to the resolves/rejects change.
The bug-hunting pass on the current revision found nothing.
Security risks
None identified. This is test-runner assertion machinery; it does not touch auth, crypto, filesystem, or network code. The new user-JS execution (calling a received function, reading a .then property, invoking the spec resolve algorithm) only runs on values the test author themselves passed to expect(), in their own test process.
Level of scrutiny
Medium-high. The diff is small and the intent is clear, but it changes acceptance semantics in the core expect path that every bun:test user goes through, and it introduces user-JS execution (with attendant exception-propagation and GC-rooting concerns — the awaited stack-rooting comment is one example) into a path that was previously a pure type check. The Rust↔C++ FFI exception contract was subtle enough that my earlier review caught a hole in it. That argues for a maintainer sanity-checking the promise-wrapping and rooting rather than an auto-approve.
Other factors
Test coverage is thorough: sync and async-settling thenables, thenables delegating to real promises, .not, functions returning rejected promises, async functions that throw, functions returning thenables, and the negative cases (non-thenable objects, functions returning non-promises, .resolves not calling functions, wrong-direction settlement, sync throws). The asymmetric-matcher tests gained thenable cases too. The file is shared with Jest/Vitest, so the positive cases double as a parity check. None of the modified files are covered by CODEOWNERS. The robobun CI comment references failures in Build #66047, which a human reviewer may want to glance at before merging.
There was a problem hiding this comment.
Thanks — all three points from my earlier pass are addressed and I have nothing further. Still flagging for a human look given this touches the core .resolves/.rejects promise path (spec-resolve wrapping, GC rooting across wait_for_promise, FFI exception propagation), which is more than I'd want to wave through on my own.
Extended reasoning...
Overview
This PR widens expect().resolves / expect().rejects in bun:test to accept thenables (objects with a callable .then) and, for .rejects, functions returning a promise — matching Jest 30 / Vitest. It touches three files: src/runtime/test_runner/expect.rs (~30 lines in process_promise plus two error-message tweaks), src/jsc/bindings/bindings.cpp (one RETURN_IF_EXCEPTION chokepoint in matchAsymmetricMatcher), and ~110 lines of new tests in test/js/bun/test/expect.test.js.
I left three comments on an earlier revision (settlement-error messages should print new_value, the silent asymmetric-matcher path could leave a pending VM exception across the .not flip, and an out-of-scope stress-test edit). All three are resolved in the current diff: the settlement errors format new_value with a regression test, matchAsymmetricMatcher now does RETURN_IF_EXCEPTION before the flip with a covering test, and the stress-test change was dropped entirely after main gave that test its own timeout. The bug-hunting system found nothing new on this revision.
Security risks
None. This is test-runner matcher logic; no auth, crypto, network, or filesystem surface. The new code paths only run user-supplied values that were already in scope.
Level of scrutiny
Moderate-to-high. The change is purely additive (only inputs that previously hard-errored now succeed, real Promises take the unchanged fast path), well-described, and thoroughly tested across positive/negative/asymmetric-matcher cases. But it sits in non-trivial territory: it constructs a JSPromise wrapper and relies on stack rooting across wait_for_promise, runs user JS (value.call, .then getter, spec resolve) on a path that was previously inert, and adjusts exception propagation at the Rust↔C++ FFI boundary. That's the kind of code where a maintainer who knows the JSC binding conventions should sign off.
Other factors
CI failures on the latest build (terminal.test.ts, sql-mysql.auth.test.ts, bake/deinitialization.test.ts) are unrelated to this change. No CODEOWNERS entry covers these files. The author's responses to review were thorough and candid (including noting the C++ RETURN_IF_EXCEPTION is defense-in-depth with no demonstrable behavior change today). I'm satisfied with the current state; deferring only because the JSC promise/FFI surface warrants a human reviewer rather than because anything looks wrong.
Jest 30's makeResolveMatcher calls a callable received value the same way makeRejectMatcher does (verified against jest 30.4.1, not its source); only the older implementation was rejects-only. Both matchers use the same 'a promise or a function returning a promise' usage error. This is also the .resolves half of #16144, which the .rejects-only version left unaddressed.
There was a problem hiding this comment.
All three earlier comments are addressed and I found nothing new, but this is core test-runner Rust/C++ that now runs user JS in new places, and the most recent commit (a8d4edf) extended .resolves to also call functions — which the PR body still says was intentionally left out — so a maintainer sign-off on the final semantics is worth having.
Extended reasoning...
Overview
The PR widens expect().resolves / .rejects (and the expect.resolvesTo / expect.rejectsTo asymmetric matchers that share the same helper) to accept thenables and callables, matching Jest/Vitest. Touched: src/runtime/test_runner/expect.rs (process_promise now calls a callable received value, wraps a thenable via JSPromise::create + spec resolve(), and reports the settled new_value in wrong-settlement errors), src/jsc/bindings/bindings.cpp (one RETURN_IF_EXCEPTION before the .not flip in matchAsymmetricMatcher), and ~120 lines of new tests in test/js/bun/test/expect.test.js.
Security risks
None apparent. This is test-runner machinery; the new user-JS calls (value.call(...), the .then get, wrapper.resolve(...)) execute code the test author already controls and propagate exceptions via ?. No untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Medium-high. process_promise sits on the path of every .resolves/.rejects assertion in bun:test, and the change crosses the Rust↔C++ FFI boundary with promise creation, microtask draining via the existing wait_for_promise, and exception state. The author was careful (the awaited binding is kept on the stack to root the wrapper across the wait, and the C++ RETURN_IF_EXCEPTION was added at the chokepoint), and the bug-hunting system found nothing on the current revision. Still, this is runtime logic rather than a mechanical change, so I'm not auto-approving.
Other factors
- All three of my earlier inline comments (settled-value in error messages,
RETURN_IF_EXCEPTIONbefore the.notflip, and the unrelated stress-test edit) were addressed in 50f4540 / d32d76a and are resolved. - The latest commit a8d4edf (after my prior review) makes
.resolvesalso call a callable received value, with the commit message citing empirical verification against Jest 30.4.1. The PR description still says the opposite (".resolves continues to require a promise or thenable" / "left out on purpose"), so the body should be synced, and a maintainer should confirm they want the.resolves-calls-functions semantics — it's reasonable, but it's a user-facing API contract. - No CODEOWNERS cover these paths. CI: per the author's status comment, all assigned lanes (incl. x64-asan) are green; only darwin-14 was still queued at last update.
|
Still reproduces on main as of f4f3844: The branch has fallen behind main and now conflicts, limited to src/runtime/test_runner/expect.rs; bindings.cpp and the test file still merge cleanly. Needs a rebase before it can land. CI was fully green on the last push (build #66461, Jun 28). |
|
#40996 now covers the plain thenable part of this PR: .resolves and .rejects accept any object with a callable then and settle it through that then. What remains unique here is calling a function passed to .rejects (#16144) and the message text. If that part should land, this branch needs a rebase on top of #40996 and a decision on whether .resolves should call a function too (Jest only does it for .rejects). |
|
Closing: superseded by #40996 and #41069. #40996 settles a plain thenable through its own |
Fixes #18857
Fixes #16144
Reproduction
All three fail on current bun:
All three pass on jest 30.4.1.
Cause
Expect::process_promiseinsrc/runtime/test_runner/expect.rsonly accepted values for whichas_any_promise()returns a realJSPromise/JSInternalPromise. Jest's contract for.resolves/.rejectsis any thenable (an object with a callablethen), and both matchers first call the received value when it is a function and await what it returns. Thenables are common in practice: knex and mongoose query builders, and many ORM and lazy-client APIs, return then-able objects rather thanPromiseinstances.The function-calling behavior was checked against real jest 30.4.1 rather than read from its source: jest's older implementation was
.rejects-only, but jest 30 calls a callable receiver for.resolvesas well, and uses the same "a promise or a function returning a promise" usage error for both. An earlier revision of this PR gated the call on.rejects.Fix
In
process_promise:.resolvesand.rejects.then, wrap it viaJSPromise::createplus the specresolve()algorithm (the same semantics asawait thenable) so the existingwait_for_promisepath can await it. The wrong-settlement errors report the settled value rather than the promise object.One line in
src/jsc/bindings/bindings.cpp:matchAsymmetricMatchernow checks for a pending exception aftermatchAsymmetricMatcherAndGetFlags. The widenedprocess_promisecan run user JS (athengetter, a received function) from inside asymmetric matching, and without the check a thrown exception was flipped into a pass/fail result by the.nothandling instead of propagating.All four entry points that share this helper pick up the change:
.resolves,.rejects, the custom-matcher path, and theexpect.resolvesTo/expect.rejectsToasymmetric matchers.Tests
resolves accepts a thenable and a function returning a promiseandrejects accepts a thenable and a function returning a promiseintest/js/bun/test/expect.test.js. They cover: sync and async-settling thenables, a thenable delegating to a real promise,.not, functions returning a promise / a thenable, an async function that resolves, an async function that throws, and the negative cases (a non-thenable object, an object with a non-callablethen, a function returning a non-promise, a synchronously throwing function propagating its own error, and a thenable settled the wrong way still failing). A third test asserts the wrong-settlement errors show the settled value.The existing
expect.resolvesToandexpect.rejectsToasymmetric-matcher tests gain thenable cases (top-level and nested intoEqual), since those matchers share the same helper and were failing on thenables the same way.expect.test.jsalso runs under jest and vitest, so the positive assertions double as a parity check. Every assertion outside anisBunguard was additionally run under jest 30.4.1 directly and passes there.All of the new assertions fail on bun without the
src/change and pass with it; the fullexpect.test.jssuite (420 tests) is green with the change.