fix(test): make mock.restore() restore modules mocked with mock.module()#26546
Open
dylan-conway wants to merge 1 commit into
Open
fix(test): make mock.restore() restore modules mocked with mock.module()#26546dylan-conway wants to merge 1 commit into
dylan-conway wants to merge 1 commit into
Conversation
Collaborator
|
Updated 12:50 AM PT - Jan 29th, 2026
❌ @dylan-conway, your commit a33f588 has 6 failures in
🧪 To try this PR locally: bunx bun-pr 26546That installs a local version of the PR into your bun-26546 --bun |
Contributor
WalkthroughCaptures original module export values before mocking and restores them on mock.restore(). Adds snapshot and restore target fields to JSModuleMock, restores ESM and CJS exports via a new OnLoad::restoreModuleMocks, and invokes restoration during the global mock.restore flow. New tests validate restoration behavior. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
ed0a065 to
350bd16
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/BunPlugin.cpp (1)
662-742: Restore misses modules cached in both ESM and CJS paths.If the same specifier exists in both
esmRegistryMapandrequireMap, the first branch setsoriginalExportsSnapshot/restoreTarget, but the second branch still patches the other cache without snapshotting.restoreModuleMocks()then restores only one cache, leaving the other mocked aftermock.restore().Consider tracking snapshots per target (ESM vs CJS) and restoring both. Example direction:
🛠️ Suggested direction (split snapshots/targets)
- WriteBarrier<JSObject> originalExportsSnapshot; - WriteBarrier<JSObject> restoreTarget; + WriteBarrier<JSObject> originalExportsSnapshotESM; + WriteBarrier<JSObject> restoreTargetESM; + WriteBarrier<JSObject> originalExportsSnapshotCJS; + WriteBarrier<JSObject> restoreTargetCJS;- if (!mock->originalExportsSnapshot) { - mock->restoreTarget.set(vm, mock, moduleNamespaceObject); + if (!mock->originalExportsSnapshotESM) { + mock->restoreTargetESM.set(vm, mock, moduleNamespaceObject); ... - mock->originalExportsSnapshot.set(vm, mock, snapshot); + mock->originalExportsSnapshotESM.set(vm, mock, snapshot); }- if (!mock->originalExportsSnapshot) { + if (!mock->originalExportsSnapshotCJS) { ... - mock->originalExportsSnapshot.set(vm, mock, snapshot); - mock->restoreTarget.set(vm, mock, moduleObject); + mock->originalExportsSnapshotCJS.set(vm, mock, snapshot); + mock->restoreTargetCJS.set(vm, mock, moduleObject); }Update the re-mock carry-over, visitChildren, and restoreModuleMocks to use both snapshots.
🤖 Fix all issues with AI agents
In `@test/js/bun/test/mock/mock-module-restore.test.ts`:
- Around line 87-100: The test uses dynamic import for "node:fs/promises" (lines
using await import) even though we're only verifying mock.module and
mock.restore behavior; change to a module-scope import to capture the original
export before mocking (e.g., import { readFile as origReadFile } from
"node:fs/promises" at top), then call mock.module("fs/promises", ...) and import
the mocked export using a normal module import or by referencing the mocked
function, and finally call mock.restore() and assert that the restored export
equals origReadFile; update references to origReadFile, mock.module, and
mock.restore accordingly so no dynamic import is used in the test.
Comment on lines
+87
to
+100
| test("mock.restore() restores builtin modules", async () => { | ||
| const origReadFile = (await import("node:fs/promises")).readFile; | ||
|
|
||
| mock.module("fs/promises", () => ({ | ||
| readFile: () => Promise.resolve("mocked-content"), | ||
| })); | ||
|
|
||
| const { readFile } = await import("node:fs/promises"); | ||
| expect(await readFile("anything")).toBe("mocked-content"); | ||
|
|
||
| mock.restore(); | ||
|
|
||
| const { readFile: restored } = await import("node:fs/promises"); | ||
| expect(restored).toBe(origReadFile); |
Contributor
There was a problem hiding this comment.
Avoid dynamic import in this test (not testing import behavior).
Line 88+ uses dynamic import for node:fs/promises, but the test is validating mock.restore() behavior rather than dynamic import semantics. Prefer a module-scope import and capture the original export before mocking.
💡 Suggested change
-import * as spyFixture from "./spymodule-fixture";
+import * as spyFixture from "./spymodule-fixture";
+import * as fsPromises from "node:fs/promises";
...
test("mock.restore() restores builtin modules", async () => {
- const origReadFile = (await import("node:fs/promises")).readFile;
+ const origReadFile = fsPromises.readFile;
...
- const { readFile } = await import("node:fs/promises");
- expect(await readFile("anything")).toBe("mocked-content");
+ expect(await fsPromises.readFile("anything")).toBe("mocked-content");
...
- const { readFile: restored } = await import("node:fs/promises");
- expect(restored).toBe(origReadFile);
+ expect(fsPromises.readFile).toBe(origReadFile);
});As per coding guidelines: Only use dynamic import or require when the test is specifically testing dynamic import or require behavior. Otherwise, always use module-scope import statements.
🤖 Prompt for AI Agents
In `@test/js/bun/test/mock/mock-module-restore.test.ts` around lines 87 - 100, The
test uses dynamic import for "node:fs/promises" (lines using await import) even
though we're only verifying mock.module and mock.restore behavior; change to a
module-scope import to capture the original export before mocking (e.g., import
{ readFile as origReadFile } from "node:fs/promises" at top), then call
mock.module("fs/promises", ...) and import the mocked export using a normal
module import or by referencing the mocked function, and finally call
mock.restore() and assert that the restored export equals origReadFile; update
references to origReadFile, mock.module, and mock.restore accordingly so no
dynamic import is used in the test.
Previously, mock.restore() only reset function spies (spyOn) but had no effect on modules mocked with mock.module(). This was because mock.restore() only called JSMock__resetSpies(), which never touched the virtualModules map or reversed overrideExportValue patches on ESM namespace objects. The fix adds snapshot/restore logic to JSModuleMock: - Before patching ESM exports, snapshot original values into a plain JSObject using WriteBarrier on the existing GC-managed JSModuleMock - On mock.restore(), iterate virtualModules, restore original export values via overrideExportValue, and clear the mock entries - Preserve true originals across re-mocks of the same specifier Key discovery: JSModuleNamespaceObject::getOwnPropertyNames() returns 0 exports for Bun's synthetic namespace objects (empty m_names). The fix uses the mock result object's property names to determine which exports to snapshot from the namespace. Closes #7823
350bd16 to
a33f588
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/bun.js/bindings/BunPlugin.cpp`:
- Around line 786-790: Remove the stray blank line inside the null-check if
block for moduleMock: in the code that casts with jsDynamicCast<JSModuleMock*>
and checks if (!moduleMock) { ... }, delete the empty line so the continue
statement immediately follows the opening brace (i.e., change the if
(!moduleMock) block to contain only the continue statement), keeping the
existing check and behavior intact.
Comment on lines
+786
to
+790
| auto* moduleMock = jsDynamicCast<JSModuleMock*>(entry.value.get()); | ||
| if (!moduleMock) { | ||
|
|
||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Remove empty line inside if block.
Line 788 appears to have an empty line inside the if block before continue. This looks like a debug artifact.
🧹 Cleanup suggestion
auto* moduleMock = jsDynamicCast<JSModuleMock*>(entry.value.get());
if (!moduleMock) {
-
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto* moduleMock = jsDynamicCast<JSModuleMock*>(entry.value.get()); | |
| if (!moduleMock) { | |
| continue; | |
| } | |
| auto* moduleMock = jsDynamicCast<JSModuleMock*>(entry.value.get()); | |
| if (!moduleMock) { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@src/bun.js/bindings/BunPlugin.cpp` around lines 786 - 790, Remove the stray
blank line inside the null-check if block for moduleMock: in the code that casts
with jsDynamicCast<JSModuleMock*> and checks if (!moduleMock) { ... }, delete
the empty line so the continue statement immediately follows the opening brace
(i.e., change the if (!moduleMock) block to contain only the continue
statement), keeping the existing check and behavior intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mock.restore()now correctly restores modules mocked withmock.module(), fixing a long-standing issue wheremock.restore()only reset function spies (spyOn) but had no effect on module mocks.Problem
mock.restore()calledJSMock__resetSpies()which only handledspyOnfunction spies. It never touched thevirtualModulesmap or reversedoverrideExportValuepatches on ESM namespace objects. This meant module mocks persisted across tests even after callingmock.restore().Solution
Added snapshot/restore logic to
JSModuleMock:overrideExportValue, save original values into a plain JSObject stored as aWriteBarrieron the existing GC-managedJSModuleMock(same pattern asspyOn'sspyOriginal)mock.restore(), iteratevirtualModules, restore original export values viaoverrideExportValue, and clear mock entriesKey discovery
JSModuleNamespaceObject::getOwnPropertyNames()returns 0 exports for Bun's synthetic namespace objects becausem_namesis empty. The fix uses the mock result object's property names to determine which exports to snapshot from the namespace.Changes
src/bun.js/bindings/BunPlugin.cpp— AddedWriteBarrierfields toJSModuleMock, snapshot logic before ESM/CJS patching,restoreModuleMocks()methodsrc/bun.js/bindings/BunPlugin.h— AddedrestoreModuleMocksdeclarationsrc/bun.js/bindings/JSMockFunction.cpp— WiredrestoreModuleMocksintoJSMock__jsRestoreAllMockstest/js/bun/test/mock/mock-module-restore.test.ts— New testsTests
5 new tests covering:
spyOn+mock.modulerestorefs/promises)All existing mock-module tests continue to pass (9 pass, 1 todo, 0 fail).
Closes #7823