From 150db2f8818f02e4a9fab1c8d8f7fd49aff1451d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 6 Nov 2026 19:14:40 +0000 Subject: [PATCH 1/3] bun:test: restore a spy when its object refuses to delete it A spy on an inherited or missing property is an own property that restoring deletes. putBackOriginal() dropped the answer of that delete, and clearSpy() forgot a spy that was still installed. - An object that was frozen or sealed after spyOn(): a direct store makes the property configurable again, and the delete is repeated. That is how the restore of an own property goes through a lock. - A Proxy that keeps the spy: the value that it showed before the spy is defined back through its defineProperty trap. The spy is looked for behind the traps, which can hide it or answer true for nothing. - If the spy is still on the object (the element of a frozen array, a Proxy that takes nothing back), the restore throws a TypeError and the spy stays registered. --- docs/test/mocks.mdx | 2 + src/jsc/bindings/JSMockFunction.cpp | 189 ++++++-- ...t-is-frozen-afterwards-is-restored.test.ts | 43 ++ test/js/bun/test/mock-disposable.test.ts | 18 + test/js/bun/test/mock-fn.test.js | 402 ++++++++++++++++++ test/js/bun/test/vi-utils.test.ts | 15 +- 6 files changed, 628 insertions(+), 41 deletions(-) create mode 100644 test/aot/a-spy-on-an-object-that-is-frozen-afterwards-is-restored.test.ts diff --git a/docs/test/mocks.mdx b/docs/test/mocks.mdx index 61f5b4ec0993..c8374e58b1f6 100644 --- a/docs/test/mocks.mdx +++ b/docs/test/mocks.mdx @@ -726,6 +726,8 @@ test("foo, bar, baz", () => { Call `mock.restore()` in an `afterEach` block, or in your test preload script, instead of repeating cleanup in every test. +A spy is restored even if its object was frozen or sealed after `spyOn()`. A `Proxy` that doesn't delete the spy, such as the `localStorage` of happy-dom, gets the original back as a property of its own. If the object still keeps the spy, as a frozen array keeps a spy on one of its elements, `mockRestore()` and `mock.restore()` throw a `TypeError` and the spy stays active. This includes spies made with `vi.spyOn()`. + ### Cleanup After Each Test File When a test file ends, Bun restores any spies that are still active and clears every mock's recorded calls. Return values and implementations set with `mockReturnValue()`, `mockImplementation()` and similar methods stay until you reset the mock. A mock in a module that several test files import keeps them from one file to the next. diff --git a/src/jsc/bindings/JSMockFunction.cpp b/src/jsc/bindings/JSMockFunction.cpp index be3854db4664..c5450c98f617 100644 --- a/src/jsc/bindings/JSMockFunction.cpp +++ b/src/jsc/bindings/JSMockFunction.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -307,6 +308,21 @@ static bool isOpaqueProperty(JSGlobalObject* globalObject, JSObject* object, Pro return object->type() == ProxyObjectType; } +// The traps of a Proxy can hide a property or run script. This is the object that they stand in front of. +static JSObject* objectBehindTraps(JSObject* object) +{ + while (auto* proxy = dynamicDowncast(object)) + object = proxy->target(); + return object; +} + +// A global object refuses to delete the name of one of its variables, and a direct store would put a property in front of the variable. +static bool isVariableOf(JSObject* object, PropertyName key) +{ + auto* scopeObject = dynamicDowncast(object); + return scopeObject && scopeObject->symbolTable()->contains(key.uid()); +} + // Lets a spy replace a property of a locked built-in. No JavaScript may run meanwhile, and the traps of an opaque object are JavaScript. class ThroughLockOfBuiltins { public: @@ -320,36 +336,51 @@ class ThroughLockOfBuiltins { std::optional m_allow; }; -static void putOwnPropertyStorage(JSGlobalObject* globalObject, JSObject* object, PropertyName key, JSValue storage, unsigned attributes, bool isOpaque) +// An opaque object decides through its own [[DefineOwnProperty]], which for a Proxy is a trap. +static bool defineOwnPropertyStorage(JSGlobalObject* globalObject, JSObject* object, PropertyName key, JSValue storage, unsigned attributes, bool shouldThrow) +{ + PropertyDescriptor descriptor; + if (attributes & PropertyAttribute::Accessor) + descriptor.setAccessorDescriptor(uncheckedDowncast(storage), attributes); + else + descriptor.setDescriptor(storage, attributes); + return object->methodTable()->defineOwnProperty(object, globalObject, key, descriptor, shouldThrow); +} + +// False if the object kept what it had. Only an index does that without an exception: JSC stores an element that has attributes through [[DefineOwnProperty]]. +static bool putOwnPropertyStorage(JSGlobalObject* globalObject, JSObject* object, PropertyName key, JSValue storage, unsigned attributes, bool isOpaque) { auto& vm = JSC::getVM(globalObject); auto scope = DECLARE_THROW_SCOPE(vm); - if (isOpaque) { - PropertyDescriptor descriptor; - if (attributes & PropertyAttribute::Accessor) - descriptor.setAccessorDescriptor(uncheckedDowncast(storage), attributes); - else - descriptor.setDescriptor(storage, attributes); - scope.release(); - object->methodTable()->defineOwnProperty(object, globalObject, key, descriptor, true); - } else if (attributes & PropertyAttribute::Accessor) { - scope.release(); - object->putDirectAccessor(globalObject, key, uncheckedDowncast(storage), attributes); - } else if (attributes & PropertyAttribute::CustomAccessorOrValue) { + if (isOpaque) + RELEASE_AND_RETURN(scope, defineOwnPropertyStorage(globalObject, object, key, storage, attributes, true)); + if (attributes & PropertyAttribute::Accessor) + RELEASE_AND_RETURN(scope, object->putDirectAccessor(globalObject, key, uncheckedDowncast(storage), attributes)); + if (attributes & PropertyAttribute::CustomAccessorOrValue) { // putDirectCustomAccessor() asserts that the property is new. { VM::DeletePropertyModeScope deleteNonConfigurable(vm, VM::DeletePropertyMode::IgnoreConfigurable); JSCell::deleteProperty(object, globalObject, key); } - RETURN_IF_EXCEPTION(scope, ); + RETURN_IF_EXCEPTION(scope, false); object->putDirectCustomAccessor(vm, key, storage, attributes); - } else if (auto index = parseIndex(key)) { - scope.release(); - object->putDirectIndex(globalObject, *index, storage, attributes, PutDirectIndexLikePutDirect); - } else { - object->putDirect(vm, key, storage, attributes); + return true; + } + if (auto index = parseIndex(key)) + RELEASE_AND_RETURN(scope, object->putDirectIndex(globalObject, *index, storage, attributes, PutDirectIndexLikePutDirect)); + object->putDirect(vm, key, storage, attributes); + return true; +} + +// For an object that isn't opaque. NodeVMGlobalObject::deleteProperty() deletes from the contextified object, where the spy never was. +[[nodiscard]] static bool deleteOwnProperty(JSGlobalObject* globalObject, JSObject* object, PropertyName key) +{ + if (object->inherits()) { + DeletePropertySlot slot; + return NodeVMGlobalObject::Base::deleteProperty(object, globalObject, key, slot); } + return JSCell::deleteProperty(object, globalObject, key); } static constexpr unsigned descriptorAttributes = PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::Accessor; @@ -455,6 +486,8 @@ class JSMockFunction final : public JSC::InternalFunction { static constexpr unsigned SpyAttributeAccessor = 1 << 29; // isOpaqueProperty() was true at install time, so restore the same way. static constexpr unsigned SpyAttributeOpaque = 1 << 28; + // The original is what an opaque target showed for a key that it didn't own. Restoring deletes the spy. A target that keeps the spy gets the original as its own. + static constexpr unsigned SpyAttributeInherited = 1 << 26; JSString* jsName() { @@ -851,20 +884,26 @@ class JSMockFunction final : public JSC::InternalFunction { this->setPrototypeParent(globalObject, next && next->kind == JSMockImplementation::Kind::Call ? next->underlyingValue.get() : JSValue()); } - bool isAccessorOf(GetterSetter* accessor) + // `storage` is getOwnPropertyStorage() of a property. + bool isAccessorOf(JSValue storage) { + auto* accessor = tryJSDynamicCast(storage); + if (!accessor) + return false; if (accessor->getter() == this || accessor->setter() == this) return true; auto* getter = dynamicDowncast(accessor->getter()); return getter && getter->boundThis() == this; } - // `original` is getOwnPropertyStorage() of the replaced property, or empty if the target had no own one. + bool isInstalledAs(JSValue storage) { return storage == JSValue(this) || this->isAccessorOf(storage); } + + // `original` is getOwnPropertyStorage() of the replaced property, or empty if the target had no own one. See SpyAttributeInherited. void didInstall(JSObject* target, const Identifier& key, JSValue original, unsigned attributes); bool isRestoredSpy() const { return isSpy && !m_spyTarget; } - // Puts the spied-on property back as it was; that can throw (a module namespace export, an indexed slot). + // Puts the spied-on property back as it was; that can throw (a module namespace export, an indexed slot, an object that doesn't give the spy up). void clearSpy(JSGlobalObject* globalObject, bool shouldReset = true) { auto scope = DECLARE_THROW_SCOPE(globalObject->vm()); @@ -1001,25 +1040,33 @@ class JSMockFunction final : public JSC::InternalFunction { unsigned currentAttributes = 0; JSValue current = getOwnPropertyStorage(globalObject, target, identifier, currentAttributes); RETURN_IF_EXCEPTION(scope, ); - auto* accessor = tryJSDynamicCast(current); - if (!accessor || !this->isAccessorOf(accessor)) + if (!this->isAccessorOf(current) && isOpaque) { + current = getOwnPropertyStorage(globalObject, objectBehindTraps(target), identifier, currentAttributes); + RETURN_IF_EXCEPTION(scope, ); + } + if (!this->isAccessorOf(current)) return; } - // Behind the lookup above: the global of a node:vm context asks its sandbox, which may be a Proxy. - ThroughLockOfBuiltins throughLock(globalObject->vm(), isOpaque); - scope.release(); - if (original) { - putOwnPropertyStorage(globalObject, target, identifier, original, attributes & ~(SpyAttributeAccessor | SpyAttributeOpaque), isOpaque); - } else if (!isOpaque && target->inherits()) { - // NodeVMGlobalObject::deleteProperty() deletes from the contextified object, where the spy never was. - DeletePropertySlot slot; - NodeVMGlobalObject::Base::deleteProperty(target, globalObject, identifier, slot); - } else { - JSCell::deleteProperty(target, globalObject, identifier); + bool isRestored = false; + if (!isOpaque) [[likely]] { + // Behind the lookup above: the global of a node:vm context asks its sandbox, which may be a Proxy. + ThroughLockOfBuiltins throughLock(globalObject->vm(), false); + if (original) + isRestored = putOwnPropertyStorage(globalObject, target, identifier, original, attributes & ~SpyAttributeAccessor, false); + else + isRestored = deleteOwnProperty(globalObject, target, identifier); } + if (isRestored) [[likely]] { + scope.release(); + return; + } + this->takeSpyOffOrThrow(globalObject, target, scope); } + // Restores through the traps of an opaque object, and is where putBackOriginal() goes when another object refused. + void takeSpyOffOrThrow(JSGlobalObject*, JSObject* target, JSC::ThrowScope&); + Recorded m_recorded; Configuration m_configuration; JSC::WriteBarrier m_initialImplementation; @@ -2950,6 +2997,62 @@ static String propertyKeyForMessage(const Identifier& key) return key.isSymbol() ? makeString("Symbol("_s, key.string(), ')') : key.string(); } +__attribute__((cold, minsize)) NEVER_INLINE void JSMockFunction::takeSpyOffOrThrow(JSGlobalObject* globalObject, JSObject* target, JSC::ThrowScope& scope) +{ + // A store or a delete that threw answered false too. + RETURN_IF_EXCEPTION(scope, ); + // A trap that ran may have restored this spy itself. + if (m_spyTarget.get() != target) + return; + auto& vm = globalObject->vm(); + JSValue original = m_spyOriginal.get(); + Identifier key = m_spyIdentifier; + unsigned attributes = m_spyAttributes; + bool isOpaque = attributes & SpyAttributeOpaque; + bool isInherited = attributes & SpyAttributeInherited; + + if (isOpaque) { + // What a trap answers isn't read: it may say that it deleted what it kept, so the object is looked at behind its traps below. + if (original && !isInherited) + putOwnPropertyStorage(globalObject, target, key, original, attributes & ~(SpyAttributeAccessor | SpyAttributeOpaque), true); + else + JSCell::deleteProperty(target, globalObject, key); + RETURN_IF_EXCEPTION(scope, ); + } else if (!original && !isVariableOf(target, key)) { + // A lock that came after spyOn() is gone through, as spyOn() goes through one that came before: a direct store makes the property configurable again. + bool isDeleted; + { + ThroughLockOfBuiltins throughLock(vm, false); + putOwnPropertyStorage(globalObject, target, key, jsUndefined(), 0, false); + RETURN_IF_EXCEPTION(scope, ); + isDeleted = deleteOwnProperty(globalObject, target, key); + } + RETURN_IF_EXCEPTION(scope, ); + if (isDeleted) + return; + } + + // The traps of an opaque object can hide the spy too. + JSObject* holder = isOpaque ? objectBehindTraps(target) : target; + unsigned currentAttributes = 0; + JSValue current = getOwnPropertyStorage(globalObject, holder, key, currentAttributes); + RETURN_IF_EXCEPTION(scope, ); + if (!this->isInstalledAs(current)) + return; + + // A Proxy that keeps the spy takes the original the way it took the spy, through its defineProperty trap. + if (isInherited) { + defineOwnPropertyStorage(globalObject, target, key, original, attributes & descriptorAttributes, false); + RETURN_IF_EXCEPTION(scope, ); + current = getOwnPropertyStorage(globalObject, holder, key, currentAttributes); + RETURN_IF_EXCEPTION(scope, ); + if (!this->isInstalledAs(current)) + return; + } + + throwTypeError(globalObject, scope, makeString("Cannot restore the `"_s, propertyKeyForMessage(key), "` property because the object does not allow it to be changed"_s)); +} + static String propertyKeyForFunctionName(const Identifier& key) { return key.isSymbol() ? makeString('[', key.string(), ']') : key.string(); @@ -3179,7 +3282,12 @@ __attribute__((minsize)) static JSC::EncodedJSValue spyOn(JSC::JSGlobalObject* l RETURN_IF_EXCEPTION(scope, {}); } - mock->didInstall(object, propertyKey, isOwn ? value : JSValue(), spyAttributes); + JSValue original = isOwn ? value : JSValue(); + if (isOpaque && !isOwn && hasValue) [[unlikely]] { + original = value; + spyAttributes = (spyAttributes & ~PropertyAttribute::DontDelete) | JSMockFunction::SpyAttributeInherited; + } + mock->didInstall(object, propertyKey, original, spyAttributes); return JSValue::encode(mock); } @@ -3256,7 +3364,14 @@ __attribute__((minsize)) static JSC::EncodedJSValue spyOn(JSC::JSGlobalObject* l putOwnPropertyStorage(globalObject, object, propertyKey, JSC::GetterSetter::create(vm, globalObject, getter, setter), PropertyAttribute::Accessor | (descriptor.attributes() & PropertyAttribute::DontEnum), isOpaque); RETURN_IF_EXCEPTION(scope, {}); - mock->didInstall(object, propertyKey, storage, spyAttributes | JSMockFunction::SpyAttributeAccessor | (isOpaque ? JSMockFunction::SpyAttributeOpaque : 0)); + spyAttributes |= JSMockFunction::SpyAttributeAccessor | (isOpaque ? JSMockFunction::SpyAttributeOpaque : 0); + if (!storage && isOpaque) { + // A `has` trap can say yes to a key that no descriptor shows. + storage = descriptor.isAccessorDescriptor() ? JSValue(JSC::GetterSetter::create(vm, globalObject, descriptor.getter(), descriptor.setter())) : descriptor.value(); + if (storage) + spyAttributes |= (descriptor.attributes() & descriptorAttributes & ~PropertyAttribute::DontDelete) | JSMockFunction::SpyAttributeInherited; + } + mock->didInstall(object, propertyKey, storage, spyAttributes); return JSValue::encode(mock); } diff --git a/test/aot/a-spy-on-an-object-that-is-frozen-afterwards-is-restored.test.ts b/test/aot/a-spy-on-an-object-that-is-frozen-afterwards-is-restored.test.ts new file mode 100644 index 000000000000..9f2d21249ef4 --- /dev/null +++ b/test/aot/a-spy-on-an-object-that-is-frozen-afterwards-is-restored.test.ts @@ -0,0 +1,43 @@ +import { expect } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; +import { run } from "./harness/run"; +import { test } from "./supported"; + +// A key that only a method assigns has a place in every instance of the class, and a spy on that key goes into the place. +// Object.freeze() locks the place, and restoring the spy has to empty it again. +const testFile = ` +import { spyOn, test } from "bun:test"; +class Plain { method(): string { return "original"; } } +class Base { hook(): string { return "original"; } } +class Derived extends Base { + enable(): void { (this as any).hook = () => "enabled"; } +} +class Later { + enable(): void { (this as any).hook = () => "enabled"; } +} +test("a spy", () => { + const seen: unknown[] = []; + for (const [make, key] of [[() => new Plain(), "method"], [() => new Derived(), "hook"], [() => new Later(), "hook"]] as const) { + const object: any = make(); + const spy = spyOn(object, key); + Object.freeze(object); + spy.mockRestore(); + seen.push([typeof object[key] === "function" ? object[key]() : typeof object[key], Object.hasOwn(object, key), Object.isFrozen(object)]); + } + console.error("SEEN " + JSON.stringify(seen)); +}); +`; + +test("a spy on an object that is frozen afterwards is restored", async () => { + using dir = tempDir("aot-spy-on-a-frozen-object", { "a.test.ts": testFile }); + const seenWith = async (...flags: string[]) => { + const { stdout, stderr, exitCode } = await run([bunExe(), "test", ...flags], String(dir), bunEnv); + return { seen: (stdout + "\n" + stderr).split("\n").filter(line => line.startsWith("SEEN ")), exitCode }; + }; + const [plain, compiled] = await Promise.all([seenWith(), seenWith("--aot")]); + expect(plain).toEqual({ + seen: [`SEEN [["original",false,true],["original",false,true],["undefined",false,true]]`], + exitCode: 0, + }); + expect(compiled).toEqual(plain); +}); diff --git a/test/js/bun/test/mock-disposable.test.ts b/test/js/bun/test/mock-disposable.test.ts index 171ca28a1848..e137c405fc08 100644 --- a/test/js/bun/test/mock-disposable.test.ts +++ b/test/js/bun/test/mock-disposable.test.ts @@ -36,3 +36,21 @@ test("using with spyOn auto-restores prototype methods", () => { expect(new Greeter().greet()).toBe("hello"); }); + +test("using with spyOn auto-restores a method of an object that is frozen inside the block", () => { + class Greeter { + greet() { + return "hello"; + } + } + const greeter = new Greeter(); + + { + using spy = spyOn(greeter, "greet").mockReturnValue("hola"); + Object.freeze(greeter); + expect(greeter.greet()).toBe("hola"); + } + + expect(greeter.greet()).toBe("hello"); + expect(Object.hasOwn(greeter, "greet")).toBe(false); +}); diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index 76e14c699481..884bd7d16a62 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -5102,6 +5102,408 @@ if (isBun) { expect(Object.getOwnPropertyDescriptor(Response.prototype, "redirected")).toEqual(original); }); + describe("an object that is locked after spyOn()", () => { + const key = Symbol("method"); + const returned = () => "original"; + class Service { + static create() { + return "original"; + } + method() { + return "original"; + } + [key]() { + return "original"; + } + get value() { + return "original"; + } + set value(next) {} + get handler() { + return returned; + } + } + const locks = [ + ["Object.freeze()", object => Object.freeze(object), Object.isFrozen], + ["Object.seal()", object => Object.seal(object), Object.isSealed], + // Only the spy's property is locked, and restoring removes that property. + ["configurable: false", (object, name) => Object.defineProperty(object, name, { configurable: false }), Object.isExtensible], // prettier-ignore + ]; + const restores = [mocked => mocked.mockRestore(), () => jest.restoreAllMocks()]; + + describe.each(flavors)("%s", (_, isVitest, fn, spy) => { + test.each(locks)("%s: a spy on an inherited method is restored", (_, lock, isLocked) => { + for (const restore of restores) { + const object = new Service(); + const mocked = spy(object, "method").mockReturnValue("spied"); + lock(object, "method"); + expect(object.method()).toBe("spied"); + + restore(mocked); + expect(object.method).toBe(Service.prototype.method); + expect([Object.hasOwn(object, "method"), isLocked(object)]).toEqual([false, true]); + // It isn't installed any more, so there is nothing to restore. + mocked.mockRestore(); + expect(Object.hasOwn(object, "method")).toBe(false); + } + }); + + test("restoreAllMocks() takes the spy off a Proxy that doesn't delete it", () => { + const target = new Service(); + const proxy = new Proxy(target, { deleteProperty: () => false }); + spy(proxy, "method").mockReturnValue("spied"); + expect(proxy.method()).toBe("spied"); + jest.restoreAllMocks(); + expect(target.method).toBe(Service.prototype.method); + }); + }); + + test("getters, setters, symbol keys, missing keys, statics and a method that a getter returns", () => { + class Derived extends Service {} + const object = new Service(); + const spies = [ + spyOn(object, "value", "get").mockReturnValue("spied"), + spyOn(object, "value", "set"), + spyOn(object, key).mockReturnValue("spied"), + spyOn(object, "handler").mockReturnValue("spied"), + spyOn(object, "missing"), + spyOn(Derived, "create").mockReturnValue("spied"), + ]; + Object.freeze(object); + Object.freeze(Derived); + const read = () => [object.value, object[key](), object.handler(), "missing" in object, Derived.create()]; + expect(read()).toEqual(["spied", "spied", "spied", true, "spied"]); + + for (const mocked of spies) mocked.mockRestore(); + expect(read()).toEqual(["original", "original", "original", false, "original"]); + expect([Reflect.ownKeys(object), Object.hasOwn(Derived, "create")]).toEqual([[], false]); + expect([Object.isFrozen(object), Object.isFrozen(Derived)]).toEqual([true, true]); + }); + + test("a method that the prototype gets while the object is locked shows through", () => { + class Derived extends Service {} + const object = new Derived(); + const mocked = spyOn(object, "method"); + Object.freeze(object); + const later = () => "later"; + Derived.prototype.method = later; + mocked.mockRestore(); + expect(object.method).toBe(later); + }); + + test.each([ + ["the spy on the method first", (method, getter) => (method.mockRestore(), getter.mockRestore())], + ["the spy on the getter first", (method, getter) => (getter.mockRestore(), method.mockRestore())], + ["both with restoreAllMocks()", () => jest.restoreAllMocks()], + ])("a spy on the getter of a spied-on inherited method, restoring %s", (_, restore) => { + const object = new Service(); + const method = spyOn(object, "method"); + const getter = spyOn(object, "method", "get"); + Object.freeze(object); + expect(object.method).toBe(method); + expect(getter).toHaveBeenCalledTimes(1); + + restore(method, getter); + expect(object.method).toBe(Service.prototype.method); + expect(Object.hasOwn(object, "method")).toBe(false); + }); + + test("a property that replaced the spy is removed, whether or not it is configurable", () => { + for (const configurable of [true, false]) { + const object = new Service(); + const mocked = spyOn(object, "method"); + Object.defineProperty(object, "method", { value: () => "replacement", configurable }); + expect(object.method()).toBe("replacement"); + mocked.mockRestore(); + expect(object.method).toBe(Service.prototype.method); + expect(Object.hasOwn(object, "method")).toBe(false); + } + }); + + test("the global of a node:vm context", () => { + const context = vm.createContext(vm.constants.DONT_CONTEXTIFY); + const global = vm.runInContext("this", context); + const mocked = spyOn(global, "toString").mockReturnValue("spied"); + Object.defineProperty(global, "toString", { configurable: false }); + expect(vm.runInContext("toString()", context)).toBe("spied"); + mocked.mockRestore(); + expect(vm.runInContext("[toString(), Object.hasOwn(this, 'toString')]", context)).toEqual([ + "[object Undefined]", + false, + ]); + }); + + test("a variable that a script declares in the place of a deleted spy stays", () => { + const mocked = spyOn(globalThis, "declaredAfterTheSpy"); + delete globalThis.declaredAfterTheSpy; + vm.runInThisContext("var declaredAfterTheSpy = 'declared';"); + mocked.mockRestore(); + expect(Object.getOwnPropertyDescriptor(globalThis, "declaredAfterTheSpy")).toEqual({ + value: "declared", + writable: true, + enumerable: true, + configurable: false, + }); + }); + }); + + describe("a Proxy that doesn't delete a spy", () => { + const returned = () => "original"; + class Service { + method() { + return "original"; + } + get value() { + return "original"; + } + set value(next) {} + get handler() { + return returned; + } + } + Service.prototype.count = 1; + const kinds = [ + ["a method", "method", proxy => spyOn(proxy, "method")], + ["a getter", "value", proxy => spyOn(proxy, "value", "get")], + ["a setter", "value", proxy => spyOn(proxy, "value", "set")], + ["a method that a getter returns", "handler", proxy => spyOn(proxy, "handler")], + ["a value that is no function", "count", proxy => spyOn(proxy, "count")], + ]; + const cannotRestore = name => + new TypeError(`Cannot restore the \`${name}\` property because the object does not allow it to be changed`); + + test.each(kinds)("%s: the target gets the inherited property as its own", (_, name, spy) => { + // `true` is the answer of Vue's readonly(), which deletes nothing. + for (const answer of [false, true]) { + const target = new Service(); + const deleteProperty = jest.fn(() => answer); + const mocked = spy(new Proxy(target, { deleteProperty })); + mocked.mockRestore(); + expect(deleteProperty).toHaveBeenCalledTimes(1); + expect(Object.getOwnPropertyDescriptor(target, name)).toEqual( + Object.getOwnPropertyDescriptor(Service.prototype, name), + ); + // It isn't installed any more, so there is nothing to restore. + mocked.mockRestore(); + expect(deleteProperty).toHaveBeenCalledTimes(1); + } + }); + + test("a property that the prototype doesn't let go of comes back configurable", () => { + const frozen = Object.freeze({ method() {}, get value() {} }); // prettier-ignore + const target = Object.create(frozen); + const proxy = new Proxy(target, { deleteProperty: () => false }); + spyOn(proxy, "method").mockRestore(); + spyOn(proxy, "value", "get").mockRestore(); + const { method, value } = Object.getOwnPropertyDescriptors(frozen); + expect(Object.getOwnPropertyDescriptors(target)).toEqual({ + method: { ...method, configurable: true }, + value: { ...value, configurable: true }, + }); + }); + + test("the localStorage of happy-dom, which hides the bound copies of its methods and accessors", () => { + const target = new Service(); + for (const name of ["method", "value"]) { + const { value, get, set, ...attributes } = Object.getOwnPropertyDescriptor(Service.prototype, name); + Object.defineProperty(target, name, { + ...attributes, + ...(value ? { value: value.bind(target) } : { get: get.bind(target), set: set.bind(target) }), + }); + } + const bound = Object.getOwnPropertyDescriptors(target); + const storage = new Proxy(target, { deleteProperty: () => false, getOwnPropertyDescriptor: () => undefined }); + + const method = spyOn(storage, "method").mockReturnValue("spied"); + const getter = spyOn(storage, "value", "get").mockReturnValue("spied"); + expect([storage.method(), storage.value]).toEqual(["spied", "spied"]); + method.mockRestore(); + getter.mockRestore(); + expect(Object.getOwnPropertyDescriptor(target, "method")).toEqual(bound.method); + // The Proxy showed the accessor of the prototype, not the bound copy. + expect(Object.getOwnPropertyDescriptor(target, "value")).toEqual( + Object.getOwnPropertyDescriptor(Service.prototype, "value"), + ); + }); + + test.each([...kinds, ["a missing property", "missing", proxy => spyOn(proxy, "missing")]])( + "%s: a Proxy that takes nothing back keeps the spy, and restoring throws", + (_, name, spy) => { + const target = new Service(); + let keeps = false; + const proxy = new Proxy(target, { + deleteProperty: (target, name) => !keeps && Reflect.deleteProperty(target, name), + defineProperty: (target, name, descriptor) => !keeps && Reflect.defineProperty(target, name, descriptor), + }); + const mocked = spy(proxy); + const other = new Service(); + spyOn(other, "method"); + const installed = Object.getOwnPropertyDescriptor(target, name); + keeps = true; + try { + expect(() => mocked.mockRestore()).toThrow(cannotRestore(name)); + expect(() => jest.restoreAllMocks()).toThrow(cannotRestore(name)); + expect(Object.getOwnPropertyDescriptor(target, name)).toEqual(installed); + expect(Object.hasOwn(other, "method")).toBe(false); + } finally { + keeps = false; + jest.restoreAllMocks(); + } + expect(Object.hasOwn(target, name)).toBe(false); + }, + ); + + test.each([ + ["a missing property", {}, proxy => spyOn(proxy, "missing")], + ["a property that only the has trap knows", { has: () => true }, proxy => spyOn(proxy, "missing", "get")], + ])("%s has nothing that the target could get back", (_, traps, spy) => { + const target = new Service(); + let keeps = true; + const deleteProperty = (target, name) => !keeps && Reflect.deleteProperty(target, name); + const mocked = spy(new Proxy(target, { ...traps, deleteProperty })); + try { + expect(() => mocked.mockRestore()).toThrow(cannotRestore("missing")); + expect(Object.hasOwn(target, "missing")).toBe(true); + } finally { + keeps = false; + mocked.mockRestore(); + } + expect(Object.hasOwn(target, "missing")).toBe(false); + }); + + test("a trap that restores the spy itself, and then answers false", () => { + const target = new Service(); + let isRestoring = false; + const proxy = new Proxy(target, { + deleteProperty(target, name) { + if (isRestoring) return Reflect.deleteProperty(target, name); + isRestoring = true; + mocked.mockRestore(); + return false; + }, + }); + const mocked = spyOn(proxy, "method"); + mocked.mockRestore(); + expect(Object.hasOwn(target, "method")).toBe(false); + }); + + test("a spy that is gone when the trap answers false leaves the target as it is", () => { + const target = new Service(); + // Deletes the spy, and returns undefined. + const careless = new Proxy(target, { deleteProperty: (target, name) => void Reflect.deleteProperty(target, name) }); // prettier-ignore + spyOn(careless, "method").mockRestore(); + expect(Object.hasOwn(target, "method")).toBe(false); + + // User code put a property of its own in the place of the spy. + const replacement = () => "replacement"; + const mocked = spyOn(new Proxy(target, { deleteProperty: () => false }), "method"); + target.method = replacement; + mocked.mockRestore(); + expect(target.method).toBe(replacement); + }); + }); + + describe("spies that are left for the end of a test file", () => { + const runTests = async files => { + const { bunEnv, bunExe, tempDir } = require("harness"); + using dir = tempDir("spies-of-a-file", files); + const tests = Object.keys(files).filter(name => name.endsWith(".test.js")); + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", ...tests.map(name => "./" + name)], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const lines = stderr.split("\n").filter(line => /^(\w*Error: |\w\.test\.js:$|\((pass|fail)\) )/.test(line)); + return { lines: lines.map(line => line.replace(/ \[[\d.]+ms\]$/, "")), exitCode }; + }; + const cannotRestore = key => + `TypeError: Cannot restore the \`${key}\` property because the object does not allow it to be changed`; + + test("an object that doesn't delete the spy is restored for the next file", async () => { + const result = await runTests({ + "shared.js": ` + export class Shared { inherited() { return "original"; } } + export const frozen = new Shared(); + export const behindProxy = new Shared(); + export const proxy = new Proxy(behindProxy, { deleteProperty: () => false }); + `, + "a.test.js": ` + import { expect, spyOn, test } from "bun:test"; + import { frozen, proxy } from "./shared.js"; + test("a", () => { + spyOn(frozen, "inherited").mockReturnValue("spied"); + Object.freeze(frozen); + spyOn(proxy, "inherited").mockReturnValue("spied"); + expect([frozen.inherited(), proxy.inherited()]).toEqual(["spied", "spied"]); + }); + `, + "b.test.js": ` + import { expect, test } from "bun:test"; + import { Shared, behindProxy, frozen } from "./shared.js"; + test("b", () => { + expect([frozen.inherited(), Object.hasOwn(frozen, "inherited")]).toEqual(["original", false]); + expect(behindProxy.inherited).toBe(Shared.prototype.inherited); + }); + `, + }); + expect(result).toEqual({ lines: ["a.test.js:", "(pass) a", "b.test.js:", "(pass) b"], exitCode: 0 }); + }); + + test("an object that keeps the spy is reported once, under the file that made the spy", async () => { + const result = await runTests({ + "a.test.js": ` + import { expect, mock, spyOn, test } from "bun:test"; + test("a", () => { + const array = (globalThis.kept = [() => "original"]); + const spy = spyOn(array, 0); + Object.freeze(array); + for (const restore of [() => spy.mockRestore(), () => mock.restore()]) { + expect(restore).toThrow(new TypeError(${JSON.stringify(cannotRestore("0").slice("TypeError: ".length))})); + } + expect(array[0]).toBe(spy); + }); + `, + "b.test.js": ` + import { spyOn, test } from "bun:test"; + test("b", () => { + const object = (globalThis.kept = Object.create([() => "original"])); + spyOn(object, 0); + Object.freeze(object); + }); + `, + "c.test.js": ` + import { spyOn, test } from "bun:test"; + class Service { method() {} } + test("c", () => { + const proxy = (globalThis.kept = new Proxy(new Service(), {})); + spyOn(proxy, "method"); + Object.freeze(proxy); + }); + `, + "d.test.js": `import { test } from "bun:test"; test("d", () => {});`, + }); + expect(result).toEqual({ + lines: [ + "a.test.js:", + "(pass) a", + cannotRestore("0"), + "b.test.js:", + "(pass) b", + cannotRestore("0"), + "c.test.js:", + "(pass) c", + cannotRestore("method"), + "d.test.js:", + "(pass) d", + ], + exitCode: 1, + }); + }); + }); + test("vi.mockObject() of a prototype whose Symbol.toStringTag getter needs an instance", () => { class Tagged { #tag = "Tagged"; diff --git a/test/js/bun/test/vi-utils.test.ts b/test/js/bun/test/vi-utils.test.ts index f0dd70c59f99..ff970a4d7126 100644 --- a/test/js/bun/test/vi-utils.test.ts +++ b/test/js/bun/test/vi-utils.test.ts @@ -833,6 +833,7 @@ describe.concurrent("vi.setConfig", () => { ${config} const fn = vi.fn(() => "original"); const object = { method: () => "original" }; + const frozen = new (class { method() { return "original"; } })(); fn("top level"); beforeAll(() => { fn("beforeAll"); }); beforeEach(() => { fn("beforeEach"); }); @@ -840,11 +841,13 @@ describe.concurrent("vi.setConfig", () => { fn("first"); fn.mockReturnValue("changed"); vi.spyOn(object, "method").mockReturnValue("spied"); + vi.spyOn(frozen, "method").mockReturnValue("spied"); + Object.freeze(frozen); vi.stubEnv("STUBBED", "1"); vi.stubGlobal("stubbed", 1); }); test("second", () => { - console.log(JSON.stringify({ calls: fn.mock.calls.flat(), returns: fn(), method: object.method(), env: process.env.STUBBED, global: typeof stubbed })); + console.log(JSON.stringify({ calls: fn.mock.calls.flat(), returns: fn(), method: object.method(), frozen: [frozen.method(), Object.hasOwn(frozen, "method")], env: process.env.STUBBED, global: typeof stubbed })); }); `; const seen = async ( @@ -875,10 +878,14 @@ describe.concurrent("vi.setConfig", () => { calls: ["top level", "beforeAll", "beforeEach", "first", "beforeEach"], returns: "changed", method: "spied", + // An inherited method, and the object is frozen after the spy is made. + frozen: ["spied", true], env: "1", global: "number", }; const cleared = { calls: ["beforeEach"] }; + const reset = { returns: "original", method: "original", frozen: ["original", true] }; + const restored = { method: "original", frozen: ["original", false] }; test.each([ ["bun:test", nothing], @@ -891,13 +898,13 @@ describe.concurrent("vi.setConfig", () => { describe.each(["bunfig", "setConfig", "preload"] as const)("through %s", through => { test.each([ [{ clearMocks: true }, cleared], - [{ mockReset: true }, { ...cleared, returns: "original", method: "original" }], - [{ restoreMocks: true }, { method: "original" }], + [{ mockReset: true }, { ...cleared, ...reset }], + [{ restoreMocks: true }, restored], [{ unstubEnvs: true }, { env: undefined }], [{ unstubGlobals: true }, { global: "undefined" }], [ { clearMocks: true, mockReset: true, restoreMocks: true, unstubEnvs: true, unstubGlobals: true }, - { ...cleared, returns: "original", method: "original", env: undefined, global: "undefined" }, + { ...cleared, ...reset, ...restored, env: undefined, global: "undefined" }, ], ])("%j", async (options, difference) => { expect(await seen("bun:test", options, through)).toEqual({ ...nothing, ...difference }); From f785b677111fd99aa80789e94e24929b868949d3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 7 Nov 2026 06:45:26 +0000 Subject: [PATCH 2/3] bun:test: hold the Proxy of each spy test, and read no answer of a defineProperty trap A spy holds its object weakly. Two of the new tests gave spyOn() a Proxy that nothing else referenced. `expect()` collects garbage when BUN_GARBAGE_COLLECTOR_LEVEL is 1, as the CI runner sets it, so the Proxy was gone when mockRestore() ran, and mockRestore() had nothing to restore. Each test now holds its Proxy and reads through it after the restore. The restore of an own property through a Proxy read the answer of the defineProperty trap. A trap that put the original back and answered false made mockRestore() throw on each call. The answer is no longer read, as for the deleteProperty trap. The object behind the traps says if the spy is still there. Tests: - The TypeError of a kept spy is checked by name and message, for vi.spyOn(), jest.spyOn() and spyOn(). - An own method that the defineProperty trap keeps or puts back. - The restoreMocks option with an object that is frozen after the spy is a test of its own. The shared fixture of the option matrix is as on main again. --- src/jsc/bindings/JSMockFunction.cpp | 4 +- test/js/bun/test/mock-fn.test.js | 158 ++++++++++++++++++---------- test/js/bun/test/vi-utils.test.ts | 41 ++++++-- 3 files changed, 132 insertions(+), 71 deletions(-) diff --git a/src/jsc/bindings/JSMockFunction.cpp b/src/jsc/bindings/JSMockFunction.cpp index c5450c98f617..a2e6336ebb88 100644 --- a/src/jsc/bindings/JSMockFunction.cpp +++ b/src/jsc/bindings/JSMockFunction.cpp @@ -3012,9 +3012,9 @@ __attribute__((cold, minsize)) NEVER_INLINE void JSMockFunction::takeSpyOffOrThr bool isInherited = attributes & SpyAttributeInherited; if (isOpaque) { - // What a trap answers isn't read: it may say that it deleted what it kept, so the object is looked at behind its traps below. + // What a trap answers isn't read: it may say that it did what it didn't, so the object is looked at behind its traps below. if (original && !isInherited) - putOwnPropertyStorage(globalObject, target, key, original, attributes & ~(SpyAttributeAccessor | SpyAttributeOpaque), true); + defineOwnPropertyStorage(globalObject, target, key, original, attributes & descriptorAttributes, false); else JSCell::deleteProperty(target, globalObject, key); RETURN_IF_EXCEPTION(scope, ); diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index 884bd7d16a62..ad832e74ed7d 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -5154,7 +5154,9 @@ if (isBun) { spy(proxy, "method").mockReturnValue("spied"); expect(proxy.method()).toBe("spied"); jest.restoreAllMocks(); - expect(target.method).toBe(Service.prototype.method); + expect(Object.getOwnPropertyDescriptor(proxy, "method")).toEqual( + Object.getOwnPropertyDescriptor(Service.prototype, "method"), + ); }); }); @@ -5262,25 +5264,30 @@ if (isBun) { } } Service.prototype.count = 1; + // A spy keeps no object alive, so each test holds its Proxy until the spy is restored. const kinds = [ - ["a method", "method", proxy => spyOn(proxy, "method")], - ["a getter", "value", proxy => spyOn(proxy, "value", "get")], - ["a setter", "value", proxy => spyOn(proxy, "value", "set")], - ["a method that a getter returns", "handler", proxy => spyOn(proxy, "handler")], - ["a value that is no function", "count", proxy => spyOn(proxy, "count")], + ["a method", "method", (proxy, spy = spyOn) => spy(proxy, "method")], + ["a getter", "value", (proxy, spy = spyOn) => spy(proxy, "value", "get")], + ["a setter", "value", (proxy, spy = spyOn) => spy(proxy, "value", "set")], + ["a method that a getter returns", "handler", (proxy, spy = spyOn) => spy(proxy, "handler")], + ["a value that is no function", "count", (proxy, spy = spyOn) => spy(proxy, "count")], ]; + // toThrow() with an Error reads only its message. const cannotRestore = name => - new TypeError(`Cannot restore the \`${name}\` property because the object does not allow it to be changed`); + expect.objectContaining({ + name: "TypeError", + message: `Cannot restore the \`${name}\` property because the object does not allow it to be changed`, + }); test.each(kinds)("%s: the target gets the inherited property as its own", (_, name, spy) => { // `true` is the answer of Vue's readonly(), which deletes nothing. for (const answer of [false, true]) { - const target = new Service(); const deleteProperty = jest.fn(() => answer); - const mocked = spy(new Proxy(target, { deleteProperty })); + const proxy = new Proxy(new Service(), { deleteProperty }); + const mocked = spy(proxy); mocked.mockRestore(); expect(deleteProperty).toHaveBeenCalledTimes(1); - expect(Object.getOwnPropertyDescriptor(target, name)).toEqual( + expect(Object.getOwnPropertyDescriptor(proxy, name)).toEqual( Object.getOwnPropertyDescriptor(Service.prototype, name), ); // It isn't installed any more, so there is nothing to restore. @@ -5291,12 +5298,11 @@ if (isBun) { test("a property that the prototype doesn't let go of comes back configurable", () => { const frozen = Object.freeze({ method() {}, get value() {} }); // prettier-ignore - const target = Object.create(frozen); - const proxy = new Proxy(target, { deleteProperty: () => false }); + const proxy = new Proxy(Object.create(frozen), { deleteProperty: () => false }); spyOn(proxy, "method").mockRestore(); spyOn(proxy, "value", "get").mockRestore(); const { method, value } = Object.getOwnPropertyDescriptors(frozen); - expect(Object.getOwnPropertyDescriptors(target)).toEqual({ + expect(Object.getOwnPropertyDescriptors(proxy)).toEqual({ method: { ...method, configurable: true }, value: { ...value, configurable: true }, }); @@ -5319,6 +5325,7 @@ if (isBun) { expect([storage.method(), storage.value]).toEqual(["spied", "spied"]); method.mockRestore(); getter.mockRestore(); + expect([storage.method(), storage.value]).toEqual(["original", "original"]); expect(Object.getOwnPropertyDescriptor(target, "method")).toEqual(bound.method); // The Proxy showed the accessor of the prototype, not the bound copy. expect(Object.getOwnPropertyDescriptor(target, "value")).toEqual( @@ -5326,55 +5333,89 @@ if (isBun) { ); }); - test.each([...kinds, ["a missing property", "missing", proxy => spyOn(proxy, "missing")]])( - "%s: a Proxy that takes nothing back keeps the spy, and restoring throws", - (_, name, spy) => { - const target = new Service(); - let keeps = false; - const proxy = new Proxy(target, { - deleteProperty: (target, name) => !keeps && Reflect.deleteProperty(target, name), - defineProperty: (target, name, descriptor) => !keeps && Reflect.defineProperty(target, name, descriptor), - }); - const mocked = spy(proxy); - const other = new Service(); - spyOn(other, "method"); - const installed = Object.getOwnPropertyDescriptor(target, name); - keeps = true; - try { - expect(() => mocked.mockRestore()).toThrow(cannotRestore(name)); - expect(() => jest.restoreAllMocks()).toThrow(cannotRestore(name)); - expect(Object.getOwnPropertyDescriptor(target, name)).toEqual(installed); - expect(Object.hasOwn(other, "method")).toBe(false); - } finally { - keeps = false; - jest.restoreAllMocks(); - } - expect(Object.hasOwn(target, name)).toBe(false); - }, - ); + const missing = ["a missing property", "missing", (proxy, spy = spyOn) => spy(proxy, "missing")]; + describe.each(flavors)("%s", (_, isVitest, fn, spyOfFlavor) => { + test.each([...kinds, missing])( + "%s: a Proxy that takes nothing back keeps the spy, and restoring throws", + (_, name, spy) => { + let keeps = false; + const proxy = new Proxy(new Service(), { + deleteProperty: (target, name) => !keeps && Reflect.deleteProperty(target, name), + defineProperty: (target, name, descriptor) => !keeps && Reflect.defineProperty(target, name, descriptor), + }); + const mocked = spy(proxy, spyOfFlavor); + const other = new Service(); + spyOfFlavor(other, "method"); + const installed = Object.getOwnPropertyDescriptor(proxy, name); + keeps = true; + try { + expect(() => mocked.mockRestore()).toThrow(cannotRestore(name)); + expect(() => jest.restoreAllMocks()).toThrow(cannotRestore(name)); + expect(Object.getOwnPropertyDescriptor(proxy, name)).toEqual(installed); + expect(Object.hasOwn(other, "method")).toBe(false); + } finally { + keeps = false; + jest.restoreAllMocks(); + } + expect(Object.hasOwn(proxy, name)).toBe(false); + }, + ); + }); test.each([ ["a missing property", {}, proxy => spyOn(proxy, "missing")], ["a property that only the has trap knows", { has: () => true }, proxy => spyOn(proxy, "missing", "get")], ])("%s has nothing that the target could get back", (_, traps, spy) => { - const target = new Service(); let keeps = true; const deleteProperty = (target, name) => !keeps && Reflect.deleteProperty(target, name); - const mocked = spy(new Proxy(target, { ...traps, deleteProperty })); + const proxy = new Proxy(new Service(), { ...traps, deleteProperty }); + const mocked = spy(proxy); try { expect(() => mocked.mockRestore()).toThrow(cannotRestore("missing")); - expect(Object.hasOwn(target, "missing")).toBe(true); + expect(Object.hasOwn(proxy, "missing")).toBe(true); } finally { keeps = false; mocked.mockRestore(); } - expect(Object.hasOwn(target, "missing")).toBe(false); + expect(Object.hasOwn(proxy, "missing")).toBe(false); + }); + + test.each([true, false])("an own method that the defineProperty trap keeps, answering %s", answer => { + const original = () => "original"; + let keeps = false; + const proxy = new Proxy( + { method: original }, + { defineProperty: (target, name, descriptor) => (keeps ? answer : Reflect.defineProperty(target, name, descriptor)) }, // prettier-ignore + ); + const mocked = spyOn(proxy, "method"); + keeps = true; + try { + expect(() => mocked.mockRestore()).toThrow(cannotRestore("method")); + expect(() => jest.restoreAllMocks()).toThrow(cannotRestore("method")); + expect(proxy.method).toBe(mocked); + } finally { + keeps = false; + jest.restoreAllMocks(); + } + expect(proxy.method).toBe(original); + }); + + test("an own method that the defineProperty trap puts back, answering false", () => { + const original = () => "original"; + let answer = true; + const proxy = new Proxy( + { method: original }, + { defineProperty: (target, name, descriptor) => Reflect.defineProperty(target, name, descriptor) && answer }, + ); + const mocked = spyOn(proxy, "method"); + answer = false; + mocked.mockRestore(); + expect(proxy.method).toBe(original); }); test("a trap that restores the spy itself, and then answers false", () => { - const target = new Service(); let isRestoring = false; - const proxy = new Proxy(target, { + const proxy = new Proxy(new Service(), { deleteProperty(target, name) { if (isRestoring) return Reflect.deleteProperty(target, name); isRestoring = true; @@ -5384,22 +5425,22 @@ if (isBun) { }); const mocked = spyOn(proxy, "method"); mocked.mockRestore(); - expect(Object.hasOwn(target, "method")).toBe(false); + expect(Object.hasOwn(proxy, "method")).toBe(false); }); test("a spy that is gone when the trap answers false leaves the target as it is", () => { - const target = new Service(); // Deletes the spy, and returns undefined. - const careless = new Proxy(target, { deleteProperty: (target, name) => void Reflect.deleteProperty(target, name) }); // prettier-ignore + const careless = new Proxy(new Service(), { deleteProperty: (target, name) => void Reflect.deleteProperty(target, name) }); // prettier-ignore spyOn(careless, "method").mockRestore(); - expect(Object.hasOwn(target, "method")).toBe(false); + expect(Object.hasOwn(careless, "method")).toBe(false); // User code put a property of its own in the place of the spy. const replacement = () => "replacement"; - const mocked = spyOn(new Proxy(target, { deleteProperty: () => false }), "method"); - target.method = replacement; + const refusing = new Proxy(new Service(), { deleteProperty: () => false }); + const mocked = spyOn(refusing, "method"); + refusing.method = replacement; mocked.mockRestore(); - expect(target.method).toBe(replacement); + expect(refusing.method).toBe(replacement); }); }); @@ -5419,8 +5460,9 @@ if (isBun) { const lines = stderr.split("\n").filter(line => /^(\w*Error: |\w\.test\.js:$|\((pass|fail)\) )/.test(line)); return { lines: lines.map(line => line.replace(/ \[[\d.]+ms\]$/, "")), exitCode }; }; - const cannotRestore = key => - `TypeError: Cannot restore the \`${key}\` property because the object does not allow it to be changed`; + const message = key => + `Cannot restore the \`${key}\` property because the object does not allow it to be changed`; + const reported = key => "TypeError: " + message(key); test("an object that doesn't delete the spy is restored for the next file", async () => { const result = await runTests({ @@ -5461,7 +5503,7 @@ if (isBun) { const spy = spyOn(array, 0); Object.freeze(array); for (const restore of [() => spy.mockRestore(), () => mock.restore()]) { - expect(restore).toThrow(new TypeError(${JSON.stringify(cannotRestore("0").slice("TypeError: ".length))})); + expect(restore).toThrow(expect.objectContaining({ name: "TypeError", message: ${JSON.stringify(message("0"))} })); } expect(array[0]).toBe(spy); }); @@ -5489,13 +5531,13 @@ if (isBun) { lines: [ "a.test.js:", "(pass) a", - cannotRestore("0"), + reported("0"), "b.test.js:", "(pass) b", - cannotRestore("0"), + reported("0"), "c.test.js:", "(pass) c", - cannotRestore("method"), + reported("method"), "d.test.js:", "(pass) d", ], diff --git a/test/js/bun/test/vi-utils.test.ts b/test/js/bun/test/vi-utils.test.ts index ff970a4d7126..7ad618fd39d3 100644 --- a/test/js/bun/test/vi-utils.test.ts +++ b/test/js/bun/test/vi-utils.test.ts @@ -833,7 +833,6 @@ describe.concurrent("vi.setConfig", () => { ${config} const fn = vi.fn(() => "original"); const object = { method: () => "original" }; - const frozen = new (class { method() { return "original"; } })(); fn("top level"); beforeAll(() => { fn("beforeAll"); }); beforeEach(() => { fn("beforeEach"); }); @@ -841,13 +840,11 @@ describe.concurrent("vi.setConfig", () => { fn("first"); fn.mockReturnValue("changed"); vi.spyOn(object, "method").mockReturnValue("spied"); - vi.spyOn(frozen, "method").mockReturnValue("spied"); - Object.freeze(frozen); vi.stubEnv("STUBBED", "1"); vi.stubGlobal("stubbed", 1); }); test("second", () => { - console.log(JSON.stringify({ calls: fn.mock.calls.flat(), returns: fn(), method: object.method(), frozen: [frozen.method(), Object.hasOwn(frozen, "method")], env: process.env.STUBBED, global: typeof stubbed })); + console.log(JSON.stringify({ calls: fn.mock.calls.flat(), returns: fn(), method: object.method(), env: process.env.STUBBED, global: typeof stubbed })); }); `; const seen = async ( @@ -878,14 +875,10 @@ describe.concurrent("vi.setConfig", () => { calls: ["top level", "beforeAll", "beforeEach", "first", "beforeEach"], returns: "changed", method: "spied", - // An inherited method, and the object is frozen after the spy is made. - frozen: ["spied", true], env: "1", global: "number", }; const cleared = { calls: ["beforeEach"] }; - const reset = { returns: "original", method: "original", frozen: ["original", true] }; - const restored = { method: "original", frozen: ["original", false] }; test.each([ ["bun:test", nothing], @@ -898,13 +891,13 @@ describe.concurrent("vi.setConfig", () => { describe.each(["bunfig", "setConfig", "preload"] as const)("through %s", through => { test.each([ [{ clearMocks: true }, cleared], - [{ mockReset: true }, { ...cleared, ...reset }], - [{ restoreMocks: true }, restored], + [{ mockReset: true }, { ...cleared, returns: "original", method: "original" }], + [{ restoreMocks: true }, { method: "original" }], [{ unstubEnvs: true }, { env: undefined }], [{ unstubGlobals: true }, { global: "undefined" }], [ { clearMocks: true, mockReset: true, restoreMocks: true, unstubEnvs: true, unstubGlobals: true }, - { ...cleared, ...reset, ...restored, env: undefined, global: "undefined" }, + { ...cleared, returns: "original", method: "original", env: undefined, global: "undefined" }, ], ])("%j", async (options, difference) => { expect(await seen("bun:test", options, through)).toEqual({ ...nothing, ...difference }); @@ -923,6 +916,32 @@ describe.concurrent("vi.setConfig", () => { }); }); + test("restoreMocks takes a spy on an inherited method off an object that is frozen afterwards", async () => { + const { stdout, stderr, exitCode } = await run(["test"], { + "a.test.ts": ` + import { test, vi } from "bun:test"; + vi.setConfig({ restoreMocks: true }); + const frozen = new (class { method() { return "original"; } })(); + const seen = () => console.log(JSON.stringify([frozen.method(), Object.hasOwn(frozen, "method")])); + test("first", () => { + vi.spyOn(frozen, "method").mockReturnValue("spied"); + Object.freeze(frozen); + seen(); + }); + test("second", seen); + `, + }); + expect({ + stdout: stdout.split("\n").filter(line => line.startsWith("[")), + stderr: results(stderr), + exitCode, + }).toEqual({ + stdout: [`["spied",true]`, `["original",false]`], + stderr: ["(pass) first", "(pass) second"], + exitCode: 0, + }); + }); + test("apply to every attempt but not to skipped tests, until resetConfig() or the file ends", async () => { const { stdout, stderr, exitCode } = await run(["test", "./a.test.ts", "./b.test.ts"], { "shared.ts": `import { vi } from "bun:test"; export const fn = vi.fn();`, From b6fbeb6618a0bb04d76a9927e793c65009936449 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 7 Nov 2026 07:16:40 +0000 Subject: [PATCH 3/3] bun:test: keep the variable test off the shared global, and match the class of the TypeError The test of a variable that a script declares in the place of a deleted spy declared it on the global object of the test run. A variable of a global object can't be deleted, so each later test file of the same process saw it. The test now uses the global of a node:vm context. Without the isVariableOf() guard it still fails. The tests of a kept spy matched the name of the error. They now match its constructor, so an Error that is named TypeError does not pass. --- test/js/bun/test/mock-fn.test.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index ad832e74ed7d..9eb8ae355974 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -5236,11 +5236,14 @@ if (isBun) { }); test("a variable that a script declares in the place of a deleted spy stays", () => { - const mocked = spyOn(globalThis, "declaredAfterTheSpy"); - delete globalThis.declaredAfterTheSpy; - vm.runInThisContext("var declaredAfterTheSpy = 'declared';"); + // A variable of a global object can't be deleted again, so the global is one that ends with the test. + const context = vm.createContext(vm.constants.DONT_CONTEXTIFY); + const global = vm.runInContext("this", context); + const mocked = spyOn(global, "declaredAfterTheSpy"); + delete global.declaredAfterTheSpy; + vm.runInContext("var declaredAfterTheSpy = 'declared';", context); mocked.mockRestore(); - expect(Object.getOwnPropertyDescriptor(globalThis, "declaredAfterTheSpy")).toEqual({ + expect(Object.getOwnPropertyDescriptor(global, "declaredAfterTheSpy")).toEqual({ value: "declared", writable: true, enumerable: true, @@ -5275,7 +5278,7 @@ if (isBun) { // toThrow() with an Error reads only its message. const cannotRestore = name => expect.objectContaining({ - name: "TypeError", + constructor: TypeError, message: `Cannot restore the \`${name}\` property because the object does not allow it to be changed`, }); @@ -5503,7 +5506,7 @@ if (isBun) { const spy = spyOn(array, 0); Object.freeze(array); for (const restore of [() => spy.mockRestore(), () => mock.restore()]) { - expect(restore).toThrow(expect.objectContaining({ name: "TypeError", message: ${JSON.stringify(message("0"))} })); + expect(restore).toThrow(expect.objectContaining({ constructor: TypeError, message: ${JSON.stringify(message("0"))} })); } expect(array[0]).toBe(spy); });