diff --git a/cli/TESTING.md b/cli/TESTING.md index df0311677d..7bf87cb350 100644 --- a/cli/TESTING.md +++ b/cli/TESTING.md @@ -53,9 +53,17 @@ in-process suite imports.** Check with `grep -rl "" test/` before r for one. A suite that drives the CLI through a spawned process is out of reach of a module mock and doesn't count. -`raw_app_push_policy_unit.test.ts` is the worked example: it stubs `gen/services.gen.ts`, -which passes the rule because nothing else in `test/` imports the three API functions it -replaces, and deliberately does not stub `bundle.ts`, which failed it. +The API client `gen/services.gen.ts` is the exception, and is stubbed only through +`mockServices` in `test/mock_services.ts`. Bun fixes a mocked module's export names at +the first `mock.module` call of the run, so a stub that replaced the module with just the +functions its suite needed left every other name undefined for the rest of the run, even +for suites that stubbed that name themselves. `test/mock_services.ts` therefore calls +`mock.module` exactly once, with every real export behind a dispatcher; `mockServices` +only swaps which stubs the dispatcher routes to, so it doesn't rely on the unreliable +`afterAll` re-mock described above. + +`raw_app_push_policy_unit.test.ts` deliberately does not stub `bundle.ts`, which failed +the rule. ## AI Benchmark Caveats diff --git a/cli/test/instance_configs_unit.test.ts b/cli/test/instance_configs_unit.test.ts index c8ba15c5dc..595f31aa6a 100644 --- a/cli/test/instance_configs_unit.test.ts +++ b/cli/test/instance_configs_unit.test.ts @@ -8,7 +8,8 @@ * - pushInstanceConfigs skips unchanged configs */ -import { expect, test, describe, beforeEach, afterEach, mock } from "bun:test"; +import { expect, test, describe, beforeEach, afterEach } from "bun:test"; +import { mockServices } from "./mock_services.ts"; import { writeFile, readFile, mkdir, rm } from "node:fs/promises"; import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -21,7 +22,7 @@ let updateConfigCalls: { name: string; requestBody: any }[] = []; let deleteConfigCalls: { name: string }[] = []; // Mock the wmill module before importing settings.ts -mock.module("../gen/services.gen.ts", () => ({ +mockServices({ listWorkerGroups: async () => listWorkerGroupsResult, updateConfig: async (args: { name: string; requestBody: any }) => { updateConfigCalls.push(args); @@ -32,7 +33,7 @@ mock.module("../gen/services.gen.ts", () => ({ listConfigs: async () => { throw new Error("listConfigs should not be called"); }, -})); +}); import { pullInstanceConfigs, diff --git a/cli/test/mock_services.ts b/cli/test/mock_services.ts new file mode 100644 index 0000000000..3e61af1979 --- /dev/null +++ b/cli/test/mock_services.ts @@ -0,0 +1,35 @@ +import { afterAll, mock } from "bun:test"; + +// Every in-process suite that stubs the API client goes through here. Bun +// fixes a mocked module's export names at the first `mock.module` call of the +// run, so a stub missing a name leaves it undefined for every later suite, even +// ones that stub it themselves. The module is therefore mocked once, with every +// real export behind a dispatcher: a suite swaps its stubs in and out of +// `active`, and never depends on a later `mock.module` call taking effect. +const real = { ...(await import("../gen/services.gen.ts")) } as Record< + string, + unknown +>; +let active: Record = {}; + +const dispatched: Record = {}; +for (const [name, value] of Object.entries(real)) { + dispatched[name] = + typeof value === "function" + ? (...args: unknown[]) => + ((active[name] ?? value) as (...a: unknown[]) => unknown)(...args) + : value; +} +mock.module("../gen/services.gen.ts", () => dispatched); + +export function mockServices(stubs: Record): void { + for (const name of Object.keys(stubs)) { + if (!(name in real)) { + throw new Error(`mockServices: services.gen.ts has no export ${name}`); + } + } + active = stubs; + afterAll(() => { + active = {}; + }); +} diff --git a/cli/test/push_workspace_key_unit.test.ts b/cli/test/push_workspace_key_unit.test.ts index 6d8950cbec..aa5d094fd7 100644 --- a/cli/test/push_workspace_key_unit.test.ts +++ b/cli/test/push_workspace_key_unit.test.ts @@ -13,7 +13,8 @@ * - WMILL_NO_REENCRYPT_ON_KEY_CHANGE=true does the same via env var */ -import { expect, test, describe, beforeEach, afterEach, mock } from "bun:test"; +import { expect, test, describe, beforeEach, afterEach } from "bun:test"; +import { mockServices } from "./mock_services.ts"; // Track calls to mocked wmill functions let remoteKey = ""; @@ -23,7 +24,7 @@ let setEncryptionKeyCalls: { }[] = []; // Mock the wmill module before importing settings.ts -mock.module("../gen/services.gen.ts", () => ({ +mockServices({ getWorkspaceEncryptionKey: async (_args: { workspace: string }) => ({ key: remoteKey, }), @@ -33,7 +34,7 @@ mock.module("../gen/services.gen.ts", () => ({ }) => { setEncryptionKeyCalls.push(args); }, -})); +}); import { pushWorkspaceKey } from "../src/core/settings.ts"; diff --git a/cli/test/push_workspace_settings_auto_invite_unit.test.ts b/cli/test/push_workspace_settings_auto_invite_unit.test.ts index b04d895056..f4564c25b2 100644 --- a/cli/test/push_workspace_settings_auto_invite_unit.test.ts +++ b/cli/test/push_workspace_settings_auto_invite_unit.test.ts @@ -4,7 +4,8 @@ * instance groups that settings.yaml does not declare. */ -import { expect, test, describe, beforeEach, mock } from "bun:test"; +import { expect, test, describe, beforeEach } from "bun:test"; +import { mockServices } from "./mock_services.ts"; let editAutoInviteCalls: unknown[] = []; let editInstanceGroupsCalls: unknown[] = []; @@ -17,10 +18,9 @@ const remoteAutoInvite = { instance_groups_roles: { eng: "developer" }, }; -// Every wmill.* call reachable from pushWorkspaceSettings is stubbed: bun shares one -// mocked module across test files, and names missing from whichever mock loads first -// stay missing for the others. -mock.module("../gen/services.gen.ts", () => ({ +// Every wmill.* call reachable from pushWorkspaceSettings is stubbed so the +// function runs without a backend. +mockServices({ getSettings: async () => ({ auto_invite: remoteAutoInvite }), getWorkspaceName: async () => "phoenix", changeWorkspaceName: async () => {}, @@ -45,7 +45,7 @@ mock.module("../gen/services.gen.ts", () => ({ editSlackCommand: async () => {}, setWorkspaceSlackOauthConfig: async () => {}, deleteWorkspaceSlackOauthConfig: async () => {}, -})); +}); const { pushWorkspaceSettings } = await import("../src/core/settings.ts"); diff --git a/cli/test/push_workspace_settings_identity_unit.test.ts b/cli/test/push_workspace_settings_identity_unit.test.ts index 5dcf9815ef..d11b5efac4 100644 --- a/cli/test/push_workspace_settings_identity_unit.test.ts +++ b/cli/test/push_workspace_settings_identity_unit.test.ts @@ -4,7 +4,8 @@ * the file carries one. Rationale lives at the apply sites in settings.ts. */ -import { expect, test, describe, beforeEach, mock } from "bun:test"; +import { expect, test, describe, beforeEach } from "bun:test"; +import { mockServices } from "./mock_services.ts"; let changeWorkspaceNameCalls: unknown[] = []; let changeWorkspaceColorCalls: unknown[] = []; @@ -15,7 +16,7 @@ let remoteWebhook: string | undefined = undefined; // Every wmill.* call reachable from pushWorkspaceSettings is stubbed so the // function runs without a backend; only the three we assert on record calls. -mock.module("../gen/services.gen.ts", () => ({ +mockServices({ getSettings: async (_a: { workspace: string }) => ({ webhook: remoteWebhook, color: remoteColor, @@ -45,7 +46,7 @@ mock.module("../gen/services.gen.ts", () => ({ editSlackCommand: async () => {}, setWorkspaceSlackOauthConfig: async () => {}, deleteWorkspaceSlackOauthConfig: async () => {}, -})); +}); const { pushWorkspaceSettings } = await import("../src/core/settings.ts"); diff --git a/cli/test/raw_app_push_policy_unit.test.ts b/cli/test/raw_app_push_policy_unit.test.ts index fbbc7b55c0..5a4eab87d5 100644 --- a/cli/test/raw_app_push_policy_unit.test.ts +++ b/cli/test/raw_app_push_policy_unit.test.ts @@ -6,26 +6,20 @@ * file states, and that the markers still close a deployed open app back down. */ -import { afterAll, beforeEach, expect, mock, test } from "bun:test"; +import { beforeEach, expect, test } from "bun:test"; import { mkdtemp, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { mockServices } from "./mock_services.ts"; let calls: any[] = []; let deployedPolicy: any; /** No app deployed at the path: `getAppByPath` 404s and the push creates one. */ let deployed = true; -// Stub only what no other in-process suite imports, and treat a stub as -// permanent for the run (see "Module mocks" in cli/TESTING.md). These three API -// functions qualify — nothing else in `test/` imports them. `bundle.ts` did not: -// stubbing it left `raw_app_svelte_plugin_unit.test.ts` asserting against an -// empty bundle, which an `afterAll` hand-back did not prevent. So the real -// bundler runs instead, on the app each push writes below. -const realServices = await import("../gen/services.gen.ts"); - -mock.module("../gen/services.gen.ts", () => ({ - ...realServices, +// `bundle.ts` is deliberately not stubbed (see "Module mocks" in +// cli/TESTING.md): the real bundler runs on the app each push writes below. +mockServices({ getAppByPath: async () => { if (!deployed) throw new Error("not found"); return { @@ -41,12 +35,6 @@ mock.module("../gen/services.gen.ts", () => ({ createAppRaw: async (a: unknown) => { calls.push(a); }, -})); - -// Belt and braces: nothing else in-process calls these, and a hand-back is not -// what makes that safe. -afterAll(() => { - mock.module("../gen/services.gen.ts", () => realServices); }); const { pushRawApp } = await import("../src/commands/app/raw_apps.ts"); diff --git a/cli/test/schedule_push_permissioned_as_unit.test.ts b/cli/test/schedule_push_permissioned_as_unit.test.ts index 17a1731494..953f8a415e 100644 --- a/cli/test/schedule_push_permissioned_as_unit.test.ts +++ b/cli/test/schedule_push_permissioned_as_unit.test.ts @@ -9,6 +9,7 @@ import { expect, test, describe, afterAll, beforeEach, mock } from "bun:test"; import { mkdtemp, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { mockServices } from "./mock_services.ts"; let updateScheduleCalls: any[] = []; let remotePermissionedAs: string | undefined = "u/svc"; @@ -45,7 +46,7 @@ afterAll(() => { } }); -await mockModule("../gen/services.gen.ts", () => ({ +mockServices({ getSchedule: async () => REMOTE_SCHEDULE(), updateSchedule: async (a: unknown) => { updateScheduleCalls.push(a); @@ -56,7 +57,7 @@ await mockModule("../gen/services.gen.ts", () => ({ is_admin: true, groups: [], }), -})); +}); await mockModule("../src/core/context.ts", (real) => ({ ...real, diff --git a/cli/test/shared_ui_diff_unit.test.ts b/cli/test/shared_ui_diff_unit.test.ts index 052fa58a8e..77328457e4 100644 --- a/cli/test/shared_ui_diff_unit.test.ts +++ b/cli/test/shared_ui_diff_unit.test.ts @@ -5,7 +5,8 @@ * (including no local folder) when the apply would be a no-op. */ -import { expect, test, describe, beforeEach, afterEach, mock } from "bun:test"; +import { expect, test, describe, beforeEach, afterEach } from "bun:test"; +import { mockServices } from "./mock_services.ts"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; @@ -14,7 +15,7 @@ let remoteFiles: Record = {}; let remoteUnreadable = false; let pushedFiles: Record | undefined; -mock.module("../gen/services.gen.ts", () => ({ +mockServices({ getSharedUi: async (_args: { workspace: string }) => { if (remoteUnreadable) throw new Error("shared UI store unreadable"); return { files: remoteFiles }; @@ -25,7 +26,7 @@ mock.module("../gen/services.gen.ts", () => ({ }) => { pushedFiles = args.requestBody.files; }, -})); +}); const { diffSharedUi, pushSharedUi } = await import( "../src/commands/shared_ui.ts"