Skip to content

Commit 04baa3f

Browse files
committed
test(repo): prefer test.each over for/forEach loops in tests
Convert hand-rolled parameterization loops in test bodies (and around test definitions) to test.each so each case becomes its own reported test with pinpointed failure attribution. Document the convention in .claude/rules/testing.md, including Bun's readonly-array spread gotcha and the runtime-data exception.
1 parent da2e3cf commit 04baa3f

8 files changed

Lines changed: 216 additions & 173 deletions

File tree

.claude/rules/testing.md

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,34 @@ This runs each unit and integration test file as a separate `bun test` subproces
2626

2727
Prefer `spyOn()` for mocking, and always restore spies in `afterAll` with `mockRestore()`.
2828

29+
Never use `for` or `forEach` loops inside a single test to verify multiple inputs or cases — use `test.each` (or `it.each` / `describe.each`) so each case is its own reported test case with its own name, setup/teardown, and pinpointed failure output.
30+
31+
```ts
32+
// ❌ Don't
33+
test("normalizes inputs", () => {
34+
for (const [input, expected] of cases) {
35+
expect(normalize(input)).toBe(expected);
36+
}
37+
});
38+
39+
// ✅ Do
40+
test.each(cases)("normalizes %s -> %s", (input, expected) => {
41+
expect(normalize(input)).toBe(expected);
42+
});
43+
```
44+
45+
Bun's `test.each` rejects `readonly`/`as const` arrays via its literal-inferring overload. Spread to a mutable copy so the literal union is preserved in the callback type:
46+
47+
```ts
48+
const MODES = ["human", "agent"] as const;
49+
test.each([...MODES])("mode %s", (mode) => {
50+
/* mode: "human" | "agent" */
51+
});
52+
```
53+
54+
Exceptions where a loop in the test body is fine:
55+
56+
- The iteration itself is the behavior under test (asserting an event fires N times, accumulating state across steps).
57+
- The data being iterated is collected at runtime inside the test and cannot be expressed as a static array at module-load time (e.g. `http.requests` after the action runs, files map captured from a callback).
58+
2959
`mock.module()` is acceptable only when registered at file top, before any consumer of the mocked module is loaded (the integration harness at `packages/cli-core/src/test/integration/lib/harness.ts` and `packages/cli-core/src/lib/credential-store.test.ts` both follow this pattern). In Bun 1.x, `mock.module()` registrations are process-lifetime and will pollute the module registry for any later test file that imports the same module via a non-mocked path, so do not call `mock.module()` from inside `beforeEach`/`describe`/`test`, and do not introduce it in test files that will run alongside files importing the real module.

packages/cli-core/src/cli-program.test.ts

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -378,28 +378,25 @@ describe("help: clerk skill install tip", () => {
378378
expect(help).toContain("clerk skill install");
379379
});
380380

381-
for (const dir of STANDARD_AGENT_DIRS) {
382-
test(`hides the tip when ${dir}/skills/clerk-cli/SKILL.md exists under HOME`, () => {
383-
const target = join(tmpHome, dir, "skills/clerk-cli");
384-
mkdirSync(target, { recursive: true });
385-
writeFileSync(join(target, "SKILL.md"), "ok");
386-
expect(renderHelp()).not.toContain(TIP_SUBSTR);
387-
});
381+
const AGENT_DIR_CASES = STANDARD_AGENT_DIRS.flatMap((dir) =>
382+
(["HOME", "cwd"] as const).map((root) => ({ dir, root })),
383+
);
388384

389-
test(`hides the tip when ${dir}/skills/clerk-cli/SKILL.md exists under cwd`, () => {
390-
const target = join(tmpCwd, dir, "skills/clerk-cli");
385+
test.each(AGENT_DIR_CASES)(
386+
"hides the tip when $dir/skills/clerk-cli/SKILL.md exists under $root",
387+
({ dir, root }) => {
388+
const base = root === "HOME" ? tmpHome : tmpCwd;
389+
const target = join(base, dir, "skills/clerk-cli");
391390
mkdirSync(target, { recursive: true });
392391
writeFileSync(join(target, "SKILL.md"), "ok");
393392
expect(renderHelp()).not.toContain(TIP_SUBSTR);
394-
});
395-
}
393+
},
394+
);
396395

