Skip to content

Commit cfbc322

Browse files
committed
Give mock.module its own automatic teardown
Bun runs every test file in one process, so a mock.module call without its own restore stays installed for the rest of the run and silently replaces the real module for other files. Route every call through a withMockedModule/withMockedModuleDuring helper that captures the real module and registers its own restore, and reject bare mock.module in test files with an eslint rule so the mistake cannot recur.
1 parent 3be75cf commit cfbc322

11 files changed

Lines changed: 225 additions & 206 deletions

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ When refactoring replaces an old path, delete the old one. No back-compat shims,
3434
- Bug fixes start with a failing test that reproduces the bug. Do not start by patching.
3535
- `tests/unit/` shared unit tests and helpers · co-located `src/**/*.test.ts` for module logic · `tests/fixtures/` fixture repos · `tests/integration/` reactor/permission harness. Planned: `tests/e2e/` (fixture-repo runs).
3636
- A test must not depend on another file having run, or on the default file order. It must pass under `bun test ./src ./tests ./evals --randomize`. If a test mutates module-level state or calls `mock.module`, it must restore that state itself (`afterEach`/`afterAll`), not rely on the process happening to reset it. When capturing a module's real exports to restore later, shallow-copy them (`{ ...moduleNamespace }`) at capture time, whether the namespace came from `await import(path)` or a static `import * as ns from "path"` — Bun mutates the live namespace object in place when the module is mocked, so holding a bare reference to it (either form) silently turns into the mocked exports.
37+
- Never call `mock.module` directly. Bun runs every test file in one process, so a `mock.module` call without its own teardown stays installed for the rest of the run and silently replaces the real module for other files — producing failures in files the change never touched, with no obvious link to the cause and no signal from `tsc` or a per-file run (CL-6967). Use `withMockedModule`/`withMockedModuleDuring` from `tests/helpers/mock-module.ts`, which capture the real module and register their own restore. An eslint rule (`no-restricted-syntax` in `eslint.config.js`) rejects bare `mock.module` calls in `*.test.ts` files.
3738

3839
## Build & Validation
3940

eslint.config.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,4 +56,22 @@ export default tseslint.config(
5656
"no-control-regex": "off",
5757
},
5858
},
59+
{
60+
// A bare `mock.module` call has no teardown of its own, so a mock left
61+
// installed by one test file silently replaces a real module for every
62+
// other file in the same `bun test` process (see CL-6967). Route through
63+
// withMockedModule/withMockedModuleDuring (tests/helpers/mock-module.ts)
64+
// instead, which register their own restore.
65+
files: ["**/*.test.ts"],
66+
rules: {
67+
"no-restricted-syntax": [
68+
"error",
69+
{
70+
selector: "CallExpression[callee.object.name='mock'][callee.property.name='module']",
71+
message:
72+
"Use withMockedModule/withMockedModuleDuring from tests/helpers/mock-module.ts instead of bare mock.module — an un-restored mock.module leaks into every test file that runs after this one.",
73+
},
74+
],
75+
},
76+
},
5977
);

