[robobun]

Repro

test("looks async, never runs", () => {
  return {
    then(resolve, reject) {
      reject(new Error("must fail"));
    },
  };
});

bun test reports (pass), and then() is never called at all. node --test and jest fail this test. Any assertion, rejection, or async work inside the thenable silently never happens.

The same applies to hooks: a beforeEach returning a rejected thenable is ignored and the test runs anyway.

Cause

BunTest::run_test_callback only takes the async path when the callback's return value downcasts to a native JSPromise:

if let Some(promise) = result.as_promise() { ... }

A thenable is not a JSPromise, so it falls through to the "synchronous test finished" path and the value is dropped. Registering the runner's completion reactions on it is not possible either: JSC__JSValue___then is a no-op for anything that isn't a JSPromise.

Fix

Coerce the return value with the spec's Promise.resolve() when it is a thenable that isn't already a promise, then let the existing promise path await it. Non-thenables are returned unchanged, so a callback that returns a plain object (or takes a done parameter) keeps taking the synchronous path.

JSPromise::resolve runs ResolvePromise, so a then that throws, a then getter that throws, and a double-resolve are all handled per spec.

Verification

test/js/bun/test/thenable.test.ts spawns bun test on two fixtures:

• a resolving thenable returned from a test and from beforeEach must settle before the next test starts (ordering assertion, no timers), and a non-thenable object must still wait for done()
• a rejected thenable, a throwing then, a throwing then getter, and a rejected beforeEach thenable must each fail, and the hook's test must not run