397-
for (const rel of EXTRA_REL_PATHS) {
398-
test(`hides the tip when ${rel} exists under cwd`, () => {
399-
const full = join(tmpCwd, rel);
400-
mkdirSync(dirname(full), { recursive: true });
401-
writeFileSync(full, "ok");
402-
expect(renderHelp()).not.toContain(TIP_SUBSTR);
403-
});
404-
}
396+
test.each([...EXTRA_REL_PATHS])("hides the tip when %s exists under cwd", (rel) => {
397+
const full = join(tmpCwd, rel);
398+
mkdirSync(dirname(full), { recursive: true });
399+
writeFileSync(full, "ok");
400+
expect(renderHelp()).not.toContain(TIP_SUBSTR);
401+
});
405402
});

packages/cli-core/src/commands/init/bootstrap.test.ts

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,18 @@ describe("BOOTSTRAP_REGISTRY", () => {
3131
expect(deps).toContain("vite");
3232
});
3333

34-
test("each entry produces a non-empty string[] for all package managers", () => {
35-
for (const entry of BOOTSTRAP_REGISTRY) {
36-
for (const pm of packageManagers) {
37-
const cmd = entry.buildCommand(pm, "test-app");
38-
expect(cmd.length).toBeGreaterThan(0);
39-
expect(cmd.every((arg) => typeof arg === "string")).toBe(true);
40-
}
41-
}
42-
});
34+
const REGISTRY_PM_PAIRS = BOOTSTRAP_REGISTRY.flatMap((entry) =>
35+
packageManagers.map((pm) => ({ dep: entry.dep, entry, pm })),
36+
);
37+
38+
test.each(REGISTRY_PM_PAIRS)(
39+
"entry $dep produces a non-empty string[] for package manager $pm",
40+
({ entry, pm }) => {
41+
const cmd = entry.buildCommand(pm, "test-app");
42+
expect(cmd.length).toBeGreaterThan(0);
43+
expect(cmd.every((arg) => typeof arg === "string")).toBe(true);
44+
},
45+
);
4346

