From f4bdf576d4ccf633e97e5044510ecbabf77aab8e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 6 Nov 2026 20:24:54 +0000 Subject: [PATCH 1/6] bun test: keep mocks in ordered weak lists, so spies are restored in the order they were made forEachIn walked a weak hash set keyed by cell address. The allocator picked the order of each restoreAllMocks(), each resetAllMocks() and each end-of-file restore. WeakMockList keeps the mocks in the order they entered. The collector removes the dead ones at the end of each collection. A spy takes its place when it is made, and a spy whose restore throws keeps its place. A pass that is terminated leaves the mocks it did not reach in the list. --- src/jsc/bindings/JSMockFunction.cpp | 155 +++++++---- src/jsc/bindings/JSMockFunction.h | 15 +- test/js/bun/test/mock-fn.test.js | 382 ++++++++++++++++++++++++++++ 3 files changed, 492 insertions(+), 60 deletions(-) diff --git a/src/jsc/bindings/JSMockFunction.cpp b/src/jsc/bindings/JSMockFunction.cpp index be3854db4664..5ac90a3bcfbb 100644 --- a/src/jsc/bindings/JSMockFunction.cpp +++ b/src/jsc/bindings/JSMockFunction.cpp @@ -19,8 +19,7 @@ #include #include #include -#include -#include +#include #include #include #include @@ -131,37 +130,61 @@ JSC_DECLARE_HOST_FUNCTION(jsMockFunctionThrowThis); uint64_t JSMockModule::s_nextInvocationId = 0; -// This is taken from JSWeakSet -// We only want to hold onto the list of active spies which haven't already been collected -// So we use a WeakSet -// Unlike using WeakSet from JS, we are able to iterate through the WeakSet. -class ActiveSpySet final : public WeakMapImpl> { +// The mocks that clearAllMocks(), resetAllMocks() or restoreAllMocks() has to visit, in the order they entered. +// The list doesn't keep them alive: the collector takes out the dead ones at the end of every collection. +class WeakMockList final : public JSC::WeakGCHashTable { + WTF_DEPRECATED_MAKE_FAST_ALLOCATED(WeakMockList); + WTF_MAKE_NONCOPYABLE(WeakMockList); + public: - using Base = WeakMapImpl>; + explicit WeakMockList(VM& vm) + : m_vm(vm) + { + vm.heap.registerWeakGCHashTable(this); + } - DECLARE_EXPORT_INFO; + ~WeakMockList() final + { + m_vm.heap.unregisterWeakGCHashTable(this); + } - static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype) + bool isEmpty() const { return m_mocks.isEmpty(); } + + void append(JSMockFunction* mock) { - return Bun::createClassStructure(vm, globalObject, prototype, JSC::TypeInfo(JSWeakSetType, StructureFlags), info()); + m_mocks.append(mock); + markDirty(m_vm); } - static ActiveSpySet* create(VM& vm, Structure* structure) + // Puts `mocks` in front of the ones in the list. + NEVER_INLINE void prepend(std::span mocks) { - ActiveSpySet* instance = new (NotNull, allocateCell(vm)) ActiveSpySet(vm, structure); - instance->finishCreation(vm); - return instance; + m_mocks.insertSpan(0, mocks); + markDirty(m_vm); + } + + // Empties the list into `mocks`, which keeps them alive. + NEVER_INLINE void moveTo(MarkedVector& mocks) + { + mocks.ensureCapacity(m_mocks.size()); + for (auto* mock : m_mocks) + mocks.append(mock); + m_mocks.shrink(0); } private: - ActiveSpySet(VM& vm, Structure* structure) - : Base(vm, structure) + void reconcileWeakReferencesAtGCEnd(VM& vm, CollectionScope) final { + m_mocks.removeAllMatching([&](JSMockFunction* mock) { + return !vm.heap.isMarked(mock); + }); + if (m_mocks.size() <= m_mocks.capacity() / 4) + m_mocks.shrinkToFit(); } -}; -static_assert(std::is_final::value, "Required for JSType based casting"); -const ClassInfo ActiveSpySet::s_info = { "ActiveSpySet"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(ActiveSpySet) }; + Vector m_mocks; + VM& m_vm; +}; class JSMockImplementation final : public JSNonFinalObject { public: @@ -884,6 +907,9 @@ class JSMockFunction final : public JSC::InternalFunction { didUninstall(); } + // Spies are restored in the order they were made. Call it before wrapFunction() and the store of the spy, which can both run script that makes another spy. + void willInstall() { takePlaceIn(List::Spies); } + static void clearAll(Zig::GlobalObject*); static void resetAll(Zig::GlobalObject*); static void restoreAll(Zig::GlobalObject*); @@ -909,7 +935,7 @@ class JSMockFunction final : public JSC::InternalFunction { Spies = 1 << 2, }; - static JSC::WriteBarrier& setOf(JSMockModule& mockModule, List list) + static std::unique_ptr& listOf(JSMockModule& mockModule, List list) { switch (list) { case List::Called: @@ -922,6 +948,9 @@ class JSMockFunction final : public JSC::InternalFunction { RELEASE_ASSERT_NOT_REACHED(); } + // The bit of `m_lists` that says the list, or the forEachIn() that runs over it, has an entry for this mock. + static constexpr uint8_t hasEntryIn(List list) { return static_cast(list) << 3; } + template static void forEachIn(Zig::GlobalObject*, List, const Functor&); ALWAYS_INLINE void enter(List list) @@ -932,15 +961,23 @@ class JSMockFunction final : public JSC::InternalFunction { NEVER_INLINE void enterSlow(List list) { - auto& vm = this->vm(); - auto& set = setOf(mockModule(), list); - if (!set) - set.set(vm, globalObject(), ActiveSpySet::create(vm, mockModule().activeSpySetStructure.getInitializedOnMainThread(globalObject()))); - uncheckedDowncast(set.get())->add(vm, this, this); + takePlaceIn(list); m_lists |= static_cast(list); } - // Only clears the flag. The set keeps a stale entry, which forEachIn() skips, because removing one costs a rehash. + // A mock has one entry, and keeps it when it leaves, so it has the same place if it enters again. + void takePlaceIn(List list) + { + if (m_lists & hasEntryIn(list)) + return; + auto& mocks = listOf(mockModule(), list); + if (!mocks) + mocks = makeUnique(vm()); + mocks->append(this); + m_lists |= hasEntryIn(list); + } + + // Only clears the flag. The list keeps a stale entry, which forEachIn() skips, because finding it to take it out costs a search. void leave(List list) { m_lists &= ~static_cast(list); @@ -1155,34 +1192,42 @@ void JSMockFunction::didInstall(JSObject* target, const Identifier& key, JSValue const ClassInfo JSMockFunctionPrototype::s_info = { "Mock"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSMockFunctionPrototype) }; -// Detaches the set first, which drops its stale entries and lets script that `apply` runs register mocks in a new set. -// Every mock leaves, so `apply` must enter() the ones that stay. Keeps going after an exception and rethrows the first one at the end. +// Visits the mocks of a list in the order they entered it. Keeps going after an exception and rethrows the first one at the end. +// Every mock leaves first, so `apply` must enter() the ones that stay. Those keep their places, ahead of the mocks that script makes enter during the pass. +// A pass that script starts from inside `apply` finds none of the mocks that this pass has yet to visit, nor the one it is visiting. template void JSMockFunction::forEachIn(Zig::GlobalObject* globalObject, List list, const Functor& apply) { auto scope = DECLARE_THROW_SCOPE(globalObject->vm()); - auto& set = setOf(globalObject->mockModule, list); - if (!set) + auto* entries = listOf(globalObject->mockModule, list).get(); + if (!entries || entries->isEmpty()) return; - MarkedArgumentBuffer mocks; - uncheckedDowncast(set.get())->takeSnapshot(mocks); - set.clear(); + MarkedVector mocks; + entries->moveTo(mocks); + size_t staying = 0; + bool isTerminating = false; JSC::Exception* firstException = nullptr; - for (size_t i = 0; i < mocks.size(); ++i) { - auto* mock = uncheckedDowncast(mocks.at(i)); - if (!(mock->m_lists & static_cast(list))) - continue; - mock->leave(list); - apply(mock); - if (auto* exception = scope.exception()) [[unlikely]] { - if (!scope.tryClearException()) - return; - if (!firstException) - firstException = exception; + for (JSMockFunction* mock : mocks) { + // A termination ends the pass. The mocks it did not get to stay in the list. + if (!isTerminating && (mock->m_lists & static_cast(list))) { + mock->leave(list); + apply(mock); + if (auto* exception = scope.exception()) [[unlikely]] { + if (!scope.tryClearException()) + isTerminating = true; + else if (!firstException) + firstException = exception; + } } + if (mock->m_lists & static_cast(list)) + mocks.at(staying++) = mock; + else + mock->m_lists &= ~hasEntryIn(list); } - if (firstException) + if (staying) + entries->prepend(mocks.mutableSpan().first(staying)); + if (firstException && !isTerminating) throwException(globalObject, scope, firstException); } @@ -1215,7 +1260,8 @@ __attribute__((minsize)) void JSMockFunction::didFinishTestFile(Zig::GlobalObjec MarkedArgumentBuffer spiesOfPreload; JSC::Exception* firstException = nullptr; // Loop, because a Proxy trap hit while restoring one spy may install another. - while (globalObject->mockModule.activeSpies) { + auto& spies = globalObject->mockModule.activeSpies; + while (spies && !spies->isEmpty()) { forEachIn(globalObject, List::Spies, [&](JSMockFunction* spy) { if (spy->isMadeByPreload) { spiesOfPreload.append(spy); @@ -1272,6 +1318,11 @@ extern "C" void JSMock__didFinishTestFile(Zig::GlobalObject* globalObject) JSMockFunction::didFinishTestFile(globalObject); } +JSMockModule::JSMockModule() = default; +JSMockModule::JSMockModule(JSMockModule&&) = default; +JSMockModule& JSMockModule::operator=(JSMockModule&&) = default; +JSMockModule::~JSMockModule() = default; + JSMockModule JSMockModule::create(JSC::JSGlobalObject* globalObject) { JSMockModule mock; @@ -1308,11 +1359,6 @@ JSMockModule JSMockModule::create(JSC::JSGlobalObject* globalObject) init.set(structure); }); - mock.activeSpySetStructure.initLater([](const JSC::LazyProperty::Initializer& init) { - Structure* implementation = ActiveSpySet::createStructure(init.vm, init.owner, jsNull()); - init.set(implementation); - }); - mock.mockModuleStructure.initLater( [](const JSC::LazyProperty::Initializer& init) { Structure* implementation = createModuleMockStructure(init.vm, init.owner, jsNull()); @@ -1418,9 +1464,6 @@ template void JSMockModule::visit(Visitor& visitor) name.visit(visitor); FOR_EACH_JSMOCKMODULE_GC_MEMBER(VISIT_JSMOCKMODULE_GC_MEMBER) #undef VISIT_JSMOCKMODULE_GC_MEMBER - visitor.append(activeSpies); - visitor.append(calledMocks); - visitor.append(configuredMocks); visitor.append(stubbedEnvs); visitor.append(stubbedGlobals); visitor.append(envsOfPreload); @@ -3146,6 +3189,7 @@ __attribute__((minsize)) static JSC::EncodedJSValue spyOn(JSC::JSGlobalObject* l } auto* mock = createMockFunction(vm, globalObject, isVitest); + mock->willInstall(); unsigned spyAttributes = slotAttributes; // A spy that shadows an inherited property is deleted on restore, so it has to be configurable. unsigned attributes = isOwn ? spyAttributes : spyAttributes & ~PropertyAttribute::DontDelete; @@ -3227,6 +3271,7 @@ __attribute__((minsize)) static JSC::EncodedJSValue spyOn(JSC::JSGlobalObject* l } auto* mock = createMockFunction(vm, globalObject, isVitest); + mock->willInstall(); if (original.isCallable()) { wrapFunction(globalObject, mock, original); } else if (access == SpyAccess::Get && descriptor.isDataDescriptor()) { diff --git a/src/jsc/bindings/JSMockFunction.h b/src/jsc/bindings/JSMockFunction.h index cb3e034016fd..081565f19949 100644 --- a/src/jsc/bindings/JSMockFunction.h +++ b/src/jsc/bindings/JSMockFunction.h @@ -17,6 +17,7 @@ using namespace JSC; using namespace WebCore; class JSMockFunction; +class WeakMockList; // Automocks a module's exports. With `spy` the mocks call through, and nested objects are mutated in place unless `leaveOriginalAlone`. JSC::JSValue mockObject(Zig::GlobalObject*, JSC::JSValue exports, bool spy, bool leaveOriginalAlone = false); @@ -28,6 +29,11 @@ void didStartDynamicImport(Zig::GlobalObject*, JSC::JSPromise* import, const Str // Wrapper to scope a bunch of GlobalObject properties related to mocks class JSMockModule final { public: + JSMockModule(); + JSMockModule(JSMockModule&&); + JSMockModule& operator=(JSMockModule&&); + ~JSMockModule(); + static uint64_t s_nextInvocationId; static uint64_t nextInvocationId() { return ++s_nextInvocationId; } @@ -37,7 +43,6 @@ class JSMockModule final { V(Structure, mockImplementationStructure) \ V(Structure, mockObjectStructure) \ V(Structure, mockModuleStructure) \ - V(Structure, activeSpySetStructure) \ V(JSFunction, withImplementationCleanupFunction) \ V(JSC::Structure, mockWithImplementationCleanupDataStructure) \ V(JSCell, lazyPrototype) \ @@ -51,12 +56,12 @@ class JSMockModule final { static JSMockModule create(JSC::JSGlobalObject*); - // Weak sets (ActiveSpySet), created on first use. - JSC::WriteBarrier activeSpies; + // Spies that are installed. Each of these three lists is created on first use. + std::unique_ptr activeSpies; // Mocks that were called, or had `.mock` read, since they were last cleared. - JSC::WriteBarrier calledMocks; + std::unique_ptr calledMocks; // Mocks whose implementation or mock name may differ from what mockReset() gives them. - JSC::WriteBarrier configuredMocks; + std::unique_ptr configuredMocks; // JSMaps from name to original value for `vi.stubEnv()` / `vi.stubGlobal()`, created on first use. JSC::WriteBarrier stubbedEnvs; diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index 76e14c699481..2f2310a06741 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -5024,6 +5024,82 @@ if (isBun) { expect(reads).toBe(1); }); + // Resetting a mock reads the `prototype` of its implementation, which calls `onReset`. + const resettable = onReset => { + const mocked = vi.fn( + new Proxy(function () {}, { + get(target, key, receiver) { + if (key === "prototype") onReset(); + return Reflect.get(target, key, receiver); + }, + }), + ); + mocked.prototype; + return mocked; + }; + + test("resetAllMocks() resets the mocks in the order they were configured", () => { + const names = ["a", "b", "c", "d", "e", "f", "g", "h"]; + const reset = []; + const mocks = names.map(name => resettable(() => reset.push(name)).mockReturnValue("configured")); + reset.length = 0; + vi.resetAllMocks(); + expect(reset).toEqual(names); + expect(mocks.map(mocked => mocked())).toEqual(names.map(() => undefined)); + }); + + test("resetAllMocks() resets a mock once, also if it was reset and configured again before", () => { + let resets = 0; + let configuresAgain = false; + const mocked = resettable(() => { + resets++; + if (configuresAgain) mocked.mockReturnValue("configured again"); + }); + for (let i = 0; i < 3; i++) mocked.mockReturnValue(i).mockReset(); + mocked.mockReturnValue("configured"); + resets = 0; + configuresAgain = true; + vi.resetAllMocks(); + configuresAgain = false; + expect([resets, mocked()]).toEqual([1, "configured again"]); + vi.resetAllMocks(); + expect([resets, mocked()]).toEqual([2, undefined]); + }); + + test("a resetAllMocks() that is terminated leaves the mocks it did not reach to the next one", () => { + const names = ["a", "b", "c", "d", "e", "f"]; + const reset = []; + let phase = "configure"; + let isSpinning = false; + const mocks = names.map(name => + resettable(() => { + if (phase === "configure") return; + if (phase === "spin" && name === "c") { + isSpinning = true; + for (;;); + } + reset.push(name); + }).mockReturnValue("configured"), + ); + globalThis.resetAllMocksUntilTerminated = () => vi.resetAllMocks(); + try { + phase = "spin"; + // The timeout of node:vm terminates the call. If it comes before `c` spins, try a longer one. + for (let timeout = 50; !isSpinning; timeout *= 2) { + expect(() => vm.runInThisContext("resetAllMocksUntilTerminated()", { timeout })).toThrow( + expect.objectContaining({ code: "ERR_SCRIPT_EXECUTION_TIMEOUT" }), + ); + } + } finally { + phase = "reset"; + reset.length = 0; + vi.resetAllMocks(); + delete globalThis.resetAllMocksUntilTerminated; + } + expect(reset).toEqual(["d", "e", "f"]); + expect(mocks.map(mocked => mocked())).toEqual(names.map(() => undefined)); + }); + test("clearAllMocks() skips a mock that hasn't been used since the last clearAllMocks()", () => { const mocked = vi.fn(); const state = mocked.mock; @@ -5090,6 +5166,312 @@ if (isBun) { expect(other.method()).toBe("original"); expect(jest.isMockFunction(other.method)).toBe(false); }); + + // Installing a spy and restoring it both define the property, which calls `onDefine`. + const trapped = (properties, onDefine) => + new Proxy(properties, { + defineProperty(target, key, descriptor) { + onDefine(key); + return Reflect.defineProperty(target, key, descriptor); + }, + }); + // A spy is only restored while its object is alive, so the tests read their objects after the restore. + const spied = (objects, key = "method") => + objects.filter(object => jest.isMockFunction(Object.getOwnPropertyDescriptor(object, key)?.value)); + + test.each(flavors)("%s: restores the spies in the order they were made", (_, isVitest, fn, spy) => { + const keys = ["a", "b", "c", "d", "e", "f", "g", "h"]; + const restored = []; + let refuses = false; + const proxy = trapped(Object.fromEntries(keys.map(key => [key, () => {}])), key => { + if (!refuses) return; + restored.push(key); + throw new Error(`refuses ${key}`); + }); + for (const key of keys) spy(proxy, key); + + refuses = true; + try { + expect(() => jest.restoreAllMocks()).toThrow(new Error("refuses a")); + expect(restored).toEqual(keys); + // A spy that stays keeps its place. + expect(() => jest.restoreAllMocks()).toThrow(new Error("refuses a")); + expect(restored).toEqual([...keys, ...keys]); + } finally { + refuses = false; + jest.restoreAllMocks(); + } + expect(keys.filter(key => jest.isMockFunction(proxy[key]))).toEqual([]); + }); + + test.each(flavors)( + "%s: two spies on one property put back their originals in that order", + (_, isVitest, fn, spy) => { + const first = () => "first"; + const second = () => "second"; + const objects = Array.from({ length: 16 }, () => ({ method: first })); + for (const object of objects) { + spy(object, "method"); + object.method = second; + spy(object, "method"); + } + jest.restoreAllMocks(); + expect(objects.map(object => object.method())).toEqual(objects.map(() => "second")); + }, + ); + + test.each(flavors)( + "%s: a spy made while another one is installed is restored after it", + (_, isVitest, fn, spy) => { + const restored = []; + const expected = []; + const proxies = []; + let isRestoring = false; + for (let i = 0; i < 6; i++) { + const inner = trapped({ method() {} }, () => isRestoring && restored.push(`inner ${i}`)); + const outer = trapped({ method() {} }, () => { + if (isRestoring) restored.push(`outer ${i}`); + else spy(inner, "method"); + }); + spy(outer, "method"); + proxies.push(outer, inner); + expected.push(`outer ${i}`, `inner ${i}`); + } + isRestoring = true; + jest.restoreAllMocks(); + expect(restored).toEqual(expected); + expect(spied(proxies)).toEqual([]); + }, + ); + + test.each(flavors)( + "%s: a spy made while a getter spy is installed is restored after it", + (_, isVitest, fn, spy) => { + const restored = []; + const expected = []; + const proxies = []; + let isRestoring = false; + for (let i = 0; i < 6; i++) { + const inner = trapped({ method() {} }, () => isRestoring && restored.push(`inner ${i}`)); + const outer = trapped({ get value() {} }, () => { + if (isRestoring) restored.push(`outer ${i}`); + else spy(inner, "method"); + }); + spy(outer, "value", "get"); + proxies.push(outer, inner); + expected.push(`outer ${i}`, `inner ${i}`); + } + isRestoring = true; + jest.restoreAllMocks(); + expect(restored).toEqual(expected); + expect(spied(proxies)).toEqual([]); + }, + ); + + test.each(flavors)( + "%s: a spy made while another one copies its original is restored after it", + (_, isVitest, fn, spy) => { + const second = () => "second"; + // The spy that is made first replaces the other one, which is then only restored while something refers to it. + const replaced = []; + const objects = Array.from({ length: 16 }, () => { + const first = function () {}; + const object = { method: first }; + let isArmed = true; + // spyOn() reads the `length` of the original before it stores the spy. + Object.defineProperty(first, "length", { + get() { + if (isArmed) { + isArmed = false; + object.method = second; + replaced.push(spy(object, "method")); + } + return 0; + }, + }); + return object; + }); + for (const object of objects) spy(object, "method"); + jest.restoreAllMocks(); + expect(objects.map(object => object.method())).toEqual(objects.map(() => "second")); + expect(replaced.length).toBe(16); + }, + ); + + test.each(flavors)("%s: a spy that stays is restored before the spies made since", (_, isVitest, fn, spy) => { + const restored = []; + const proxies = []; + const indices = [0, 1, 2, 3, 4, 5]; + let phase = "install"; + for (const i of indices) { + const later = trapped({ method() {} }, () => phase === "restore" && restored.push(`later ${i}`)); + const stuck = trapped({ method() {} }, () => { + if (phase === "restore") restored.push(`stuck ${i}`); + if (phase !== "refuse") return; + spy(later, "method"); + throw new Error(`refuses ${i}`); + }); + spy(stuck, "method"); + proxies.push(stuck, later); + } + phase = "refuse"; + try { + expect(() => jest.restoreAllMocks()).toThrow(new Error("refuses 0")); + } finally { + phase = "restore"; + jest.restoreAllMocks(); + } + expect(restored).toEqual([...indices.map(i => `stuck ${i}`), ...indices.map(i => `later ${i}`)]); + expect(spied(proxies)).toEqual([]); + }); + + test.each(flavors)("%s: the spies left after a garbage collection are still in order", (_, isVitest, fn, spy) => { + const restored = []; + const proxies = []; + let isRestoring = false; + for (let i = 0; i < 8; i++) { + for (let j = 0; j < 100; j++) spy({ method() {} }, "method"); + proxies.push(trapped({ method() {} }, () => isRestoring && restored.push(i))); + spy(proxies[i], "method"); + } + Bun.gc(true); + isRestoring = true; + jest.restoreAllMocks(); + expect(restored).toEqual([0, 1, 2, 3, 4, 5, 6, 7]); + expect(spied(proxies)).toEqual([]); + }); + + test.each(flavors)( + "%s: a restoreAllMocks() that is terminated leaves the spies it did not reach to the next one", + (_, isVitest, fn, spy) => { + const keys = ["a", "b", "c", "d", "e", "f"]; + const restored = []; + let phase = "install"; + let isSpinning = false; + const proxy = trapped(Object.fromEntries(keys.map(key => [key, () => {}])), key => { + if (phase === "install") return; + if (phase === "spin" && key === "c") { + isSpinning = true; + for (;;); + } + restored.push(key); + }); + for (const key of keys) spy(proxy, key); + globalThis.restoreAllMocksUntilTerminated = () => jest.restoreAllMocks(); + try { + phase = "spin"; + // The timeout of node:vm terminates the call. If it comes before `c` spins, try a longer one. + for (let timeout = 50; !isSpinning; timeout *= 2) { + expect(() => vm.runInThisContext("restoreAllMocksUntilTerminated()", { timeout })).toThrow( + expect.objectContaining({ code: "ERR_SCRIPT_EXECUTION_TIMEOUT" }), + ); + } + expect(keys.filter(key => jest.isMockFunction(proxy[key]))).toEqual(["c", "d", "e", "f"]); + } finally { + phase = "restore"; + restored.length = 0; + jest.restoreAllMocks(); + delete globalThis.restoreAllMocksUntilTerminated; + } + expect(restored).toEqual(["c", "d", "e", "f"]); + expect(keys.filter(key => jest.isMockFunction(proxy[key]))).toEqual([]); + }, + ); + }); + + describe("the end of a test file", () => { + const runTestFiles = async files => { + const { bunEnv, bunExe, tempDir } = require("harness"); + using dir = tempDir("mock-fn-end-of-file", files); + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", ...Object.keys(files).map(name => `./${name}`)], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + }; + const reported = stderr => + stderr + .split(/\r?\n/) + .filter(line => /^(error: |[ab]\.test\.ts:$|\((pass|fail)\) | \d+ errors?$)/.test(line)) + .map(line => line.replace(/ \[[\d.]+ms\]$/, "")); + + test("two spies on one property put back their originals in the order they were made", async () => { + const { stderr, exitCode } = await runTestFiles({ + "a.test.ts": ` + import { spyOn, test } from "bun:test"; + globalThis.objects = Array.from({ length: 16 }, () => ({ method: () => "first" })); + test("a", () => { + for (const object of objects) { + spyOn(object, "method"); + object.method = () => "second"; + spyOn(object, "method"); + } + }); + `, + "b.test.ts": ` + import { expect, test } from "bun:test"; + test("b", () => expect(objects.map(object => object.method())).toEqual(objects.map(() => "second"))); + `, + }); + expect({ reported: reported(stderr), exitCode }).toEqual({ + reported: ["a.test.ts:", "(pass) a", "b.test.ts:", "(pass) b"], + exitCode: 0, + }); + }); + + test("of the spies that can't be restored, the one that was made first is reported", async () => { + // `target` is a Proxy that refuses to have its spies restored once `isOver` is set. + // It is a global, because a spy is only restored while its object is alive. + const refusesRestore = ` + import { jest, spyOn, test, vi } from "bun:test"; + const keys = Array.from({ length: 32 }, (_, i) => "key" + i); + let isOver = false; + const target = globalThis[import.meta.file] = new Proxy(Object.fromEntries(keys.map(key => [key, () => {}])), { + defineProperty(target, key, descriptor) { + if (isOver) throw new Error("the trap refuses " + key); + return Reflect.defineProperty(target, key, descriptor); + }, + }); + function makeSpies() { + const makers = [spyOn, jest.spyOn, vi.spyOn]; + keys.forEach((key, i) => makers[i % 3](target, key)); + isOver = true; + } + `; + const { stderr, exitCode } = await runTestFiles({ + "a.test.ts": ` + ${refusesRestore} + vi.setConfig({ restoreMocks: true }); + test("a", makeSpies); + test("the next test", () => {}); + test("the last test", () => {}); + `, + "b.test.ts": ` + ${refusesRestore} + test("b", makeSpies); + `, + }); + expect({ reported: reported(stderr), exitCode }).toEqual({ + reported: [ + "a.test.ts:", + "(pass) a", + "error: the trap refuses key0", + "(fail) the next test", + "error: the trap refuses key0", + "(fail) the last test", + "error: the trap refuses key0", + "b.test.ts:", + "(pass) b", + "error: the trap refuses key0", + " 2 errors", + ], + exitCode: 1, + }); + }); }); test("restoring a native accessor that was made non-configurable meanwhile", () => { From 75b2f07ad78fc0722a8f597a50995c03fbfff454 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 8 Nov 2026 13:16:12 +0000 Subject: [PATCH 2/6] bun test: number spies where spyOn() makes them, and restore them in that order The order of a restore is now a number on the spy, from a counter of its global, and no longer the place of the spy in the list. A spy keeps its number when it enters the list again. So a spy whose restore failed, a spy that spyOn() installs after script made a newer one, and a spy that a terminated restoreAllMocks() did not reach are all restored in the order they were made. restoreAllMocks() sorts its spies only if one of them entered the list after a newer one. The lists have the membership that the sets had on main. forEachIn() takes every entry out before it visits the mocks, so a mock that enters again while a pass runs gets a new entry, and a pass that script starts from inside the pass finds it. A pass that is terminated puts back the mocks it did not reach, and the spy it was at if that one is still installed. resetAllMocks() and clearAllMocks() visit mocks in the order they entered their list. The per-global number and the out-of-line sort are as on the branch robobun/10053f09/restore-spies-newest-first, with the other direction. Tests: the corners above, a nested resetAllMocks(), collections during and between passes, the spies of a preload, and the review findings on the terminated-reset test and the getter-spy test. --- src/jsc/bindings/JSMockFunction.cpp | 105 +++++++++----- src/jsc/bindings/JSMockFunction.h | 2 + test/js/bun/test/mock-fn.test.js | 209 +++++++++++++++++++++++----- 3 files changed, 248 insertions(+), 68 deletions(-) diff --git a/src/jsc/bindings/JSMockFunction.cpp b/src/jsc/bindings/JSMockFunction.cpp index 5ac90a3bcfbb..744996a17549 100644 --- a/src/jsc/bindings/JSMockFunction.cpp +++ b/src/jsc/bindings/JSMockFunction.cpp @@ -130,8 +130,7 @@ JSC_DECLARE_HOST_FUNCTION(jsMockFunctionThrowThis); uint64_t JSMockModule::s_nextInvocationId = 0; -// The mocks that clearAllMocks(), resetAllMocks() or restoreAllMocks() has to visit, in the order they entered. -// The list doesn't keep them alive: the collector takes out the dead ones at the end of every collection. +// The mocks that a clearAllMocks(), resetAllMocks() or restoreAllMocks() has to visit, in the order they entered. It doesn't keep them alive: the collector takes out the dead ones. class WeakMockList final : public JSC::WeakGCHashTable { WTF_DEPRECATED_MAKE_FAST_ALLOCATED(WeakMockList); WTF_MAKE_NONCOPYABLE(WeakMockList); @@ -156,20 +155,24 @@ class WeakMockList final : public JSC::WeakGCHashTable { markDirty(m_vm); } - // Puts `mocks` in front of the ones in the list. - NEVER_INLINE void prepend(std::span mocks) + // For the list of spies, which also notes whether they entered in the order they were made. + void append(JSMockFunction* spy, uint64_t spyOrder) { - m_mocks.insertSpan(0, mocks); - markDirty(m_vm); + if (spyOrder < m_lastSpyOrder) [[unlikely]] + m_isInSpyOrder = false; + m_lastSpyOrder = spyOrder; + append(spy); } - // Empties the list into `mocks`, which keeps them alive. - NEVER_INLINE void moveTo(MarkedVector& mocks) + // Empties the list into `mocks`, which keeps them alive. Returns whether spies are in the order they were made. + NEVER_INLINE bool moveTo(MarkedVector& mocks) { mocks.ensureCapacity(m_mocks.size()); for (auto* mock : m_mocks) mocks.append(mock); m_mocks.shrink(0); + m_lastSpyOrder = 0; + return std::exchange(m_isInSpyOrder, true); } private: @@ -184,6 +187,8 @@ class WeakMockList final : public JSC::WeakGCHashTable { Vector m_mocks; VM& m_vm; + uint64_t m_lastSpyOrder { 0 }; + bool m_isInSpyOrder { true }; }; class JSMockImplementation final : public JSNonFinalObject { @@ -908,7 +913,7 @@ class JSMockFunction final : public JSC::InternalFunction { } // Spies are restored in the order they were made. Call it before wrapFunction() and the store of the spy, which can both run script that makes another spy. - void willInstall() { takePlaceIn(List::Spies); } + void willInstall() { m_spyOrder = ++mockModule().spiesMade; } static void clearAll(Zig::GlobalObject*); static void resetAll(Zig::GlobalObject*); @@ -948,10 +953,12 @@ class JSMockFunction final : public JSC::InternalFunction { RELEASE_ASSERT_NOT_REACHED(); } - // The bit of `m_lists` that says the list, or the forEachIn() that runs over it, has an entry for this mock. + // The bit of `m_lists` that says the list has an entry for this mock. static constexpr uint8_t hasEntryIn(List list) { return static_cast(list) << 3; } template static void forEachIn(Zig::GlobalObject*, List, const Functor&); + static void sortSpies(std::span); + static void enterAgainAfterTermination(List, std::span); ALWAYS_INLINE void enter(List list) { @@ -961,19 +968,22 @@ class JSMockFunction final : public JSC::InternalFunction { NEVER_INLINE void enterSlow(List list) { - takePlaceIn(list); + takeEntryIn(list); m_lists |= static_cast(list); } - // A mock has one entry, and keeps it when it leaves, so it has the same place if it enters again. - void takePlaceIn(List list) + // The list has at most one entry for a mock. The entry stays when the mock leaves, until a forEachIn() takes it out. + void takeEntryIn(List list) { if (m_lists & hasEntryIn(list)) return; auto& mocks = listOf(mockModule(), list); if (!mocks) mocks = makeUnique(vm()); - mocks->append(this); + if (list == List::Spies) + mocks->append(this, m_spyOrder); + else + mocks->append(this); m_lists |= hasEntryIn(list); } @@ -1065,8 +1075,13 @@ class JSMockFunction final : public JSC::InternalFunction { JSC::Identifier m_spyIdentifier; unsigned m_spyAttributes { 0 }; uint8_t m_lists { 0 }; + // From JSMockModule::spiesMade: a spy that was made later has a higher one. + uint64_t m_spyOrder { 0 }; }; +// The cell of a mock is 208 bytes, because an IsoSubspace rounds the size up to 16. +static_assert(sizeof(JSMockFunction) <= 208); + template void JSMockFunction::visitAdditionalChildrenInGCThread(Visitor& visitor) { @@ -1181,6 +1196,7 @@ static SpyWeakHandleOwner& weakValueHandleOwner() void JSMockFunction::didInstall(JSObject* target, const Identifier& key, JSValue original, unsigned attributes) { + ASSERT(m_spyOrder); enter(List::Spies); this->isSpy = true; m_spyTarget = JSC::Weak(target, &weakValueHandleOwner(), nullptr); @@ -1192,9 +1208,24 @@ void JSMockFunction::didInstall(JSObject* target, const Identifier& key, JSValue const ClassInfo JSMockFunctionPrototype::s_info = { "Mock"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSMockFunctionPrototype) }; -// Visits the mocks of a list in the order they entered it. Keeps going after an exception and rethrows the first one at the end. -// Every mock leaves first, so `apply` must enter() the ones that stay. Those keep their places, ahead of the mocks that script makes enter during the pass. -// A pass that script starts from inside `apply` finds none of the mocks that this pass has yet to visit, nor the one it is visiting. +NEVER_INLINE void JSMockFunction::sortSpies(std::span spies) +{ + std::ranges::sort(spies, {}, &JSMockFunction::m_spyOrder); +} + +// A termination ended a pass at the first of `mocks`. If that one is a spy that is still installed, it enters the list again, and so do the mocks the pass did not get to. +NEVER_INLINE void JSMockFunction::enterAgainAfterTermination(List list, std::span mocks) +{ + if (list == List::Spies && mocks.front()->m_spyTarget) + mocks.front()->enter(list); + for (auto* mock : mocks.subspan(1)) { + if (mock->m_lists & static_cast(list)) + mock->takeEntryIn(list); + } +} + +// Empties the list first, which drops its stale entries and lets script that `apply` runs register mocks in it for a later pass. +// Every mock leaves, so `apply` must enter() the ones that stay. Keeps going after an exception and rethrows the first one at the end. template void JSMockFunction::forEachIn(Zig::GlobalObject* globalObject, List list, const Functor& apply) { @@ -1204,30 +1235,30 @@ void JSMockFunction::forEachIn(Zig::GlobalObject* globalObject, List list, const return; MarkedVector mocks; - entries->moveTo(mocks); - size_t staying = 0; - bool isTerminating = false; + bool isInSpyOrder = entries->moveTo(mocks); + for (auto* mock : mocks) + mock->m_lists &= ~hasEntryIn(list); + // Only restores overwrite each other, so only spies are put in the order they were made. They are out of it if script ran inside spyOn(), or a restore failed. + if (list == List::Spies && !isInSpyOrder) [[unlikely]] + sortSpies(mocks.mutableSpan()); + ASSERT(list != List::Spies || std::ranges::is_sorted(mocks.mutableSpan(), {}, &JSMockFunction::m_spyOrder)); JSC::Exception* firstException = nullptr; - for (JSMockFunction* mock : mocks) { - // A termination ends the pass. The mocks it did not get to stay in the list. - if (!isTerminating && (mock->m_lists & static_cast(list))) { - mock->leave(list); - apply(mock); - if (auto* exception = scope.exception()) [[unlikely]] { - if (!scope.tryClearException()) - isTerminating = true; - else if (!firstException) - firstException = exception; + for (size_t i = 0; i < mocks.size(); ++i) { + auto* mock = mocks.at(i); + if (!(mock->m_lists & static_cast(list))) + continue; + mock->leave(list); + apply(mock); + if (auto* exception = scope.exception()) [[unlikely]] { + if (!scope.tryClearException()) { + enterAgainAfterTermination(list, mocks.mutableSpan().subspan(i)); + return; } + if (!firstException) + firstException = exception; } - if (mock->m_lists & static_cast(list)) - mocks.at(staying++) = mock; - else - mock->m_lists &= ~hasEntryIn(list); } - if (staying) - entries->prepend(mocks.mutableSpan().first(staying)); - if (firstException && !isTerminating) + if (firstException) throwException(globalObject, scope, firstException); } diff --git a/src/jsc/bindings/JSMockFunction.h b/src/jsc/bindings/JSMockFunction.h index 081565f19949..9f51a999835f 100644 --- a/src/jsc/bindings/JSMockFunction.h +++ b/src/jsc/bindings/JSMockFunction.h @@ -75,6 +75,8 @@ class JSMockModule final { // True until the first test starts. A preload, or a beforeAll() of one, can only run before that. bool preloadMayBeRunning { true }; + // How many spies this global has made. A spy's number is its place in restoreAllMocks(). + uint64_t spiesMade { 0 }; // Called by GlobalObject::visitChildren template diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index 2f2310a06741..024d0624dea5 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -5040,12 +5040,14 @@ if (isBun) { test("resetAllMocks() resets the mocks in the order they were configured", () => { const names = ["a", "b", "c", "d", "e", "f", "g", "h"]; + const configured = ["c", "a", "h", "b", "g", "d", "f", "e"]; const reset = []; - const mocks = names.map(name => resettable(() => reset.push(name)).mockReturnValue("configured")); + const mocks = Object.fromEntries(names.map(name => [name, resettable(() => reset.push(name))])); + for (const name of configured) mocks[name].mockReturnValue("configured"); reset.length = 0; vi.resetAllMocks(); - expect(reset).toEqual(names); - expect(mocks.map(mocked => mocked())).toEqual(names.map(() => undefined)); + expect(reset).toEqual(configured); + expect(names.map(name => mocks[name]())).toEqual(names.map(() => undefined)); }); test("resetAllMocks() resets a mock once, also if it was reset and configured again before", () => { @@ -5066,16 +5068,54 @@ if (isBun) { expect([resets, mocked()]).toEqual([2, undefined]); }); + test("a resetAllMocks() that is called while a mock is reset also resets that mock if it was configured again", () => { + let isArmed = false; + const mocked = resettable(() => { + if (!isArmed) return; + isArmed = false; + mocked.mockReturnValue("configured again"); + vi.resetAllMocks(); + }); + mocked.mockReturnValue("configured"); + isArmed = true; + vi.resetAllMocks(); + expect(mocked()).toBeUndefined(); + }); + + test("a resetAllMocks() that is called while a mock is reset also resets a mock that was configured meanwhile", () => { + // With `wasConfigured` the running resetAllMocks() has yet to visit `later`. Without it, `later` was reset before. + for (const wasConfigured of [false, true]) { + let isArmed = false; + let later; + let seen = "the reset step did not run"; + const first = resettable(() => { + if (!isArmed) return; + isArmed = false; + if (wasConfigured) later.mockReset(); + later.mockReturnValue("configured again"); + vi.resetAllMocks(); + seen = later(); + }); + later = resettable(() => {}); + first.mockReturnValue("configured"); + later.mockReturnValue("configured"); + if (!wasConfigured) later.mockReset(); + isArmed = true; + vi.resetAllMocks(); + expect({ wasConfigured, seen, later: later() }).toEqual({ wasConfigured, seen: undefined, later: undefined }); + } + }); + test("a resetAllMocks() that is terminated leaves the mocks it did not reach to the next one", () => { const names = ["a", "b", "c", "d", "e", "f"]; const reset = []; let phase = "configure"; - let isSpinning = false; + let spinning; const mocks = names.map(name => resettable(() => { if (phase === "configure") return; - if (phase === "spin" && name === "c") { - isSpinning = true; + if (phase === "spin") { + spinning = name; for (;;); } reset.push(name); @@ -5084,8 +5124,9 @@ if (isBun) { globalThis.resetAllMocksUntilTerminated = () => vi.resetAllMocks(); try { phase = "spin"; - // The timeout of node:vm terminates the call. If it comes before `c` spins, try a longer one. - for (let timeout = 50; !isSpinning; timeout *= 2) { + // The timeout of node:vm terminates the call. One that comes before a reset spins can end the reset of + // a mock that has left the list already, so the next try, with a longer timeout, spins in the mock after it. + for (let timeout = 50; spinning === undefined; timeout *= 2) { expect(() => vm.runInThisContext("resetAllMocksUntilTerminated()", { timeout })).toThrow( expect.objectContaining({ code: "ERR_SCRIPT_EXECUTION_TIMEOUT" }), ); @@ -5096,7 +5137,7 @@ if (isBun) { vi.resetAllMocks(); delete globalThis.resetAllMocksUntilTerminated; } - expect(reset).toEqual(["d", "e", "f"]); + expect(reset).toEqual(names.slice(names.indexOf(spinning) + 1)); expect(mocks.map(mocked => mocked())).toEqual(names.map(() => undefined)); }); @@ -5250,6 +5291,7 @@ if (isBun) { const restored = []; const expected = []; const proxies = []; + const getters = []; let isRestoring = false; for (let i = 0; i < 6; i++) { const inner = trapped({ method() {} }, () => isRestoring && restored.push(`inner ${i}`)); @@ -5257,6 +5299,7 @@ if (isBun) { if (isRestoring) restored.push(`outer ${i}`); else spy(inner, "method"); }); + getters.push([outer, Object.getOwnPropertyDescriptor(outer, "value").get]); spy(outer, "value", "get"); proxies.push(outer, inner); expected.push(`outer ${i}`, `inner ${i}`); @@ -5265,6 +5308,8 @@ if (isBun) { jest.restoreAllMocks(); expect(restored).toEqual(expected); expect(spied(proxies)).toEqual([]); + for (const [outer, getter] of getters) + expect(Object.getOwnPropertyDescriptor(outer, "value").get).toBe(getter); }, ); @@ -5274,27 +5319,33 @@ if (isBun) { const second = () => "second"; // The spy that is made first replaces the other one, which is then only restored while something refers to it. const replaced = []; - const objects = Array.from({ length: 16 }, () => { - const first = function () {}; - const object = { method: first }; - let isArmed = true; - // spyOn() reads the `length` of the original before it stores the spy. - Object.defineProperty(first, "length", { - get() { - if (isArmed) { - isArmed = false; - object.method = second; - replaced.push(spy(object, "method")); - } - return 0; - }, + for (const restoresAllInBetween of [false, true]) { + const objects = Array.from({ length: 16 }, () => { + const first = function () {}; + const object = { method: first }; + let isArmed = true; + // spyOn() reads the `length` of the original before it stores the spy. + Object.defineProperty(first, "length", { + get() { + if (isArmed) { + isArmed = false; + if (restoresAllInBetween) jest.restoreAllMocks(); + object.method = second; + replaced.push(spy(object, "method")); + } + return 0; + }, + }); + return object; }); - return object; - }); - for (const object of objects) spy(object, "method"); - jest.restoreAllMocks(); - expect(objects.map(object => object.method())).toEqual(objects.map(() => "second")); - expect(replaced.length).toBe(16); + for (const object of objects) spy(object, "method"); + jest.restoreAllMocks(); + expect({ restoresAllInBetween, methods: objects.map(object => object.method()) }).toEqual({ + restoresAllInBetween, + methods: objects.map(() => "second"), + }); + } + expect(replaced.length).toBe(32); }, ); @@ -5341,6 +5392,58 @@ if (isBun) { expect(spied(proxies)).toEqual([]); }); + test.each(flavors)( + "%s: the spies that an eden collection frees are out of the list before the next restoreAllMocks()", + (_, isVitest, fn, spy) => { + const { edenGC } = require("bun:jsc"); + edenGC(); + const restored = []; + const proxies = []; + let isRestoring = false; + const make = i => { + proxies.push(trapped({ method() {} }, () => isRestoring && restored.push(i))); + spy(proxies[i], "method"); + }; + for (let i = 0; i < 4; i++) { + make(i); + (() => { + for (let j = 0; j < 200; j++) spy({ method() {} }, "method").mockRestore(); + })(); + } + edenGC(); + // The spies made from here on take the cells that the collection freed. + for (let i = 4; i < 8; i++) make(i); + isRestoring = true; + jest.restoreAllMocks(); + expect(restored).toEqual([0, 1, 2, 3, 4, 5, 6, 7]); + expect(spied(proxies)).toEqual([]); + }, + ); + + test.each(flavors)( + "%s: a collection during restoreAllMocks() does not free the spies that it has yet to restore", + (_, isVitest, fn, spy) => { + const restored = []; + let phase = "install"; + const proxies = Array.from({ length: 32 }, (_, i) => + trapped({ method() {} }, () => { + if (phase !== "restore") return; + restored.push(i); + if (i !== 0) return; + // From here on only the restoreAllMocks() that runs refers to the other spies. + phase = "replace"; + for (const proxy of proxies.slice(1)) proxy.method = () => {}; + phase = "restore"; + Bun.gc(true); + }), + ); + for (const proxy of proxies) spy(proxy, "method"); + phase = "restore"; + jest.restoreAllMocks(); + expect(restored).toEqual(proxies.map((_, i) => i)); + }, + ); + test.each(flavors)( "%s: a restoreAllMocks() that is terminated leaves the spies it did not reach to the next one", (_, isVitest, fn, spy) => { @@ -5380,11 +5483,12 @@ if (isBun) { }); describe("the end of a test file", () => { - const runTestFiles = async files => { + const runTestFiles = async (files, flags = []) => { const { bunEnv, bunExe, tempDir } = require("harness"); using dir = tempDir("mock-fn-end-of-file", files); + const testFiles = Object.keys(files).filter(name => name.includes(".test.")); await using proc = Bun.spawn({ - cmd: [bunExe(), "test", ...Object.keys(files).map(name => `./${name}`)], + cmd: [bunExe(), "test", ...flags, ...testFiles.map(name => `./${name}`)], env: bunEnv, cwd: String(dir), stdout: "pipe", @@ -5472,6 +5576,49 @@ if (isBun) { exitCode: 1, }); }); + + test("the spies of a preload stay in the order they were made", async () => { + const { stderr, exitCode } = await runTestFiles( + { + "preload.ts": ` + import { spyOn } from "bun:test"; + globalThis.restored = []; + globalThis.isRestoring = false; + globalThis.trapped = name => + new Proxy({ method() {} }, { + defineProperty(target, key, descriptor) { + if (isRestoring) restored.push(name); + return Reflect.defineProperty(target, key, descriptor); + }, + }); + globalThis.ofPreload = ["p0", "p1", "p2", "p3", "p4", "p5", "p6", "p7"].map(trapped); + for (const object of ofPreload) spyOn(object, "method"); + `, + "a.test.ts": ` + import { spyOn, test } from "bun:test"; + test("a", () => { + globalThis.ofA = ["a0", "a1"].map(trapped); + for (const object of ofA) spyOn(object, "method"); + }); + `, + "b.test.ts": ` + import { expect, jest, spyOn, test } from "bun:test"; + test("b", () => { + globalThis.ofB = ["b0", "b1"].map(trapped); + for (const object of ofB) spyOn(object, "method"); + isRestoring = true; + jest.restoreAllMocks(); + expect(restored).toEqual(["p0", "p1", "p2", "p3", "p4", "p5", "p6", "p7", "b0", "b1"]); + }); + `, + }, + ["--preload", "./preload.ts"], + ); + expect({ reported: reported(stderr), exitCode }).toEqual({ + reported: ["a.test.ts:", "(pass) a", "b.test.ts:", "(pass) b"], + exitCode: 0, + }); + }); }); test("restoring a native accessor that was made non-configurable meanwhile", () => { From 41923295daf15bbff280e5c27cd626d0c01a6437 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 8 Nov 2026 18:03:04 +0000 Subject: [PATCH 3/6] bun test: keep the spies in order where they enter the list, and number a spy in didInstall() if its maker did not A spy that enters the list after a newer one is put in front of it, with the walk that the undo of module mocks uses. The list of spies is then always in the order they were made, so restoreAllMocks() does not sort, and the list needs no flag for it. didInstall() gives a number to a spy that has none. A new place that installs a spy then needs no call of its own to keep the order. --- src/jsc/bindings/JSMockFunction.cpp | 41 +++++++++++------------------ test/js/bun/test/mock-fn.test.js | 1 + 2 files changed, 17 insertions(+), 25 deletions(-) diff --git a/src/jsc/bindings/JSMockFunction.cpp b/src/jsc/bindings/JSMockFunction.cpp index d23361b63121..9f89fe12b35f 100644 --- a/src/jsc/bindings/JSMockFunction.cpp +++ b/src/jsc/bindings/JSMockFunction.cpp @@ -131,7 +131,7 @@ JSC_DECLARE_HOST_FUNCTION(jsMockFunctionThrowThis); uint64_t JSMockModule::s_nextInvocationId = 0; -// The mocks that a clearAllMocks(), resetAllMocks() or restoreAllMocks() has to visit, in the order they entered. It doesn't keep them alive: the collector takes out the dead ones. +// The mocks that a clearAllMocks(), resetAllMocks() or restoreAllMocks() has to visit, in the order it visits them. It doesn't keep them alive: the collector takes out the dead ones. class WeakMockList final : public JSC::WeakGCHashTable { WTF_DEPRECATED_MAKE_FAST_ALLOCATED(WeakMockList); WTF_MAKE_NONCOPYABLE(WeakMockList); @@ -156,24 +156,23 @@ class WeakMockList final : public JSC::WeakGCHashTable { markDirty(m_vm); } - // For the list of spies, which also notes whether they entered in the order they were made. - void append(JSMockFunction* spy, uint64_t spyOrder) + // For a list that has an order of its own: `mock` goes in front of the entries at the end that `comesAfter` is true for. + template void insertBefore(JSMockFunction* mock, const Predicate& comesAfter) { - if (spyOrder < m_lastSpyOrder) [[unlikely]] - m_isInSpyOrder = false; - m_lastSpyOrder = spyOrder; - append(spy); + size_t index = m_mocks.size(); + while (index && comesAfter(m_mocks[index - 1])) [[unlikely]] + --index; + m_mocks.insert(index, mock); + markDirty(m_vm); } - // Empties the list into `mocks`, which keeps them alive. Returns whether spies are in the order they were made. - NEVER_INLINE bool moveTo(MarkedVector& mocks) + // Empties the list into `mocks`, which keeps them alive. + NEVER_INLINE void moveTo(MarkedVector& mocks) { mocks.ensureCapacity(m_mocks.size()); for (auto* mock : m_mocks) mocks.append(mock); m_mocks.shrink(0); - m_lastSpyOrder = 0; - return std::exchange(m_isInSpyOrder, true); } private: @@ -188,8 +187,6 @@ class WeakMockList final : public JSC::WeakGCHashTable { Vector m_mocks; VM& m_vm; - uint64_t m_lastSpyOrder { 0 }; - bool m_isInSpyOrder { true }; }; class JSMockImplementation final : public JSNonFinalObject { @@ -958,7 +955,6 @@ class JSMockFunction final : public JSC::InternalFunction { static constexpr uint8_t hasEntryIn(List list) { return static_cast(list) << 3; } template static void forEachIn(Zig::GlobalObject*, List, const Functor&); - static void sortSpies(std::span); static void enterAgainAfterTermination(List, std::span); ALWAYS_INLINE void enter(List list) @@ -981,8 +977,9 @@ class JSMockFunction final : public JSC::InternalFunction { auto& mocks = listOf(mockModule(), list); if (!mocks) mocks = makeUnique(vm()); + // Only restores overwrite each other, so only spies are kept in the order they were made. One enters after a newer spy if script ran inside spyOn(), or if its restore failed. if (list == List::Spies) - mocks->append(this, m_spyOrder); + mocks->insertBefore(this, [this](JSMockFunction* spy) { return spy->m_spyOrder > m_spyOrder; }); else mocks->append(this); m_lists |= hasEntryIn(list); @@ -1203,7 +1200,9 @@ static SpyWeakHandleOwner& weakValueHandleOwner() void JSMockFunction::didInstall(JSObject* target, const Identifier& key, JSValue original, unsigned attributes) { - ASSERT(m_spyOrder); + // A spy whose maker did not call willInstall() takes its number here. + if (!m_spyOrder) + willInstall(); enter(List::Spies); this->isSpy = true; m_spyTarget = JSC::Weak(target, &weakValueHandleOwner(), nullptr); @@ -1215,11 +1214,6 @@ void JSMockFunction::didInstall(JSObject* target, const Identifier& key, JSValue const ClassInfo JSMockFunctionPrototype::s_info = { "Mock"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSMockFunctionPrototype) }; -NEVER_INLINE void JSMockFunction::sortSpies(std::span spies) -{ - std::ranges::sort(spies, {}, &JSMockFunction::m_spyOrder); -} - // A termination ended a pass at the first of `mocks`. If that one is a spy that is still installed, it enters the list again, and so do the mocks the pass did not get to. NEVER_INLINE void JSMockFunction::enterAgainAfterTermination(List list, std::span mocks) { @@ -1242,12 +1236,9 @@ void JSMockFunction::forEachIn(Zig::GlobalObject* globalObject, List list, const return; MarkedVector mocks; - bool isInSpyOrder = entries->moveTo(mocks); + entries->moveTo(mocks); for (auto* mock : mocks) mock->m_lists &= ~hasEntryIn(list); - // Only restores overwrite each other, so only spies are put in the order they were made. They are out of it if script ran inside spyOn(), or a restore failed. - if (list == List::Spies && !isInSpyOrder) [[unlikely]] - sortSpies(mocks.mutableSpan()); ASSERT(list != List::Spies || std::ranges::is_sorted(mocks.mutableSpan(), {}, &JSMockFunction::m_spyOrder)); JSC::Exception* firstException = nullptr; for (size_t i = 0; i < mocks.size(); ++i) { diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index 024d0624dea5..bfdb45b33741 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -5038,6 +5038,7 @@ if (isBun) { return mocked; }; + // This is the order of Bun's list. @vitest/spy 5.1.0 resets a vi.fn() in this order too, and a vi.fn(implementation) in the order they were made. test("resetAllMocks() resets the mocks in the order they were configured", () => { const names = ["a", "b", "c", "d", "e", "f", "g", "h"]; const configured = ["c", "a", "h", "b", "g", "d", "f", "e"]; From 7a5bfdda01cc82d5dbe908d2a04e699839e271f9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 8 Nov 2026 19:34:31 +0000 Subject: [PATCH 4/6] bun test: run the three end-of-file tests of mock-fn.test.js concurrently Each of them spawns its own bun test run in its own directory, and they share nothing. --- test/js/bun/test/mock-fn.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index bfdb45b33741..df27012c8716 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -5483,7 +5483,7 @@ if (isBun) { ); }); - describe("the end of a test file", () => { + describe.concurrent("the end of a test file", () => { const runTestFiles = async (files, flags = []) => { const { bunEnv, bunExe, tempDir } = require("harness"); using dir = tempDir("mock-fn-end-of-file", files); From 2fd34797c2000cc8e96efd021f68184cdcc37766 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 9 Nov 2026 07:18:33 +0000 Subject: [PATCH 5/6] ci: retrigger From 46be5b9c1f5f904d9158c7f0131116292da1579e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 9 Nov 2026 07:47:45 +0000 Subject: [PATCH 6/6] bun test: two more checks in the tests of the mock lists A resetAllMocks() that is called while a mock is reset leaves the mocks that the running one has yet to reset. The running one resets them after the nested call returns. main does the same, so the new test passes there. The preload test now checks that the end of a.test.ts restored the spies of that file and left the spies of the preload installed. --- test/js/bun/test/mock-fn.test.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index df27012c8716..0ebf9c9f3584 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -5107,6 +5107,28 @@ if (isBun) { } }); + test("a resetAllMocks() that is called while a mock is reset leaves the mocks that the running one has yet to reset", () => { + let isArmed = false; + let seen = "the reset step did not run"; + // The mock that is reset first configures the other one again. The running resetAllMocks() resets that one later. + const mocks = [0, 1].map(i => + resettable(() => { + if (!isArmed) return; + isArmed = false; + mocks[1 - i].mockReturnValue("configured again"); + vi.resetAllMocks(); + seen = mocks[1 - i](); + }), + ); + for (const mocked of mocks) mocked.mockReturnValue("configured"); + isArmed = true; + vi.resetAllMocks(); + expect({ seen, after: mocks.map(mocked => mocked()) }).toEqual({ + seen: "configured again", + after: [undefined, undefined], + }); + }); + test("a resetAllMocks() that is terminated leaves the mocks it did not reach to the next one", () => { const names = ["a", "b", "c", "d", "e", "f"]; const reset = []; @@ -5605,6 +5627,9 @@ if (isBun) { "b.test.ts": ` import { expect, jest, spyOn, test } from "bun:test"; test("b", () => { + const spied = objects => objects.filter(object => jest.isMockFunction(object.method)).length; + // The end of a.test.ts restored the spies of that file only. + expect({ ofPreload: spied(ofPreload), ofA: spied(ofA) }).toEqual({ ofPreload: 8, ofA: 0 }); globalThis.ofB = ["b0", "b1"].map(trapped); for (const object of ofB) spyOn(object, "method"); isRestoring = true;