src/auth/codex/instructions.test.ts

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,15 @@
1-
import { afterAll, afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
1+
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
2+
import { withMockedModule } from "../../../tests/helpers/mock-module.js";
23

34
// Reused by both the TUI and exec boot paths (src/tui/runner.ts,
45
// src/exec/runner.ts) to refresh the pinned Codex instructions before first
56
// Codex inference. This exercises the shared refresh/fallback logic directly,
67
// with disk I/O faked so tests never touch the real ~/.corbits cache.
7-
//
8-
// Shallow-copy + afterAll restore is required: Bun mutates the live module
9-
// namespace when mock.module runs, and CI loads ./src before ./tests — a leaked
10-
// node:fs mock turns later suites into ENOENT / missing-settings failures.
11-
12-
const realFs = { ...(await import("node:fs")) };
138

149
let fakeDisk = new Map<string, string>();
1510

16-
mock.module("node:fs", () => ({
17-
...realFs,
11+
await withMockedModule(import.meta.resolve("node:fs"), (real: typeof import("node:fs")) => ({
12+
...real,
1813
readFileSync: (path: string) => {
1914
const contents = fakeDisk.get(path);
2015
if (contents === undefined) {
@@ -30,10 +25,6 @@ mock.module("node:fs", () => ({
3025
mkdirSync: () => undefined,
3126
}));
3227

33-
afterAll(() => {
34-
mock.module("node:fs", () => realFs);
35-
});
36-
3728
const { refreshCodexInstructions, codexInstructions, codexInstructionsHash } =
3829
await import("./instructions.js");
3930
const { GPT_5_CODEX_PROMPT } = await import("./prompts/gpt-5-codex.js");

src/auth/oauth-scope-check.test.ts

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,26 @@
1-
import { afterAll, afterEach, describe, expect, mock, test } from "bun:test";
1+
import { afterEach, describe, expect, test } from "bun:test";
2+
import { withMockedModule } from "../../tests/helpers/mock-module.js";
23

34
// getValidCodexToken/getValidXaiToken hit the real home-level auth store and
45
// refresh endpoints; stub the session layer so this test only exercises the
56
// scope probe's own HTTP call and status classification. Other suites
67
// (tests/unit/codex-session.test.ts) import the real modules directly, so the
78
// mocks must be torn down after this file's tests run rather than leaking
89
// into the rest of the bun test process.
9-
const realCodexSession = { ...(await import("./codex/session.js")) };
10-
const realXaiSession = { ...(await import("./xai/session.js")) };
11-
12-
mock.module("./codex/session.js", () => ({
13-
...realCodexSession,
14-
getValidCodexToken: async () => ({ access: "codex-token", accountId: "acct-1" }),
15-
}));
16-
mock.module("./xai/session.js", () => ({
17-
...realXaiSession,
18-
getValidXaiToken: async () => ({ access: "xai-token" }),
19-
}));
20-
21-
afterAll(() => {
22-
mock.module("./codex/session.js", () => realCodexSession);
23-
mock.module("./xai/session.js", () => realXaiSession);
24-
});
10+
await withMockedModule(
11+
import.meta.resolve("./codex/session.js"),
12+
(real: typeof import("./codex/session.js")) => ({
13+
...real,
14+
getValidCodexToken: async () => ({ access: "codex-token", accountId: "acct-1" }),
15+
}),
16+
);
17+
await withMockedModule(
18+
import.meta.resolve("./xai/session.js"),
19+
(real: typeof import("./xai/session.js")) => ({
20+
...real,
21+
getValidXaiToken: async () => ({ access: "xai-token" }),
22+
}),
23+
);
2524

2625
const { checkOAuthProviderScope } = await import("./oauth-scope-check.js");
2726

src/session/state.test.ts

Lines changed: 15 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,26 @@
1-
import { afterAll, afterEach, beforeEach, expect, mock, test } from "bun:test";
1+
import { afterEach, beforeEach, expect, test } from "bun:test";
22
import { mkdir, rm } from "node:fs/promises";
33
import { join } from "node:path";
44
import { tmpdir } from "node:os";
5-
6-
// Bun mutates the imported namespace object in place when a module is
7-
// mocked, so the capture is shallow-copied immediately -- holding onto the
8-
// live namespace would turn into the mocked exports as soon as mock.module
9-
// below runs, making a later restore a no-op.
10-
const realFs = { ...(await import("node:fs/promises")) };
5+
import { withMockedModule } from "../../tests/helpers/mock-module.js";
116

127
// Simulates the straggler write's real await point (e.g. cycleRecorder.dispose
138
// during the terminal path) landing its writeFile after a later-issued
149
// terminal write's writeFile, so rename-order alone would let it win.
15-
const realWriteFile = realFs.writeFile;
1610
let delayNextWrite = false;
17-
mock.module("node:fs/promises", () => ({
18-
...realFs,
19-
writeFile: async (path: string, data: string) => {
20-
if (delayNextWrite) {
21-
delayNextWrite = false;
22-
await new Promise((resolve) => setTimeout(resolve, 30));
23-
}
24-
return realWriteFile(path, data);
25-
},
26-
}));
27-
28-
afterAll(() => {
29-
mock.module("node:fs/promises", () => realFs);
30-
});
11+
await withMockedModule(
12+
import.meta.resolve("node:fs/promises"),
13+
(real: typeof import("node:fs/promises")) => ({
14+
...real,
15+
writeFile: async (path: string, data: string) => {
16+
if (delayNextWrite) {
17+
delayNextWrite = false;
18+
await new Promise((resolve) => setTimeout(resolve, 30));
19+
}
20+
return real.writeFile(path, data);
21+
},
22+
}),
23+
);
3124

3225
const { finalizeRunState, loadState, saveState } = await import("./state.js");
3326
const { getActiveRun, setActiveRun } = await import("./active-run.js");

src/tools/web-search.test.ts

Lines changed: 25 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,33 @@
1-
import { afterAll, afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
1+
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
2+
import { withMockedModule } from "../../tests/helpers/mock-module.js";
23

34
const calls: { toolName: string; args: Record<string, unknown> }[] = [];
45
let connectConfigs: { name: string; url?: string }[] = [];
56

6-
// Bun mutates the imported namespace object in place when a module is
7-
// mocked, so the capture is shallow-copied immediately -- holding onto the
8-
// live namespace would turn into the mocked exports as soon as mock.module
9-
// below runs, making the afterAll restore a no-op. The mock also needs to
10-
// spread the real module rather than replace it outright, or any other
11-
// export (unwrapToolContent, connectMCPServers) disappears for the rest of
12-
// the process for every file that runs after this one.
13-
const realClient = { ...(await import("../mcp/client.js")) };
14-
15-
mock.module("../mcp/client.js", () => ({
16-
...realClient,
17-
connectMCPServer: async (config: { name: string; url?: string }) => {
18-
connectConfigs.push(config);
19-
return {
20-
ok: true,
21-
client: {
22-
serverName: config.name,
23-
tools: [],
24-
call: async (toolName: string, args: Record<string, unknown>) => {
25-
calls.push({ toolName, args });
26-
return "mock result";
7+
// The mock needs to spread the real module rather than replace it outright,
8+
// or any other export (unwrapToolContent, connectMCPServers) disappears for
9+
// the rest of the process for every file that runs after this one.
10+
await withMockedModule(
11+
import.meta.resolve("../mcp/client.js"),
12+
(real: typeof import("../mcp/client.js")) => ({
13+
...real,
14+
connectMCPServer: async (config: { name: string; url?: string }) => {
15+
connectConfigs.push(config);
16+
return {
17+
ok: true,
18+
client: {
19+
serverName: config.name,
20+
tools: [],
21+
call: async (toolName: string, args: Record<string, unknown>) => {
22+
calls.push({ toolName, args });
23+
return "mock result";
24+
},
25+
close: async () => undefined,
2726
},
28-
close: async () => undefined,
29-
},
30-
};
31-
},
32-
}));
33-
34-
afterAll(() => {
35-
mock.module("../mcp/client.js", () => realClient);
36-
});
27+
};
28+
},
29+
}),
30+
);
3731

3832
const {
3933
createWebSearchTool,

src/tui/mouse-reporting-disabled.test.ts

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,9 @@
77
* `createCliRenderer` branch runs, and assert on the options it was
88
* actually called with.
99
*/
10-
import { afterAll, afterEach, describe, expect, mock, test } from "bun:test";
10+
import { afterEach, describe, expect, test } from "bun:test";
1111
import type { Harness } from "./harness.js";
12+
import { withMockedModule } from "../../tests/helpers/mock-module.js";
1213

1314
interface CapturedRendererOptions {
1415
readonly useMouse?: boolean;
@@ -21,33 +22,20 @@ const mountedHarnesses: Harness[] = [];
2122
// The mock must be registered before anything (including this file's own
2223
// helpers) does a real `@opentui/core` import, or that import wins the module
2324
// cache and the mock never takes effect. Every dependency below is loaded
24-
// with a dynamic `import()` after `mock.module` for that reason.
25-
//
26-
// Bun mutates the imported namespace object in place when a module is
27-
// mocked, so the capture is shallow-copied immediately -- holding onto the
28-
// live namespace would turn into the mocked exports the moment mock.module
29-
// below runs, making the afterAll restore below a no-op.
30-
const realCore = { ...(await import("@opentui/core")) };
31-
32-
mock.module("@opentui/core", () => ({
33-
...realCore,
34-
createCliRenderer: async (options: CapturedRendererOptions) => {
35-
capturedOptions.push(options);
36-
const { createHarness } = await import("./harness.js");
37-
const harness = await createHarness({ width: 80, height: 24 });
38-
mountedHarnesses.push(harness);
39-
return harness.renderer;
40-
},
41-
}));
42-
43-
// `mock.module` replaces the shared module cache for the whole test process,
44-
// not just this file — every other test that imports `@opentui/core` runs in
45-
// the same process. Put the real module back once this file is done so a
46-
// later un-injected `createCliRenderer` caller does not silently get this
47-
// fake harness renderer instead.
48-
afterAll(() => {
49-
mock.module("@opentui/core", () => realCore);
50-
});
25+
// with a dynamic `import()` after the mock is installed for that reason.
26+
await withMockedModule(
27+
import.meta.resolve("@opentui/core"),
28+
(real: typeof import("@opentui/core")) => ({
29+
...real,
30+
createCliRenderer: async (options: CapturedRendererOptions) => {
31+
capturedOptions.push(options);
32+
const { createHarness } = await import("./harness.js");
33+
const harness = await createHarness({ width: 80, height: 24 });
34+
mountedHarnesses.push(harness);
35+
return harness.renderer;
36+
},
37+
}),
38+
);
5139

5240
afterEach(() => {
5341
let harness: Harness | undefined;

src/tui/provider-setup-submit.test.ts

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,22 @@
1-
import { describe, test, expect, afterEach, afterAll, mock } from "bun:test";
1+
import { describe, test, expect, afterEach } from "bun:test";
22
import { mkdtemp, rm } from "node:fs/promises";
33
import { tmpdir } from "node:os";
44
import { join } from "node:path";
55

66
import type { OAuthScopeCheckResult } from "../auth/oauth-scope-check.js";
7+
import { withMockedModule } from "../../tests/helpers/mock-module.js";
78

89
// The oauth branch probes real provider scope over the network; stub the
910
// check so these tests exercise buildProviderSubmitHandler's own branching
10-
// (ok / insufficient-scope / unavailable) without a live call. Restored after
11-
// this file's tests run so a mock never leaks into another suite that
12-
// imports provider-setup-submit.js and expects the real probe.
13-
const realOAuthScopeCheck = { ...(await import("../auth/oauth-scope-check.js")) };
11+
// (ok / insufficient-scope / unavailable) without a live call.
1412
let scopeCheckResult: OAuthScopeCheckResult = { status: "ok" };
15-
mock.module("../auth/oauth-scope-check.js", () => ({
16-
...realOAuthScopeCheck,
17-
checkOAuthProviderScope: async () => scopeCheckResult,
18-
}));
19-
20-
afterAll(() => {
21-
mock.module("../auth/oauth-scope-check.js", () => realOAuthScopeCheck);
22-
});
13+
await withMockedModule(
14+
import.meta.resolve("../auth/oauth-scope-check.js"),
15+
(real: typeof import("../auth/oauth-scope-check.js")) => ({
16+
...real,
17+
checkOAuthProviderScope: async () => scopeCheckResult,
18+
}),
19+
);
2320

2421
const { buildProviderSubmitHandler } = await import("./provider-setup-submit.js");
2522
const { loadLocalSettings, loadSettings, localSettingsPath } =

tests/helpers/mock-module.ts

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
import { afterAll, mock } from "bun:test";
2+
import { fileURLToPath } from "node:url";
3+
4+
// `mock.module` keys its registry by filesystem path, not by `file://` URL --
5+
// registering under a URL string silently fails to intercept a module
6+
// resolved elsewhere by relative specifier, once anything else in the
7+
// process has already loaded the real module. `import.meta.resolve` returns
8+
// a `file://` URL for relative specifiers, so normalize it back to a path.
9+
function toModulePath(path: string): string {
10+
return path.startsWith("file://") ? fileURLToPath(path) : path;
11+
}
12+
13+
async function captureModule<T extends object>(path: string): Promise<T> {
14+
// Bun mutates the imported namespace object in place when a module is
15+
// mocked, so the capture must be a shallow copy taken before any mock
16+
// installs -- holding onto the live namespace would silently turn into
17+
// the mocked exports as soon as mock.module runs.
18+
return { ...(await import(path)) } as T;
19+
}
20+
21+
/**
22+
* Mocks a module for the rest of this test file and registers its own
23+
* `afterAll` restore, so correctness never depends on remembering to add
24+
* one. Bun runs every test file in a single process, so an un-restored
25+
* `mock.module` silently replaces the real module for every file that runs
26+
* after this one -- this is the only sanctioned way to call `mock.module`
27+
* at file scope.
28+
*
29+
* `impl` receives the captured real module so mocks can spread it
30+
* (`...real`) without a separate capture line.
31+
*
32+
* Pass `path` as `import.meta.resolve("./relative/path.js")` from the
33+
* calling file, not a bare relative specifier -- both `import()` and
34+
* `mock.module` inside this helper resolve relative specifiers against
35+
* this file's own location, not the caller's.
36+
*/
37+
export async function withMockedModule<T extends object>(
38+
path: string,
39+
impl: (real: T) => object,
40+
): Promise<T> {
41+
const modulePath = toModulePath(path);
42+
const real = await captureModule<T>(modulePath);
43+
mock.module(modulePath, () => impl(real));
44+
afterAll(() => {
45+
mock.module(modulePath, () => real);
46+
});
47+
return real;
48+
}
49+
50+
/**
51+
* Mocks a module only for the duration of `run`, restoring it immediately
52+
* afterward -- even if `run` throws -- rather than leaving it mocked for
53+
* the rest of the file. Use this when a mock only needs to apply around a
54+
* single call.
55+
*
56+
* Pass `path` as `import.meta.resolve("./relative/path.js")` -- see
57+
* `withMockedModule` above.
58+
*/
59+
export async function withMockedModuleDuring<T extends object, R>(
60+
path: string,
61+
impl: (real: T) => object,
62+
run: () => Promise<R>,
63+
): Promise<R> {
64+
const modulePath = toModulePath(path);
65+
const real = await captureModule<T>(modulePath);
66+
mock.module(modulePath, () => impl(real));
67+
try {
68+
return await run();
69+
} finally {
70+
mock.module(modulePath, () => real);
71+
}
72+
}

0 commit comments

Comments
 (0)