fix(bun:test): return an object when constructing a mock function#31365
fix(bun:test): return an object when constructing a mock function#31365robobun wants to merge 1 commit into
Conversation
Mock functions used the call handler for both [[Call]] and [[Construct]]. A native constructor must return an object, so constructing a mock whose invocation result was not an object (e.g. `Reflect.construct(jest.fn(), [])`) failed the isCell() assertion in JSC::JSValue::asCell, and `new (jest.fn())()` evaluated to undefined. Give JSMockFunction a dedicated construct handler that mirrors ordinary function construction: create `this` from newTarget.prototype, invoke the mock with it, and return the invocation result if it is an object, otherwise the newly created `this`.
|
Updated 12:06 PM PT - May 24th, 2026
❌ @robobun, your commit ec96979 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31365That installs a local version of the PR into your bun-31365 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR extends Bun's mock function implementation to support JavaScript constructor semantics. Mock functions can now be called with ChangesMock function constructor support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as a duplicate of #30212, which fixes the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| JSObject* thisObject = prototype.isObject() | ||
| ? JSC::constructEmptyObject(globalObject, asObject(prototype)) | ||
| : JSC::constructEmptyObject(globalObject); | ||
|
|
||
| JSC::CallData callData = JSC::getCallData(fn); | ||
| ASSERT(callData.type != JSC::CallData::Type::None); | ||
| JSC::ArgList args = JSC::ArgList(callframe); | ||
| JSValue returnValue = JSC::call(globalObject, fn, callData, thisObject, args); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| if (returnValue.isObject()) { | ||
| return JSValue::encode(returnValue); | ||
| } | ||
|
|
||
| return JSValue::encode(thisObject); | ||
| } |
There was a problem hiding this comment.
🟡 Now that [[Construct]] actually works, it might be worth pushing the constructed instance onto fn->instances here so fn.mock.instances is populated like it is in Jest. The instances array is already declared, exposed on the mock object (offset 2), cleared in clear(), and GC-visited — it's just never written to, so fn.mock.instances stays empty after new fn(). This is pre-existing scaffolding that was never wired up rather than a regression, so feel free to leave it for a follow-up.
Extended reasoning...
What's missing
JSMockFunction declares a WriteBarrier<JSArray> instances field, lazily creates it in getInstances(), exposes it as the instances property at offset 2 of the mock-object structure, clears it in clear(), and visits it in visitAdditionalChildrenInGCThread. But nothing in the file ever pushes to it. jsMockFunctionCall records calls, contexts, invocationCallOrder, and returnValues, and the new jsMockFunctionConstruct simply delegates to jsMockFunctionCall via JSC::call without separately recording the constructed instance. The result is that fn.mock.instances is always an empty array, even after new fn().
Why it matters now
In Jest, mock.instances is the canonical API for tracking objects created by constructing a mock — new fn() pushes the resulting this (or the returned object) onto fn.mock.instances, while plain calls push undefined. Before this PR the [[Construct]] slot pointed at jsMockFunctionCall, so new (mock())() either returned undefined (release) or hit a debug assertion; the empty instances array was effectively unobservable because construction itself was broken. With this PR construction works correctly and the PR description says it "matches Jest", so users porting Jest tests that read fn.mock.instances[0] will now hit a visible compat gap.
Step-by-step
const Fn = jest.fn(function () { this.x = 1 });const a = new Fn();— entersjsMockFunctionConstruct, which buildsthisObject, callsJSC::call(globalObject, fn, callData, thisObject, args)(recordingcalls[0] = [],contexts[0] = thisObject, etc.), and returnsthisObject.- Nothing in either function touches
fn->instances. Fn.mock.instances→getInstances()lazily allocates an empty array →[].- In Jest the same code yields
Fn.mock.instances[0] === a.
Suggested fix
In jsMockFunctionConstruct, after computing the final return value (returnValue.isObject() ? returnValue : thisObject), push it onto fn->instances the same way jsMockFunctionCall pushes to contexts (lazily creating the array on first use). For full Jest parity jsMockFunctionCall would also push undefined for non-construct calls, but the construct case is the important one and is squarely in scope for this PR.
Severity
This is pre-existing unwired scaffolding — the instances field has always been exposed and always been empty — so it is not a regression and shouldn't block the crash fix. Filing as a nit since this PR is the natural place to wire it up, but a follow-up is fine too.
1 participant
What does this PR do?
Fixes a crash (debug assertion
ASSERTION FAILED: isCell()inJSC::JSValue::asCell,JSCJSValue.h:1043) found by fuzzing, triggered by constructing abun:testmock function when its invocation result is not an object:JSMockFunctionpassedjsMockFunctionCallas both its[[Call]]and[[Construct]]handler. JSC requires a native constructor to return an object (Interpreter::executeConstructdoesasObject(result)on the return value), but the mock call handler can return any value —undefinedwhen there is no implementation, whatevermockReturnValue()was given, etc. Constructing such a mock throughReflect.construct(or any C++ caller ofJSC::construct) hit the assertion, and plainnewon a mock returned a non-object, which violates constructor semantics.This PR gives
JSMockFunctiona dedicated construct handler that mirrors how constructing an ordinary JavaScript function behaves (and matches Jest):thisfromnewTarget.prototype(falling back to a plain object)this, so calls/contexts are recorded andfunction () { this.x = 1 }implementations workthisBehavior before/after:
Reflect.construct(mock(), [])new (mock())()undefinednew (mock(function () { this.x = 1 }))().x1new (mock(() => someObject))()someObjectsomeObject(unchanged)new (mock().mockReturnValue(42))()How did you verify your code works?
test/js/bun/test/mock-fn.test.jscoveringnewandReflect.constructon mocks with no implementation, with an implementation that assigns tothis, with an object-returning implementation, and with a non-objectmockReturnValue. 4 of the 5 new tests fail on the current release build and all pass with this change.mock-fn,mock-disposable,mock-module*,spyMatchers,expect-toHaveReturnedWith) pass, including withBUN_JSC_validateExceptionChecks=1.