4447
test("Next.js uses project name as target directory", () => {
4548
const cmd = entryFor("next").buildCommand("bun", "my-cool-app");

packages/cli-core/src/commands/skill/install.test.ts

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -121,23 +121,23 @@ const ALL_BUNDLED_FILES = [
121121
] as const;
122122

123123
describe("withStagedClerkSkill version rendering", () => {
124-
test("substitutes CLI_VERSION in every staged file", async () => {
125-
await withStagedClerkSkill("4.5.6", async (stageDir) => {
126-
for (const rel of ALL_BUNDLED_FILES) {
124+
const VERSION_CASES: { version: string | undefined; label: string }[] = [
125+
{ version: "4.5.6", label: "explicit 4.5.6" },
126+
{ version: undefined, label: "undefined → latest" },
127+
];
128+
const VERSION_FILE_CASES = VERSION_CASES.flatMap(({ version, label }) =>
129+
ALL_BUNDLED_FILES.map((rel) => ({ version, label, rel })),
130+
);
131+
132+
test.each(VERSION_FILE_CASES)(
133+
"substitutes {{CLI_VERSION}} ($label) in $rel",
134+
async ({ version, rel }) => {
135+
await withStagedClerkSkill(version, async (stageDir) => {
127136
const content = await readFile(join(stageDir, rel), "utf8");
128137
expect(content, rel).not.toContain("{{CLI_VERSION}}");
129-
}
130-
});
131-
});
132-
133-
test("resolves undefined version to `latest` in every staged file", async () => {
134-
await withStagedClerkSkill(undefined, async (stageDir) => {
135-
for (const rel of ALL_BUNDLED_FILES) {
136-
const content = await readFile(join(stageDir, rel), "utf8");
137-
expect(content, rel).not.toContain("{{CLI_VERSION}}");
138-
}
139-
});
140-
});
138+
});
139+
},
140+
);
141141
});
142142

143143
describe("bundled SKILL.md frontmatter", () => {

packages/cli-core/src/lib/listage.test.ts

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,20 +34,20 @@ describe("scrollBounds", () => {
3434
expect(scrollBounds(20, 19, 5)).toEqual({ above: 15, below: 0 });
3535
});
3636

37-
test("above + below + pageSize = totalItems (pageSize=5)", () => {
38-
for (let active = 0; active < 20; active++) {
39-
const { above, below } = scrollBounds(20, active, 5);
40-
expect(above + below + 5).toBe(20);
41-
}
42-
});
43-
44-
test("above + below + pageSize = totalItems (pageSize=7, odd)", () => {
45-
// Odd pageSize may drift by ±1 at boundaries but must never be catastrophically wrong
46-
for (let active = 0; active < 20; active++) {
47-
const { above, below } = scrollBounds(20, active, 7);
48-
expect(above + below + 7).toBe(20);
49-
}
50-
});
37+
// Invariant must hold for any active position and any pageSize — including
38+
// odd pageSizes where above/below may drift by ±1 at boundaries.
39+
const PAGE_SIZES = [5, 7];
40+
const SCROLL_CASES = PAGE_SIZES.flatMap((pageSize) =>
41+
Array.from({ length: 20 }, (_, active) => ({ pageSize, active })),
42+
);
43+
44+
test.each(SCROLL_CASES)(
45+
"above + below + pageSize = totalItems (pageSize=$pageSize, active=$active)",
46+
({ pageSize, active }) => {
47+
const { above, below } = scrollBounds(20, active, pageSize);
48+
expect(above + below + pageSize).toBe(20);
49+
},
50+
);
5151
});
5252

5353
describe("withScrollIndicators", () => {

packages/cli-core/src/lib/users.test.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,9 @@ describe("users helpers", () => {
5656
expect((error as CliError).exitCode).toBe(EXIT_CODE.USAGE);
5757
});
5858

59-
test("parseUsersPayload rejects non-object JSON (arrays, primitives, null)", () => {
60-
for (const input of ['["email@example.com"]', '"just a string"', "42", "null"]) {
59+
test.each(['["email@example.com"]', '"just a string"', "42", "null"])(
60+
"parseUsersPayload rejects non-object JSON: %s",
61+
(input) => {
6162
let error: unknown;
6263
try {
6364
parseUsersPayload(input);
@@ -66,8 +67,8 @@ describe("users helpers", () => {
6667
}
6768
expect(error).toBeInstanceOf(CliError);
6869
expect((error as CliError).code).toBe(ERROR_CODE.INVALID_JSON);
69-
}
70-
});
70+
},
71+
);
7172

7273
test("redactUsersDisplayPayload masks passwords, codes, and private/unsafe metadata", () => {
7374
expect(

packages/cli-core/src/test/integration/onboard.test.ts

Lines changed: 87 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -18,77 +18,91 @@ import {
1818
const h = useIntegrationTestHarness();
1919
const MODES = ["human", "agent"] as const;
2020

21-
for (const mode of MODES) {
22-
test.each([
23-
{
24-
framework: "Next.js",
25-
deps: { next: "15.0.0" },
26-
expectedKey: "NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY",
27-
envFile: ".env.local",
28-
},
29-
{
30-
framework: "React/Vite",
31-
deps: { react: "19.0.0" },
32-
devDeps: { vite: "6.0.0" },
33-
expectedKey: "VITE_CLERK_PUBLISHABLE_KEY",
34-
envFile: ".env.local",
35-
},
36-
{
37-
framework: "Express",
38-
deps: { express: "4.21.0" },
39-
expectedKey: "CLERK_PUBLISHABLE_KEY",
40-
envFile: ".env.local",
41-
},
42-
{
43-
framework: "Astro",
44-
deps: { astro: "5.0.0" },
45-
expectedKey: "PUBLIC_CLERK_PUBLISHABLE_KEY",
46-
envFile: ".env",
47-
},
48-
{
49-
framework: "Expo",
50-
deps: { expo: "52.0.0" },
51-
expectedKey: "EXPO_PUBLIC_CLERK_PUBLISHABLE_KEY",
52-
envFile: ".env.local",
53-
},
54-
{
55-
framework: "Nuxt",
56-
deps: { nuxt: "3.0.0" },
57-
expectedKey: "NUXT_PUBLIC_CLERK_PUBLISHABLE_KEY",
58-
expectedSecretKey: "NUXT_CLERK_SECRET_KEY",
59-
envFile: ".env",
60-
},
61-
{
62-
framework: "TanStack Start",
63-
deps: { "@tanstack/react-start": "1.0.0", react: "19.0.0" },
64-
expectedKey: "VITE_CLERK_PUBLISHABLE_KEY",
65-
envFile: ".env.local",
66-
},
67-
{
68-
framework: "React Router",
69-
deps: { "react-router": "7.0.0", react: "19.0.0" },
70-
expectedKey: "VITE_CLERK_PUBLISHABLE_KEY",
71-
envFile: ".env.local",
72-
},
73-
{
74-
framework: "Vue",
75-
deps: { vue: "3.0.0" },
76-
expectedKey: "VITE_CLERK_PUBLISHABLE_KEY",
77-
envFile: ".env.local",
78-
},
79-
{
80-
framework: "Fastify",
81-
deps: { fastify: "5.0.0" },
82-
expectedKey: "CLERK_PUBLISHABLE_KEY",
83-
envFile: ".env.local",
84-
},
85-
{
86-
framework: "No framework (fallback)",
87-
deps: { lodash: "4.0.0" },
88-
expectedKey: "CLERK_PUBLISHABLE_KEY",
89-
envFile: ".env.local",
90-
},
91-
])(`$framework (${mode})`, async ({ deps, devDeps, expectedKey, expectedSecretKey, envFile }) => {
21+
interface FrameworkCase {
22+
framework: string;
23+
deps: Record<string, string>;
24+
devDeps?: Record<string, string>;
25+
expectedKey: string;
26+
expectedSecretKey?: string;
27+
envFile: string;
28+
}
29+
30+
const FRAMEWORK_CASES: FrameworkCase[] = [
31+
{
32+
framework: "Next.js",
33+
deps: { next: "15.0.0" },
34+
expectedKey: "NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY",
35+
envFile: ".env.local",
36+
},
37+
{
38+
framework: "React/Vite",
39+
deps: { react: "19.0.0" },
40+
devDeps: { vite: "6.0.0" },
41+
expectedKey: "VITE_CLERK_PUBLISHABLE_KEY",
42+
envFile: ".env.local",
43+
},
44+
{
45+
framework: "Express",
46+
deps: { express: "4.21.0" },
47+
expectedKey: "CLERK_PUBLISHABLE_KEY",
48+
envFile: ".env.local",
49+
},
50+
{
51+
framework: "Astro",
52+
deps: { astro: "5.0.0" },
53+
expectedKey: "PUBLIC_CLERK_PUBLISHABLE_KEY",
54+
envFile: ".env",
55+
},
56+
{
57+
framework: "Expo",
58+
deps: { expo: "52.0.0" },
59+
expectedKey: "EXPO_PUBLIC_CLERK_PUBLISHABLE_KEY",
60+
envFile: ".env.local",
61+
},
62+
{
63+
framework: "Nuxt",
64+
deps: { nuxt: "3.0.0" },
65+
expectedKey: "NUXT_PUBLIC_CLERK_PUBLISHABLE_KEY",
66+
expectedSecretKey: "NUXT_CLERK_SECRET_KEY",
67+
envFile: ".env",
68+
},
69+
{
70+
framework: "TanStack Start",
71+
deps: { "@tanstack/react-start": "1.0.0", react: "19.0.0" },
72+
expectedKey: "VITE_CLERK_PUBLISHABLE_KEY",
73+
envFile: ".env.local",
74+
},
75+
{
76+
framework: "React Router",
77+
deps: { "react-router": "7.0.0", react: "19.0.0" },
78+
expectedKey: "VITE_CLERK_PUBLISHABLE_KEY",
79+
envFile: ".env.local",
80+
},
81+
{
82+
framework: "Vue",
83+
deps: { vue: "3.0.0" },
84+
expectedKey: "VITE_CLERK_PUBLISHABLE_KEY",
85+
envFile: ".env.local",
86+
},
87+
{
88+
framework: "Fastify",
89+
deps: { fastify: "5.0.0" },
90+
expectedKey: "CLERK_PUBLISHABLE_KEY",
91+
envFile: ".env.local",
92+
},
93+
{
94+
framework: "No framework (fallback)",
95+
deps: { lodash: "4.0.0" },
96+
expectedKey: "CLERK_PUBLISHABLE_KEY",
97+
envFile: ".env.local",
98+
},
99+
];
100+
101+
const ONBOARD_CASES = MODES.flatMap((mode) => FRAMEWORK_CASES.map((c) => ({ ...c, mode })));
102+
103+
test.each(ONBOARD_CASES)(
104+
"$framework ($mode)",
105+
async ({ mode, deps, devDeps, expectedKey, expectedSecretKey, envFile }) => {
92106
const pkg = {
93107
name: "test-project",
94108
dependencies: deps,
@@ -118,5 +132,5 @@ for (const mode of MODES) {
118132

119133
const plapiCalls = http.requests.filter((r) => r.url.includes("/applications/"));
120134
expect(plapiCalls.length).toBeGreaterThan(0);
121-
});
122-
}
135+
},
136+
);

0 commit comments

Comments
 (0)