fix(test): mock.module() replacements no longer leak across test files#27823
Open
ivanfilhoz wants to merge 6 commits intooven-sh:mainfrom
Open
fix(test): mock.module() replacements no longer leak across test files#27823ivanfilhoz wants to merge 6 commits intooven-sh:mainfrom
ivanfilhoz wants to merge 6 commits intooven-sh:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds scoped module mocking: introduces a Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/BunPlugin.cpp`:
- Around line 1085-1089: The call that unconditionally clears
global->onLoadPlugins.moduleMockScopeMarkers during mock.restore() drops the
active file scope so subsequent mock.module() calls in the same file aren't
tracked and cannot be rolled back by endModuleMockingScope(); instead, change
the clear to preserve the current file's active scope marker(s): identify the
current scope marker(s) (the marker created for the running test file) and only
remove/filter out markers that belong to other scopes, leaving the active
marker(s) in global->onLoadPlugins.moduleMockScopeMarkers intact; update related
state (e.g., mustDoExpensiveRelativeLookup and moduleMockChanges) accordingly so
endModuleMockingScope() can still roll back mocks for the current file while
fully clearing unrelated scopes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e893d85b-887e-4073-a356-f6ca5cdff657
📒 Files selected for processing (5)
src/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/BunPlugin.hsrc/bun.js/bindings/JSGlobalObject.zigsrc/cli/test_command.zigtest/js/bun/test/mock/mock-module.test.ts
ivanfilhoz
commented
Mar 5, 2026
ivanfilhoz
force-pushed
the
claude/fix-mock-module-leak-across-test-files
branch
from
2a9aa64 to
565527d
Compare
Introduces a per-file scoped undo log for mock.module() calls so that mocks registered in one test file are automatically cleaned up — including cache eviction from requireMap and esmRegistryMap — before the next file runs, without affecting real onLoad plugin virtual modules. - Split BunPlugin::OnLoad virtual module storage into `virtualModules` (plugin-registered) and `moduleMocks` (mock.module() calls) so each can be managed independently. - Add `beginModuleMockingScope()` / `endModuleMockingScope()` which push and pop scope markers onto a change-log stack, restoring previous state on scope exit and evicting resolved-module cache entries so that subsequent imports in other files see the real implementation. - Call begin/end around each test file in test_command.zig via defer. - Add a cross-file regression test that spawns a subprocess with two test files (one mocking, one asserting real behavior) and asserts both pass with exit code 0. Fixes: mock.module() leak reported in oven-sh/bun where calling mock.module() in file A caused file B (in the same bun test invocation) to receive the mocked module instead of the real one, even after mock.restore() was called in afterAll(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When mock.restore() (jsFunctionBunPluginClear) was called mid-file while a per-file mock scope was active, it unconditionally cleared moduleMockScopeMarkers. Any mock.module() calls made after mock.restore() in the same file were no longer tracked, so endModuleMockingScope() could not roll them back and mocks could leak to the next file. Fix: instead of clearing moduleMockScopeMarkers, reset each marker to 0 (since moduleMockChanges is now empty). This preserves the active scope depth so subsequent mock.module() calls within the same file are still tracked and endModuleMockingScope() can clean them up correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When mock.module() is called and the specifier is already present in esmRegistry, it patches the namespace object in-place via overrideExportValue() rather than evicting the registry entry. The previous endModuleMockingScope() only evicted the specifier — it never restored the namespace values — so other cached modules with live ESM bindings to that namespace continued to see mocked exports in subsequent test files. Fix: before overriding exports, snapshot the original values into a plain object stored in the ModuleMockChange record. At scope end, if a snapshot is present, restore each export via overrideExportValue() instead of evicting. The registry entry is kept alive so cached dependents pick up the restored values without a redundant re-evaluation. Also cap the snapshot object's inlineCapacity at JSFinalObject::maxInlineCapacity to avoid an assertion failure when mocking large modules like node:fs or node:fs/promises whose export count exceeds the JSC inline-property limit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ivanfilhoz
force-pushed
the
claude/fix-mock-module-leak-across-test-files
branch
from
565527d to
5e87117
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/BunPlugin.cpp`:
- Around line 1157-1166: Before deleting module mock bookkeeping, replay and
apply the recorded cleanup actions so cached lazy mocks and in-place namespace
patches are actually reverted: iterate the entries in
global->onLoadPlugins.moduleMockChanges and undo each change against
esmRegistry/requireMap and any patched module namespaces (the same steps
endModuleMockingScope would perform), only after successfully reverting them
clear moduleMockChanges and then delete global->onLoadPlugins.moduleMocks;
ensure endModuleMockingScope still relies on moduleMockScopeMarkers and that
mustDoExpensiveRelativeLookup is left false as before.
- Around line 464-507: The restore branch for change.patchedNamespace fixes the
ESM live bindings but never clears the mutated CommonJS cache, so subsequent
require() calls still see mocked exports; update the patchedNamespace path
(inside the if (ns && snapshot) block) to evict the CJS/esm cached entry the
same way the else branch does: construct the specifier with jsString(vm,
change.path) and call requireMap->remove(globalObject, specifier) (and
esmRegistry->remove(globalObject, specifier) if present), handling
scope.exception() exactly as done in the else branch so the
requireMap/esmRegistry entries are cleared when a module was patched in-place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8668d8df-ab3f-416b-9991-6567132f7ded
📒 Files selected for processing (5)
src/bun.js/bindings/BunPlugin.cppsrc/bun.js/bindings/BunPlugin.hsrc/bun.js/bindings/JSGlobalObject.zigsrc/cli/test_command.zigtest/js/bun/test/mock/mock-module.test.ts
…, and mock.restore() Address three review findings from CodeRabbit: 1. Evict CJS requireMap entry when restoring in-place patched namespaces in endModuleMockingScope(). Previously only the ESM namespace exports were restored, leaving the mutated CJS cache intact for subsequent require(). 2. Preserve the original export snapshot when mock.module() is called multiple times on the same specifier within one scope. Previously the second call would overwrite the snapshot with already-mocked values, losing the real originals. 3. Replay undo actions in mock.restore() (jsFunctionBunPluginClear) before clearing bookkeeping. Previously it deleted moduleMocks and cleared moduleMockChanges without actually reverting cached esmRegistry/requireMap entries or restoring patched namespaces, so endModuleMockingScope() had nothing left to undo. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/BunPlugin.cpp`:
- Around line 1171-1249: The restoration loop that replays module mock undo
actions duplicates the logic in endModuleMockingScope(); extract this into a
shared helper (e.g., replayModuleMockChanges(GlobalObject*, size_t startIndex,
size_t endIndex) or similar) that takes the GlobalObject and the range/indices
to replay, move the loop body that reads
global->onLoadPlugins.moduleMockChanges, restores patchedNamespace snapshots,
and evicts requireMap/esmRegistry entries into that helper, then call this
helper from both the current mock.restore() code and endModuleMockingScope() to
remove duplication while preserving existing exception handling and scope usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 369dc97d-056a-4d4a-8cac-3d503fe35526
📒 Files selected for processing (1)
src/bun.js/bindings/BunPlugin.cpp
…up logic Extract the shared undo loop (restore patched namespaces, evict requireMap/esmRegistry) into BunPlugin::OnLoad::revertMockChanges(), called from both endModuleMockingScope() and jsFunctionBunPluginClear(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
1 participant
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.
Problem
When
mock.module()is called in one test file, the mock state leaks into subsequent test files run in the same process. Two distinct code paths both had this bug:Case 1 — lazy factory path:
mock.module(specifier, factory)is called before the module has been loaded. The factory result ends up inesmRegistry. When the test file's scope ends,endModuleMockingScopemust evict that entry so later files see the real module.Case 2 — in-place patch path:
mock.module(specifier, factory)is called after the module is already inesmRegistry(e.g. a top-levelimportprecedes themock.modulecall). In this case the namespace object is patched in-place viaoverrideExportValue()rather than evicting the registry entry. The oldendModuleMockingScopeevicted the specifier but never restored the namespace values, so any cached module with a live ESM binding to that namespace (e.g. a helper that re-exports from it) continued to see mocked exports in subsequent test files.Additionally, the in-place path introduced an assertion failure (
inlineCapacity <= JSFinalObject::maxInlineCapacity) when mocking large modules likenode:fsornode:fs/promises, whose export count exceeds the JSC inline-property limit.Fix
endModuleMockingScopenow handles both cases explicitly:esmRegistryandrequireMapas before.ModuleMockChangerecord. At scope end, restore each export viaoverrideExportValue()instead of evicting. The registry entry is kept alive so cached dependents pick up the restored values without a redundant re-evaluation.The snapshot object's
inlineCapacityis capped atJSFinalObject::maxInlineCapacity(matching the pattern used elsewhere in the codebase) to avoid the assertion crash on large modules.Also fixes
mock.restore()to preserve scope markers (resetting them to 0) rather than clearing them entirely, so mocks registered after amock.restore()call within the same file scope are still properly tracked and cleaned up.Test
test/js/bun/test/mock/mock-module.test.ts— added"mock.module does not leak across test files": spawns a subprocess where file A mocks a module and callsmock.restore()inafterAll, and file B imports the real module and asserts it sees the original value.Verified: fails with system bun (v1.3.10), passes with this build.