Skip to content

fix(bun:test): return an object when constructing a mock function #31365

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

fix(bun:test): return an object when constructing a mock function #31365

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
39 changes: 38 additions & 1 deletion 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 @@ -981,6 +982,42 @@
return JSValue::encode(jsUndefined());
}

// Native constructors must always return an object, so this mirrors the behavior of
// constructing a plain JavaScript function: create `this` from newTarget.prototype,
// invoke the mock with it, and return the result if it is an object, otherwise `this`.
JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * globalObject, CallFrame* callframe))
{
auto& vm = JSC::getVM(globalObject);
auto scope = DECLARE_THROW_SCOPE(vm);
JSMockFunction* fn = dynamicDowncast<JSMockFunction>(callframe->jsCallee());
if (!fn) [[unlikely]] {
throwTypeError(globalObject, scope, "Expected callee to be mock function"_s);
return {};
}

JSObject* newTarget = callframe->newTarget().getObject();
if (!newTarget) [[unlikely]] {
newTarget = fn;
}
JSValue prototype = newTarget->get(globalObject, vm.propertyNames->prototype);
RETURN_IF_EXCEPTION(scope, {});
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);
}

Check warning on line 1019 in src/jsc/bindings/JSMockFunction.cpp

View check run for this annotation

Claude / Claude Code Review

mock.instances not populated when constructing a mock 

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.
Comment on lines +1004 to +1019

@claude claude Bot May 24, 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.

🟡 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

  1. const Fn = jest.fn(function () { this.x = 1 });
  2. const a = new Fn(); — enters jsMockFunctionConstruct, which builds thisObject, calls JSC::call(globalObject, fn, callData, thisObject, args) (recording calls[0] = [], contexts[0] = thisObject, etc.), and returns thisObject.
  3. Nothing in either function touches fn->instances.
  4. Fn.mock.instances → getInstances() lazily allocates an empty array → [].
  5. 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.


void JSMockFunctionPrototype::finishCreation(JSC::VM& vm, JSC::JSGlobalObject* globalObject)
{
Base::finishCreation(vm);
Expand Down
40 changes: 40 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 @@ -794,6 +794,46 @@ describe("mock()", () => {

expect(bar()()).toBe(true);
});

describe("constructing a mock", () => {
test("new on a mock with no implementation returns an object and records the call", () => {
const fn = jest.fn();
const instance = new fn(1, 2);
expect(typeof instance).toBe("object");
expect(instance).not.toBeNull();
expect(fn).toHaveBeenCalledTimes(1);
expect(fn.mock.calls[0]).toEqual([1, 2]);
expect(fn.mock.contexts[0]).toBe(instance);
});

test("Reflect.construct on a mock returns an object", () => {
const fn = jest.fn();
const instance = Reflect.construct(fn, []);
expect(typeof instance).toBe("object");
expect(instance).not.toBeNull();
});

test("new calls the implementation with the newly created `this`", () => {
const fn = jest.fn(function () {
this.x = 42;
});
const instance = new fn();
expect(instance.x).toBe(42);
});

test("new returns the implementation's return value when it is an object", () => {
const result = { a: 1 };
const fn = jest.fn(() => result);
expect(new fn()).toBe(result);
});

test("new ignores non-object return values", () => {
const fn = jest.fn().mockReturnValue(42);
const instance = new fn();
expect(typeof instance).toBe("object");
expect(instance).not.toBeNull();
});
});
});

describe("spyOn", () => {
Expand Down
Loading