Skip to content

bun:test: return an object when constructing a mock function #31373

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
Closed

bun:test: return an object when constructing a mock function #31373

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
38 changes: 35 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,38 @@
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);

// For constructors implemented in C++, the this value slot holds new.target.
JSValue newTarget = callframe->newTarget();
JSObject* prototype = nullptr;
if (newTarget && 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, {});

// [[Construct]] must return an object, so fall back to the newly created
// instance when the mock implementation produced a primitive.
if (result && result.isObject())
return JSValue::encode(result);
return JSValue::encode(thisObject);

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

View check run for this annotation

Claude / Claude Code Review

mock.instances not populated on construct (pre-existing) 

Pre-existing, not introduced here: `mock.instances` is exposed on the mock object but is never populated — `jsMockFunctionCallImpl` pushes to `calls`/`contexts`/`invocationCallOrder`/`returnValues` but never to `fn->instances`. Now that mocks have a real `[[Construct]]` path, users will reach for `fn.mock.instances[0]` (the canonical Jest API for constructor calls) and find it empty. Might be worth pushing `thisValue` into `instances` alongside `contexts` while you're in here, but it shouldn't b
Comment on lines +1004 to +1013

@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.

🟣 Pre-existing, not introduced here: mock.instances is exposed on the mock object but is never populated — jsMockFunctionCallImpl pushes to calls/contexts/invocationCallOrder/returnValues but never to fn->instances. Now that mocks have a real [[Construct]] path, users will reach for fn.mock.instances[0] (the canonical Jest API for constructor calls) and find it empty. Might be worth pushing thisValue into instances alongside contexts while you're in here, but it shouldn't block this crash fix.

Extended reasoning...

What the bug is

JSMockFunction declares a WriteBarrier<JSArray> instances field, lazily creates it via getInstances(), exposes it at offset 2 of the mock-object structure (the mock.instances property), clears it in clear(), and visits it in GC. But nothing in jsMockFunctionCallImpl (or anywhere else in the file) ever pushes to it. So fn.mock.instances is always an empty array regardless of how many times the mock is called or constructed.

In Jest, mock.instances records the this value for every invocation — for new fn() that's the newly created instance, and for plain calls it's whatever this was bound to. It is the documented way to assert on constructed instances (e.g. expect(Foo.mock.instances[0]).toBe(a)).

Code path

Walk through const fn = jest.fn(); const inst = new fn(); after this PR:

  1. jsMockFunctionConstruct runs, builds thisObject from new.target.prototype.
  2. It calls jsMockFunctionCallImpl(globalObject, callframe, thisObject).
  3. Inside the impl, fn->calls gets [ [] ], fn->contexts gets [thisObject], fn->invocationCallOrder gets [n], and fn->returnValues gets [{type:'return', value:undefined}].
  4. There is no statement of the form fn->instances...->push(...) or instances->initializeIndex(...) — grep for instances in the file and you'll find only the declaration, clear(), getInstances(), the GC visit, and the structure offset.
  5. jsMockFunctionConstruct returns thisObject.
  6. fn.mock.instances is still []; fn.mock.contexts is [inst].

The PR's own test asserts expect(fn.mock.contexts[0]).toBe(instance) rather than fn.mock.instances[0], which is consistent with instances being empty.

Why existing code doesn't prevent it

There simply is no write site. The array is wired up end-to-end on the read side (lazy creation, structure slot, GC) but the population step was never implemented. Before this PR it mattered less because constructing a mock didn't have proper semantics anyway (it returned primitives / asserted in debug). Now that [[Construct]] is real, the gap becomes user-visible in the exact scenario this PR enables.

Impact

Tests ported from Jest that do things like:

const Ctor = jest.fn();
const a = new Ctor();
expect(Ctor.mock.instances).toContain(a); // fails: instances is []

will fail under bun:test even though new Ctor() now behaves correctly. The workaround is mock.contexts, but that's not what Jest users typically reach for when testing constructors.

How to fix

In jsMockFunctionCallImpl, mirror the contexts push for instances: after pushing thisValue to fn->contexts, also push it to fn->instances (lazily creating the array the same way). Jest records this in instances for every call, not just constructor calls, so doing it unconditionally in the shared impl matches upstream semantics and keeps instances.length === calls.length.

Severity

Pre-existing. instances was never populated before this PR either; this change just makes the omission relevant. It shouldn't block landing the crash fix, but it's a natural follow-up (or a one-line addition here if the author prefers).

}

void JSMockFunctionPrototype::finishCreation(JSC::VM& vm, JSC::JSGlobalObject* globalObject)
{
Base::finishCreation(vm);
Expand Down
36 changes: 36 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 @@ -1015,3 +1015,39 @@ describe("spyOn", () => {

// spyOn does not work with getters/setters yet.
});

describe("mock constructor calls", () => {
test("constructing a mock with no implementation returns an object", () => {
const fn = jest.fn();
const instance = new fn();
expect(typeof instance).toBe("object");
expect(typeof Reflect.construct(fn, [])).toBe("object");
expect(fn.mock.contexts[0]).toBe(instance);
});

test("constructing a mock whose implementation returns a primitive returns an object", () => {
const fn = jest.fn(() => 42);
expect(typeof new fn()).toBe("object");
expect(typeof Reflect.construct(fn, [])).toBe("object");
expect(fn.mock.results).toEqual([
{ type: "return", value: 42 },
{ type: "return", value: 42 },
]);
});

test("constructing a mock whose implementation returns an object returns that object", () => {
const obj = { a: 1 };
const fn = jest.fn(() => obj);
expect(new fn()).toBe(obj);
expect(Reflect.construct(fn, [])).toBe(obj);
});

if (isBun) {
test("constructing a spy on a missing property does not crash", () => {
const target = {};
const spy = spyOn(target, "doesNotExist");
expect(typeof Reflect.construct(spy, [])).toBe("object");
spy.mockRestore();
});
}
});
Loading