Before the fix both tests fail (the fixture's 4 failures are reported as 4 pass, exit 0). After, both pass.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4b867072-3369-46bf-8b16-12c46526cc49

📥 Commits

Reviewing files that changed from the base of the PR and between 8eed39c and 7ec6067.

📒 Files selected for processing (2)

• src/runtime/test_runner/bun_test.rs
• test/js/bun/test/test-test.test.ts

Walkthrough

Adds a coerce_thenable helper in the Bun test runner that converts non-native thenable return values from test callbacks into native JSPromises, wires it into run_test_callback with error handling, and adds integration tests covering resolving and rejecting thenable scenarios.

Changes

Thenable Support in Bun Test Runner

Layer / File(s)	Summary
Thenable coercion helper and callback wiring src/runtime/test_runner/bun_test.rs	Adds coerce_thenable to detect and wrap thenable objects into native JSPromises, and integrates it into run_test_callback after the callback returns, with error handling that clears exceptions, reports uncaught exceptions, and substitutes JSValue::ZERO on coercion failure.
Thenable integration tests test/js/bun/test/thenable.test.ts	Adds two spawned bun test scenarios verifying resolving thenables are awaited without premature completion, and rejecting/throwing thenables (including hook rejections and then getter throws) cause failures with exit code 1 and expected stderr output.

Sequence Diagram(s)

sequenceDiagram
  participant TestCallback
  participant run_test_callback
  participant coerce_thenable
  participant JSPromise

  TestCallback->>run_test_callback: return value
  run_test_callback->>coerce_thenable: coerce(value)
  coerce_thenable->>coerce_thenable: check for callable then
  alt is thenable
    coerce_thenable->>JSPromise: create and resolve with value
    JSPromise-->>run_test_callback: native JSPromise
  else not thenable
    coerce_thenable-->>run_test_callback: original value
  end
  alt coercion throws
    run_test_callback->>run_test_callback: clear exception, on_uncaught_exception, log
    run_test_callback-->>run_test_callback: JSValue::ZERO
  end

Loading

Compact metadata: 4 ranges changed across 2 files (+174/-0 lines).

Related issues: None found in provided context.

Related PRs: None found in provided context.

Suggested labels: test-runner, bun-test

Suggested reviewers: None specified.

Poem

A rabbit hopped through Promise land,
Found thenables not quite as planned,
Coerced with care, wrapped tight and neat,
Now tests await what they should meet,
And errors caught with nimble paw. 🐇

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: awaiting thenables from test or hook callbacks.
Description check	✅ Passed	The description covers the change and verification details, though it uses different headings than the template.
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.

---

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

[robobun]

^{Updated 9:08 AM PT - Jul 5th, 2026}

❌ @robobun, your commit 7ec6067 has some failures in Build #68565 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 33372

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

bun-33372 --bun

[claude]

I didn't find any bugs, but this changes run_test_callback — the path every test/hook goes through — and adds an observable .then lookup on every returned object plus a new done+thenable interaction, so it's worth a human look before merging.

Extended reasoning...

Overview

This PR makes bun:test await userland thenables (PromiseLike) returned from test and hook callbacks, matching Jest and node --test. It adds a ~15-line coerce_thenable helper in src/runtime/test_runner/bun_test.rs that wraps a returned non-promise object with a callable .then in a native JSPromise via the spec's ResolvePromise, then reuses the existing promise path in run_test_callback. A new test/js/bun/test/thenable.test.ts spawns fixtures covering resolving/rejecting thenables, throwing then, a throwing then getter, a rejected beforeEach thenable, and confirms non-thenables still wait on done().

Security risks

None identified. The change reads a .then property on values already returned from user callbacks running in the same VM; there's no new privilege boundary, untrusted-input parsing, or resource exposure.

Level of scrutiny

Medium-high. run_test_callback is the core path every test and hook callback flows through, so a subtle regression here would affect every bun test user. The change is small and mirrors the surrounding error-handling pattern exactly, but it introduces two observable behavior changes: (1) every object returned from a callback now gets a .then property access it didn't get before (observable through getters/proxies, and the getter is invoked twice — once by the callable check and once by ResolvePromise), and (2) a callback that both takes done and returns a thenable now waits on both via the dcb_ref refcount, where previously the thenable was ignored. Both are arguably correctness fixes that align with Jest, but they're the kind of semantic shift a maintainer should sign off on.

Other factors

The tests are thorough and follow the harness conventions well (subprocess spawn, ordering assertions instead of timers, snapshot on the pass fixture, targeted toContain on the fail fixture). The JSValue::ZERO path (callback threw) is safe because !value.is_object() short-circuits. Error handling in the coercion branch mirrors the callback-error branch immediately above it. CI was still building at the time of review, so there's no green-build confirmation yet.

[robobun]

Both flagged behaviors are intentional. Detail on each, in case it helps the human pass:

The .then lookup. It only runs when the return value is an object that is not already a JSPromise. Callbacks that return undefined (the overwhelming majority) short-circuit on is_object() and never touch it. The shape matches the thenable check already in the tree for node's captureRejections (JSEventListener.cpp: isObject() then get(vm.propertyNames->then) then isCallable()), and it matches jest, which reads then twice as well: once for typeof returnedValue.then === 'function' and once for the returnedValue.then(resolve, reject) call. So a suite that breaks under the second read is already broken under jest.

The read goes through get_if_property_exists, which stops at Object.prototype, so a polluted Object.prototype.then cannot retroactively turn every object-returning test into an awaited one.

Handing the value to ResolvePromise instead of calling then directly (as jest does) is deliberate: it gives one-shot resolve semantics and turns a then that throws after resolving, or resolves twice, into the spec outcome rather than a double completion.

A single read is possible: resolve a fresh promise unconditionally and infer thenable-ness from its status, since pending means a thenable job was queued and fulfilled means it was not. It costs a throwaway promise on every object return and makes the non-thenable path less obvious, so I kept the explicit check. Happy to switch if a maintainer prefers it.

done plus a thenable. That is already the behavior for a returned pending native promise: the dcb_ref refcount makes the runner wait for both the promise and done(). Before this change the thenable was dropped, so the test finished as soon as done() fired even if the thenable had rejected. A non-thenable return value still takes the synchronous path untouched, which thenable.test.ts pins with the { then: null } plus done case (it would regress if the coercion were applied to every object).

[coderabbitai]

Actionable comments posted: 2

🤖 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/bun_test.rs`:
- Around line 1123-1132: The `then` accessor in `bun_test.rs` is being observed
twice: once via `value.get_if_property_exists(global_this, "then")` and again
indirectly by `promise.resolve(global_this, value)`, which can re-run promise
assimilation side effects. Update the logic around the `then` handling path to
reuse the cached lookup result or a one-lookup helper instead of triggering a
second property access, and add a test in the `bun_test`/promise assimilation
path that verifies the getter is called only once.

In `@test/js/bun/test/thenable.test.ts`:
- Around line 1-142: The new thenable coverage should be added to the existing
bun:test runner integration tests instead of introducing a separate file. Move
the cases from the current thenable test module into the appropriate existing
suite that exercises test/hook execution in bun:test, keeping the assertions
around returned thenables, rejected thenables, throwing then, and throwing then
getters together for discoverability. Use the existing bun:test integration
entrypoint that already covers runner behavior so these scenarios live alongside
related test execution cases.

🪄 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: 32a87cab-331e-4106-b83a-9b734038b193

📥 Commits

Reviewing files that changed from the base of the PR and between fb50cce and 8eed39c.

📒 Files selected for processing (2)

• src/runtime/test_runner/bun_test.rs
• test/js/bun/test/thenable.test.ts

[claude]

No bugs found, but this changes the core test-runner callback path (every test/hook flows through it) and adds an observable .then read on returned objects, plus there's an open design alternative you flagged — worth a maintainer's eyes.

Extended reasoning...

Overview

This PR makes bun test await userland thenables (PromiseLike) returned from test/hook callbacks, matching jest and node --test. It adds a coerce_thenable helper in src/runtime/test_runner/bun_test.rs that wraps a returned thenable in a real JSPromise via ResolvePromise, then inserts a call to it in run_test_callback between the callback invocation and the existing promise-handling path. A new spawned-fixture test file covers resolve/reject/throwing-then/throwing-getter/hook cases and pins the non-thenable + done path.

Security risks

Low. The new .then property read on returned objects can invoke user getters/Proxy traps, but this is inside the test runner where user code is already being executed with full trust. No auth, crypto, or sandbox boundaries are involved.

Level of scrutiny

High. run_test_callback is the single funnel every test and hook callback in every bun test run passes through. The change:

• Adds a new observable side effect (reading .then on any object a callback returns), which could interact with Proxy return values or side-effecting getters in existing suites.
• Sits amid dense unsafe re-entrancy handling (raw *mut BunTest derefs, UnsafeCell aliasing contracts) — the new call runs user JS between two such reborrows.
• Subtly changes done + returned-object semantics (now waits for both if the object is a thenable).

Other factors

The author explicitly offers a design alternative (unconditional Promise.resolve to avoid the double .then read) and says "Happy to switch if a maintainer prefers it" — that's a design call a human should make. The PR is well-tested and well-argued, but it is neither mechanical nor obvious, so I'm not auto-approving.

[robobun]

Took the review's advice over my earlier comment: coerce_thenable now reads then exactly once (7ec6067), so that comment's reasoning about the double read no longer applies to the code.

ResolvePromise already fulfills a non-thenable synchronously, so the promise's status after resolve() is the answer and no separate lookup is needed:

let promise = bun_jsc::JSPromise::create(global_this);
promise.resolve(global_this, value)?;
match promise.status() {
    // Not a thenable. Keep the original value on the synchronous path.
    PromiseStatus::Fulfilled => Ok(value),
    PromiseStatus::Pending | PromiseStatus::Rejected => Ok(promise.to_js()),
}

Pending means the thenable job was queued, rejected means the then getter threw, and fulfilled means it was never a thenable, so the original value goes back out and a returned plain object (with or without a done parameter) keeps taking the synchronous path exactly as before. That path is still pinned by the { then: null } plus done case.

Verified against a getter call count, which is now part of the fixture:

build	then reads
main	0 (never awaited, the bug)
c2a2a0f (two lookups)	2
7ec6067	1

Also moved both cases out of the new file and into test/js/bun/test/test-test.test.ts, alongside the existing coverage of run_test_callback.

Fail-before and pass-after, with src/ reverted to the merge base and rebuilt (USE_SYSTEM_BUN=1 is not a valid check here, that binary gets rebuilt from the branch):

src/ at fb50cce9:  0 pass, 2 fail
src/ at 7ec6067:  26 pass, 0 fail   (whole file)

[claude]

I didn't find any bugs, but this changes the hot path of run_test_callback (every test/hook return now allocates a throwaway JSPromise and observably reads .then on returned objects), so a maintainer should confirm the approach and the perf/behavior tradeoff.

Extended reasoning...

Overview

Adds coerce_thenable to BunTest::run_test_callback so that a test or hook callback returning a userland thenable (not a native JSPromise) is awaited instead of silently dropped. The helper creates a fresh JSPromise, calls the spec's ResolvePromise on the return value, and infers thenable-ness from the resulting status (Pending/Rejected → treat as async; Fulfilled → keep the original value on the sync path). Comprehensive integration tests are added to test/js/bun/test/test-test.test.ts covering resolving thenables, rejecting thenables, throwing then, throwing then getters, hooks, and the non-thenable-object-plus-done case.

Security risks

None. This is test-runner control flow; no untrusted-input parsing, auth, crypto, or filesystem paths are touched.

Level of scrutiny

Medium-high. run_test_callback is executed for every single test and hook callback in every bun test invocation. The change is small (~30 native lines) and the error-handling mirrors the existing callback-threw path exactly, but it introduces two observable behavior changes that a maintainer should sign off on:

1. Every callback that returns a non-promise object now has .then read on it (via ResolvePromise), where previously it was ignored. This is spec-correct and matches jest/node:test, but it's a user-visible semantic change.
2. Every such object return now allocates a throwaway JSPromise. The author explicitly flagged this cost in the thread and switched to it after CodeRabbit feedback about double .then reads — a maintainer should confirm the tradeoff is acceptable.

Other factors

• The implementation looks correct: JSValue::ZERO from the earlier error path short-circuits on is_object(); the promise local is stack-rooted; the Err branch mirrors the neighboring exception handler.
• Test coverage is thorough and follows repo conventions (tempDir, normalizeBunSnapshot, concurrent pipe drain, exit-code asserted last, single-then-read pinned).
• The CodeRabbit "double lookup" concern was addressed in 7ec6067; the remaining "move tests into existing file" suggestion was also addressed (tests are now in test-test.test.ts, not a new file).
• CI build #68565 was still in progress at review time.

[robobun]

Two notes for whoever picks this up.

What the coercion actually costs

The review above reads as if every test/hook return now allocates a promise and reads .then. It is narrower than that, because of the two early exits:

if !value.is_object() || value.as_promise().is_some() {
    return Ok(value);
}

callback returns	promise allocated	then reads
undefined (nearly all tests and hooks)	no	0
a native promise	no	0
a plain non-thenable object	yes, discarded	0
a thenable	yes, kept	1

The plain-object row reads then zero times because ResolvePromise checks isDefinitelyNonThenable() first, which is a cached per-structure bit, and fulfills on the spot. So the only callbacks that pay anything are the rare ones returning an object, and the only ones that observe then are the thenables this PR exists to support.

CI

Build 68565 finished 285 passed, 1 failed. The one red job is :darwin: 26 aarch64 - test-bun, and it never ran a test:

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).

Same failure, same agent (darwin-aarch64-26-5-1-1), on both builds of this branch (68545 and 68565), on two different commits. Every other lane is green, including every test-bun lane that actually executed the changed test-test.test.ts (linux x64/aarch64, musl, windows, darwin-x64, asan). The only annotations either build produced are binary-size (no change) and flaky [warning] for unrelated Windows install tests that retried green.

I already pushed one empty commit to re-roll and landed on the same agent, so I am not going to push another. The job needs a retry from someone with buildkite write access.

[robobun]

Superseded by #41008. That PR coerces a returned thenable the same way and also covers a pending Promise subclass whose overridden then() starts the work (Bun.SQL's Query, Bun.$'s ShellPromise), which this branch leaves on the internal-state path. It is rebased on current main and carries the same rejected-thenable and hook coverage.

[robobun]

Closing: superseded by #41008.