Skip to content

fix(bun:test): constructing a mock function must return an object #31381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
+64 −3
Closed

fix(bun:test): constructing a mock function must return an object #31381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions src/jsc/bindings/JSMockFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
}

JSC_DECLARE_HOST_FUNCTION(jsMockFunctionCall);
JSC_DECLARE_HOST_FUNCTION(jsMockFunctionConstruct);
JSC_DECLARE_CUSTOM_GETTER(jsMockFunctionGetter_protoImpl);
JSC_DECLARE_CUSTOM_GETTER(jsMockFunctionGetter_mock);
JSC_DECLARE_HOST_FUNCTION(jsMockFunctionGetter_mockGetLastCall);
Expand Down Expand Up @@ -462,7 +463,7 @@
}

JSMockFunction(JSC::VM& vm, JSC::Structure* structure, CallbackKind wrapKind)
: Base(vm, structure, jsMockFunctionCall, jsMockFunctionCall)
: Base(vm, structure, jsMockFunctionCall, jsMockFunctionConstruct)
{
initMock();
}
Expand Down Expand Up @@ -826,7 +827,7 @@
return result;
}

JSC_DEFINE_HOST_FUNCTION(jsMockFunctionCall, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe))
static JSC::EncodedJSValue jsMockFunctionCallImpl(JSGlobalObject* lexicalGlobalObject, CallFrame* callframe, JSValue thisValue)
{
Zig::GlobalObject* globalObject = uncheckedDowncast<Zig::GlobalObject>(lexicalGlobalObject);
auto& vm = JSC::getVM(globalObject);
Expand All @@ -838,7 +839,6 @@
}

JSC::ArgList args = JSC::ArgList(callframe);
JSValue thisValue = callframe->thisValue();
JSC::JSArray* argumentsArray = nullptr;
{
JSC::ObjectInitializationScope object(vm);
Expand Down Expand Up @@ -981,6 +981,40 @@
return JSValue::encode(jsUndefined());
}

JSC_DEFINE_HOST_FUNCTION(jsMockFunctionCall, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe))
{
return jsMockFunctionCallImpl(lexicalGlobalObject, callframe, callframe->thisValue());
}

JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe))
{
auto& vm = JSC::getVM(lexicalGlobalObject);
auto scope = DECLARE_THROW_SCOPE(vm);

JSValue newTarget = callframe->newTarget();
JSObject* prototype = nullptr;
if (newTarget.isObject()) {
JSValue prototypeValue = asObject(newTarget)->get(lexicalGlobalObject, vm.propertyNames->prototype);
RETURN_IF_EXCEPTION(scope, {});
if (prototypeValue.isObject())
prototype = asObject(prototypeValue);
}

JSObject* thisObject = prototype
? JSC::constructEmptyObject(lexicalGlobalObject, prototype)
: JSC::constructEmptyObject(lexicalGlobalObject);

JSValue result = JSValue::decode(jsMockFunctionCallImpl(lexicalGlobalObject, callframe, thisObject));
RETURN_IF_EXCEPTION(scope, {});

// A [[Construct]] call must always produce an object. If the mock implementation returned a
// primitive, return the newly created instance instead, matching ordinary constructor semantics.
if (result && result.isObject())
return JSValue::encode(result);

return JSValue::encode(thisObject);

Check notice on line 1015 in src/jsc/bindings/JSMockFunction.cpp

View check run for this annotation

Claude / Claude Code Review

mock.instances not populated on construct 

Not blocking, and pre-existing, but worth a follow-up: `mock.instances` is still never populated — `fn->instances` is only declared, cleared, lazily created empty, and GC-visited, never written to — so even with the new construct path, `new fn(); fn.mock.instances` is `[]` where Jest gives `[<instance>]`. Since this PR now creates the very `thisObject` that `mock.instances` is meant to record, it'd be a natural place to push it (and, to fully match Jest, push `thisValue` on every call alongside
Comment on lines +1003 to +1015

@claude claude Bot May 25, 2026

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.

🟣 Not blocking, and pre-existing, but worth a follow-up: mock.instances is still never populated — fn->instances is only declared, cleared, lazily created empty, and GC-visited, never written to — so even with the new construct path, new fn(); fn.mock.instances is [] where Jest gives [<instance>]. Since this PR now creates the very thisObject that mock.instances is meant to record, it'd be a natural place to push it (and, to fully match Jest, push thisValue on every call alongside the contexts push in jsMockFunctionCallImpl).

Extended reasoning...

What this is

JSMockFunction has an instances field (line 271) that backs mockFn.mock.instances, but nothing in the file ever writes to it. Grepping confirms it appears only at:

  • line 271 — the WriteBarrier<JSArray> declaration,
  • line 350 — clear() clears it,
  • lines 420–425 — getInstances() lazily creates an empty array,
  • line 483 — GC visit,
  • line 757 — the "instances" slot in the mock object structure.

There is no instances->push(...) or initializeIndex call anywhere, so fn.mock.instances is always an empty array — both before and after this PR. In Jest, mock.instances records the this value for every invocation (parallel to mock.contexts), and is most useful exactly for the new fn() case this PR is adding.

Why mention it on this PR

This is a pre-existing gap — instances has never been populated in Bun, and this PR does not regress it. The refutation correctly notes that the PR's stated scope is the isCell() crash fix and that "matching Jest's behavior" in the description refers to the primitive-return-replaced-by-this semantics, not full mock.instances parity. That's all fair, and this should not block the PR.

It is still worth a non-blocking note because the new jsMockFunctionConstruct is the first place in this file that actually creates the instance object that mock.instances is supposed to hold. Before this PR there was no obvious hook; now there is, and it's two lines away from where the value would be pushed. Flagging it as a "while you're here" follow-up is reasonable; landing it separately is equally fine.

Step-by-step proof

  1. const fn = jest.fn();
  2. const inst = new fn(); — enters jsMockFunctionConstruct (line 989), allocates thisObject (lines 1003–1005), calls jsMockFunctionCallImpl with thisValue = thisObject.
  3. jsMockFunctionCallImpl pushes onto fn->calls, fn->contexts, fn->invocationCallOrder, and fn->returnValues (lines 854–910). It never touches fn->instances.
  4. jsMockFunctionConstruct returns thisObject (line 1015). fn->instances is still unset.
  5. fn.mock.instances triggers getInstances() (line 420), which lazily creates an empty array. Result: [].
  6. In Jest the same code yields [inst].

Suggested follow-up fix

In jsMockFunctionCallImpl, right next to the existing contexts push (lines 868–880), add an analogous block that pushes thisValue onto fn->instances. That matches Jest exactly: Jest records this into mock.instances on every call (for plain calls it's the receiver, for new it's the freshly created instance), so doing it only in jsMockFunctionConstruct would still diverge for non-construct calls. Doing it in the shared impl covers both, and the new construct path already threads the right thisObject through as thisValue.

}

void JSMockFunctionPrototype::finishCreation(JSC::VM& vm, JSC::JSGlobalObject* globalObject)
{
Base::finishCreation(vm);
Expand Down
27 changes: 27 additions & 0 deletions test/js/bun/test/mock-fn.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,33 @@ describe("mock()", () => {
expect(fn).toHaveBeenCalledWith();
});

test("are constructable", () => {
// Constructing a mock must always produce an object, even when the
// implementation returns a primitive or there is no implementation at all.
const fn = jest.fn();
expect(new fn()).toBeInstanceOf(Object);
expect(Reflect.construct(fn, [])).toBeInstanceOf(Object);
expect(fn).toHaveBeenCalledTimes(2);

const withPrimitive = jest.fn(() => 42);
expect(new withPrimitive()).toBeInstanceOf(Object);

const withReturnValue = jest.fn();
withReturnValue.mockReturnValue(5);
expect(new withReturnValue()).toBeInstanceOf(Object);
expect(Reflect.construct(withReturnValue, [])).toBeInstanceOf(Object);

const result = { a: 1 };
const withObject = jest.fn(() => result);
expect(new withObject()).toBe(result);
expect(Reflect.construct(withObject, [])).toBe(result);

const thrower = jest.fn(() => {
throw new Error("boom");
});
expect(() => new thrower()).toThrow("boom");
});

test("mockName returns this", () => {
const fn = jest.fn();
expect(fn.mockName()).toBe(fn);
Expand Down
Loading