diff --git a/.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md b/.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md index 2d001e9f..dd2abffd 100644 --- a/.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md +++ b/.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md @@ -19,7 +19,7 @@ Both patterns compile, pass `oxlint`, and pass `bun test` — they are not caugh **Alternatives considered:** - **Add these as more Do's/Don'ts to ARCH-005** — Rejected: ARCH-005's own Compliance section already documents that its Do's and Don'ts section exceeds the `archgate review-context` briefing budget; appending more would push more of it out of every future briefing, including the parts already there. -- **Enforce via a new oxlint plugin immediately** — Deferred, not rejected: this repo already has a precedent (`lint/expect-expect.ts`, `lint/no-bare-env-restore.ts`) for exactly this kind of AST-detectable test-shape rule, and it is the right enforcement layer for both patterns (syntax-detectable, not behavioral). Building it is out of scope for the session that produced this ADR, which only fixed existing instances; the rule is recorded as future work in Compliance and Enforcement below. +- **Enforce via an oxlint plugin** — Adopted for the loop pattern: this repo's precedent (`lint/expect-expect.ts`, `lint/no-bare-env-restore.ts`) is exactly this kind of AST-detectable test-shape rule, and `lint/no-loop-test-cases.ts` follows it. The boolean-collapse pattern is syntax-detectable too and is a candidate for the same layer (Compliance and Enforcement below). - **Leave undocumented, rely on code review alone** — Rejected: the instances this ADR responds to were already written by reviewed, merged PRs; undocumented convention did not prevent the drift. ## Decision @@ -41,20 +41,20 @@ A loop that only builds shared setup data or fixtures for a single overall asser ### Do -- **DO** use `test.each(cases)("", (...args) => {...})` for the same check against independent inputs — each case becomes its own named, independently-reportable test. -- **DO** use `describe.each(cases)(...)` when a case needs its own group of related tests, not just one assertion. +- **DO** use `test.each(cases)("<title with %s/%p/%d>", (...args) => {...})` for the same check against independent inputs — each case becomes its own named test. +- **DO** use `describe.each(cases)(...)` when a case needs a group of related tests, not one assertion. - **DO** pass array rows (`[a, b, expected]`) for positional destructuring, object rows (`{a, b, expected}`) for named fields (title with `$field`). - **DO** assert derived comparisons directly on the values: `expect(actual).toBe(expected)` / `.toEqual(expected)`. -- **DO** match the matcher to the check: `.toContain()`/`.toMatch()` for substrings, `.toBeInstanceOf()` for type checks (including `Array.isArray` replacements), `.toHaveLength()` for counts, `.find(...)` + `.toBeDefined()`/`.toBeUndefined()` for "does any item satisfy X." -- **DO** keep loop→`test.each()` conversions 1:1 — every assertion that ran per-iteration in the original loop must still run per-case; never drop or merge assertions. +- **DO** match the matcher to the check: `.toContain()`/`.toMatch()` for substrings, `.toBeInstanceOf()` for type checks, `.toHaveLength()` for counts, `.find(...)` + `.toBeDefined()`/`.toBeUndefined()` for "does any item satisfy X." +- **DO** keep loop→`test.each()` conversions 1:1 — every per-iteration assertion still runs per case; never drop or merge assertions. ### Don't -- **DON'T** register a `test()`/`it()` or call `expect()` once per iteration inside a `for`/`.forEach` loop — use `test.each()`/`describe.each()` instead. +- **DON'T** register `test()`/`it()`/`describe()` inside a loop, loop `expect()` over rows that exist at registration time (an array literal or `const` array), or let a loop carry a test's only `expect()` — use `test.each()`, or assert the collection outside the loop. - **DON'T** write `expect(a === b).toBe(true)` or `.toBe(false)` — assert `expect(a).toBe(b)` / `.not.toBe(b)` directly. -- **DON'T** collapse `.some(predicate)`/`.every(predicate)` into `.toBe(true)` — it reports `false !== true` with no indication of which element (or none) satisfied the predicate. Use `.find(predicate)` + `.toBeDefined()`, a per-item `expect()` loop, or a matcher over the mapped values (e.g. `expect(arr.map(x => x.message)).toContain(...)`). +- **DON'T** collapse `.some(predicate)`/`.every(predicate)` into `.toBe(true)` — it reports `false !== true` without saying which element satisfied the predicate. Use `.find(predicate)` + `.toBeDefined()`, a per-item `expect()` loop guarded by an assertion on the whole collection, or a matcher over the mapped values (e.g. `expect(arr.map(x => x.message)).toContain(...)`). - **DON'T** write `expect(Array.isArray(x)).toBe(true)` — use `expect(x).toBeInstanceOf(Array)`. -- **DON'T** precompute a boolean just to assert it (`const equal = JSON.stringify(a) === JSON.stringify(b); expect(equal).toBe(true);`) — assert on the values directly with `.toEqual()`/`.toBe()` so a failure shows the actual diff. +- **DON'T** precompute a boolean just to assert it (`const equal = a === b; expect(equal).toBe(true);`) — assert on the values directly with `.toEqual()`/`.toBe()` so a failure shows the actual diff. ## Implementation Pattern @@ -111,8 +111,8 @@ expect(violations.some((v) => v.message.includes('"fetch" global'))).toBe(true); ### Risks -- **No automated enforcement yet**: neither pattern is currently caught by `oxlint` or `archgate check`, so new instances can still be introduced and merged undetected, the same way the instances motivating this ADR were. - - **Mitigation:** tracked as future work in Compliance and Enforcement below (a candidate `oxlint` plugin following the existing `bun-test/expect-expect` precedent); until it exists, code review is the only enforcement layer, so reviewers MUST check new/changed test files against the Do's and Don'ts above. +- **Boolean collapse has no automated enforcement**: `expect(<comparison>).toBe(true)` is caught by neither `oxlint` nor `archgate check`, so new instances can be introduced and merged undetected. + - **Mitigation:** a `test-cases/no-boolean-collapse` rule beside `no-loop-test-cases` is the intended layer (Compliance and Enforcement below); until it exists, reviewers MUST check new/changed test files against Manual Enforcement item 2. - **Existing violations elsewhere in the codebase may resurface**: this ADR's Context reflects a repository-wide audit at a point in time; files not covered by that audit, or added afterward, may still contain either anti-pattern. - **Mitigation:** treat any test file touched for an unrelated change as an opportunity to fix nearby instances of either pattern, consistent with ARCH-005's existing test-hygiene expectations. @@ -120,13 +120,14 @@ expect(violations.some((v) => v.message.includes('"fetch" global'))).toBe(true); ### Automated Enforcement -- **Not yet implemented.** No `oxlint` rule or `archgate` rule currently detects either anti-pattern. A future `oxlint` plugin (e.g. `bun-test/no-loop-test-cases` and `bun-test/no-boolean-collapse`, registered via `jsPlugins` in `.oxlintrc.json` alongside the existing `bun-test/expect-expect` and `test-isolation/no-bare-env-restore` plugins) is the intended enforcement layer once written, matching this repo's existing pattern of using oxlint for syntax-detectable test-shape rules and reserving `archgate` rules for structural/governance checks. This ADR's `rules: false` reflects that no companion `.rules.ts` exists. +- **`test-cases/no-loop-test-cases`** (`lint/no-loop-test-cases.ts`, registered in `.oxlintrc.json` `jsPlugins` and enabled as an error for `tests/**/*.test.ts`) reports three shapes: a `test()`/`it()`/`describe()` registered inside a `for`/`for...in`/`for...of`/`while`/`do...while` body or an iteration callback (`.forEach`, `.map`, `.flatMap`, `.filter`, `.some`, `.every`); an `expect()` inside a `for...of` or iteration callback over rows that exist at registration time (a non-empty array literal, or a `const NAME = [...]` binding in module, `describe`, or test scope, including `as const`/`satisfies` forms); and a loop that carries a test's only `expect()` calls. A runtime-derived loop of per-item `expect()`s with an assertion on the collection outside the loop is not reported. Plugin names are unique within `jsPlugins`, so this plugin is `test-cases`. +- **Boolean collapse is unenforced.** No rule detects `expect(<comparison>).toBe(true)`; a `test-cases/no-boolean-collapse` rule in the same plugin is the intended layer. `rules: false` reflects that no companion `.rules.ts` exists: both patterns are syntax-detectable, which this repo enforces with oxlint plugins, reserving `archgate` rules for structural checks. ### Manual Enforcement Code reviewers MUST verify, for any new or changed file under `tests/**/*.test.ts`: -1. No `for`/`.forEach` loop registers a `test()`/`it()`/`describe()` call, and no `for`/`.forEach` loop inside a single test calls `expect()` once per logically independent case — either shape MUST be `test.each()`/`describe.each()` instead. +1. Beyond what `test-cases/no-loop-test-cases` reports, no loop inside a test calls `expect()` once per logically independent case over rows that exist at registration time but are not an array literal or `const` array (for example rows returned by a helper call) — that shape MUST be `test.each()`/`describe.each()` too. 2. No `expect(<comparison>).toBe(true)` or `.toBe(false)` where `<comparison>` is itself a boolean expression (`===`, `.some()`, `.every()`, `Array.isArray()`, `.includes()`) — the assertion MUST target the underlying values with a specific matcher. 3. A `test.each()`/`describe.each()` conversion preserves every assertion that ran in the original loop — none dropped, none merged into a single case. diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index b9f64e49..da9af509 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -22,9 +22,11 @@ Exceptions: minor follow-up tweaks after validation already passed, and non-code - **This repo sets `strict: true`** in `.archgate/config.json`, so the `[suppression]`, `[briefing]`, and `[adr]` advisories block `check`, `review-context`, and `adr sync` alike, with no flag passed (ARCH-026). The practical bite: an over-budget ADR section fails the build rather than warning it — see [[adr-briefing-budget]]. - **Commit with `--signoff`** — the DCO check rejects commits without `Signed-off-by`. - **This repo is PUBLIC** — no private sibling-repo internals, no Claude session links in PRs or commits. +- **An ADR restates its enforcement status in up to five places** — the Context alternatives, a Risk with its mitigation, Compliance → Automated Enforcement, Compliance → Manual Enforcement, and the Don'ts. After changing one, grep the whole file for the old claim ("not yet", "future work", the old rule name) before calling the edit done; `check` validates only the section budget, not consistency. +- **A hook-created worktree is cut from whatever branch the primary checkout has out, not from `origin/main`.** Run `git log --oneline origin/main..HEAD` before the first commit; when foreign commits show, confirm they touch none of your files while HEAD is still the hook-created branch (`git diff --name-only origin/main HEAD -- <your files>` must print nothing), then commit the work and `git rebase --onto origin/main HEAD~1` so the PR carries only its own commit. - **Commit before fire-testing a rule or guard.** The loop (mutate → confirm the check fails → restore) restores with `git checkout <file>`, which discards all uncommitted work in that file, including the fix under test. - **Fire-test with `ARCHGATE_TELEMETRY=0` whenever the guard under test throws.** A local `bun run cli` run reports `environment: production` to Sentry, so a guard reaching the exit-2 boundary files a real issue against the product from a deliberately broken tree. A guard reporting an ADR violation is safe; one that throws is not. -- **Fire-test a guard in BOTH directions** — that it blocks the bad case AND still permits the legitimate one. A green suite proves only that the gate closes, not that it isn't over-rejecting. Fire-testing also exposes branches that can never fire: when an earlier pipeline stage aborts on the same condition, the later branch is dead governance (`check` regenerates `rules.d.ts` before any rule runs, so a throw from `generateRulesDts()` preempts any rule testing the same thing). Delete the unreachable branch and name the real enforcement point. +- **Fire-test a guard in BOTH directions** — that it blocks the bad case AND still permits the legitimate one. A regression test for a timing or scheduling bug gets the same treatment: run it against the pre-fix module (`git show origin/main:<path>` copied into place, restored from a saved copy) and make the victim do what the bug needs — a rule that never yields is never charged by a shared clock, so a no-op victim passes on the buggy code. A green suite proves only that the gate closes, not that it isn't over-rejecting. Fire-testing also exposes branches that can never fire: when an earlier pipeline stage aborts on the same condition, the later branch is dead governance (`check` regenerates `rules.d.ts` before any rule runs, so a throw from `generateRulesDts()` preempts any rule testing the same thing). Delete the unreachable branch and name the real enforcement point. - **Confirm a probe or fire-test fixture holds the bytes you meant before believing the result.** A `bun -e` body inside shell single quotes is still parsed as a JS string literal, so an escape written for the file collapses one level on the way in and the injected "violation" lands as ordinary well-formed text — a false negative that looks like a defect in the rule. A quoted `<<'EOF'` heredoc through the Bash tool is not reliably literal either: a backslash-heavy regex probe run that way reported no match for a pattern that matches fine. Write any escape-sensitive probe to a real file with the Write tool instead of inlining it in a shell command, build such fixtures from `String.fromCodePoint` (oxlint's `unicorn/prefer-code-point` rejects `fromCharCode`), and print the file back (`cat -A`) first. The same applies to WHERE a mutation lands: a first-occurrence substitution of `});` in a test file hits the `beforeEach`/`afterEach` closings before any `test(...)`, so a rule that ignores hooks reports nothing and the fire-test reads as a defect in the rule. Target the line numbers of the construct under test and show `git diff -U0` before running the check. - **A Sentry issue whose `install_path` is a local worktree is an artifact of that worktree, not a user report.** Confirm provenance from `install_path`, `is_ci`, and the timestamp — then still ask whether the error was classified correctly, because reaching Sentry at all claims archgate has a bug rather than the caller. - **`--update-snapshots` is never on its own the fix for a failing snapshot.** `tests/helpers/__snapshots__/rules-shim.test.ts.snap` is the entire `rules.d.ts` a governed project receives, and its diff is the review artifact — read every hunk and confirm it follows from an intended `src/formats/rules.ts` edit before regenerating. Deleting the file routes around nothing: Bun fails a missing snapshot whenever `CI` is set, and passes it locally. diff --git a/.oxlintrc.json b/.oxlintrc.json index 1f777106..5548103f 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -7,6 +7,7 @@ "./lint/no-bare-fetch.ts", "./lint/no-shared-module-mock.ts", "./lint/no-lowered-test-timeout.ts", + "./lint/no-loop-test-cases.ts", "./.archgate/lint/oxlint.ts" ], "options": { "typeAware": true }, @@ -141,7 +142,8 @@ "bun-test/expect-expect": "error", "test-mocking/no-shared-module-mock": "error", "test-isolation/no-bare-env-restore": "error", - "test-timeout/no-lowered-test-timeout": "error" + "test-timeout/no-lowered-test-timeout": "error", + "test-cases/no-loop-test-cases": "error" } }, { diff --git a/lint/no-loop-test-cases.ts b/lint/no-loop-test-cases.ts new file mode 100644 index 00000000..47e76312 --- /dev/null +++ b/lint/no-loop-test-cases.ts @@ -0,0 +1,372 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate + +// Custom oxlint JS plugin: a loop must not stand in for `test.each()` and +// must not carry a test's only assertions (ARCH-025). Reports three shapes: +// a registration inside a loop, `expect()` over rows that exist at +// registration time, and a loop holding every `expect()` of a test. + +/** Minimal ESTree-ish node shape. The oxlint AST is ESLint-compatible. */ +type AstNode = { type: string } & Record<string, unknown>; + +function isAstNode(value: unknown): value is AstNode { + return ( + typeof value === "object" && + value !== null && + "type" in value && + typeof value.type === "string" + ); +} + +function asNode(value: unknown): AstNode | undefined { + return isAstNode(value) ? value : undefined; +} + +function asArray(value: unknown): unknown[] { + return Array.isArray(value) ? value : []; +} + +const MODIFIERS: ReadonlySet<string> = new Set([ + "skip", + "only", + "todo", + "skipIf", + "runIf", + "todoIf", + "each", + "concurrent", + "failing", + "if", +]); + +/** + * The bare identifier a registration callee is rooted at: `test`, `it` or + * `describe`, reached only through bun:test modifier members and call links + * (`test.skipIf(c)`, `test.each(rows)`). Any other member (`it.run`, + * `test.tests`) is not a registration. + */ +function registrationRoot(node: AstNode | undefined): string | undefined { + let current = node; + while (current) { + switch (current.type) { + case "Identifier": { + return typeof current.name === "string" && + (current.name === "test" || + current.name === "it" || + current.name === "describe") + ? current.name + : undefined; + } + case "MemberExpression": { + const property = asNode(current.property); + if ( + current.computed === true || + property?.type !== "Identifier" || + typeof property.name !== "string" || + !MODIFIERS.has(property.name) + ) { + return undefined; + } + current = asNode(current.object); + break; + } + case "CallExpression": { + current = asNode(current.callee); + break; + } + default: { + return undefined; + } + } + } + return undefined; +} + +const TS_WRAPPERS: ReadonlySet<string> = new Set([ + "TSAsExpression", + "TSSatisfiesExpression", + "TSNonNullExpression", + "TSTypeAssertion", +]); + +/** Strip TypeScript wrappers (`as`, `satisfies`, `!`, `<T>`) that hide a literal. */ +function unwrap(node: AstNode | undefined): AstNode | undefined { + let current = node; + while (current && TS_WRAPPERS.has(current.type)) { + current = asNode(current.expression); + } + return current; +} + +/** Child nodes of an AST node, skipping back-references and locations. */ +function childNodes(node: AstNode): AstNode[] { + const out: AstNode[] = []; + for (const key of Object.keys(node)) { + if (key === "parent" || key === "loc" || key === "range") continue; + const value = node[key]; + if (Array.isArray(value)) { + for (const item of value) { + const child = asNode(item); + if (child) out.push(child); + } + } else { + const child = asNode(value); + if (child) out.push(child); + } + } + return out; +} + +function isFunction(node: AstNode | undefined): boolean { + return ( + node?.type === "ArrowFunctionExpression" || + node?.type === "FunctionExpression" + ); +} + +function isFunctionScope(node: AstNode): boolean { + return isFunction(node) || node.type === "FunctionDeclaration"; +} + +/** A non-empty array literal; `[]` is a runtime accumulator, not a row table. */ +function isRowTable(node: AstNode | undefined): boolean { + const array = unwrap(node); + return ( + array?.type === "ArrayExpression" && asArray(array.elements).length > 0 + ); +} + +/** + * Names bound by `const NAME = [row, ...]` in `root`'s own scope. Nested + * functions are skipped, except the callbacks of `describe()` calls when + * `intoDescribe` is set, since a data table declared in a describe block is + * visible to every test below it. + */ +function collectStaticArrays( + root: AstNode, + intoDescribe: boolean +): ReadonlySet<string> { + const names = new Set<string>(); + const visit = (node: AstNode, entered: boolean): void => { + if (isFunctionScope(node) && !entered) return; + if (node.type === "VariableDeclaration" && node.kind === "const") { + for (const item of asArray(node.declarations)) { + const declarator = asNode(item); + const id = asNode(declarator?.id); + if ( + id?.type === "Identifier" && + typeof id.name === "string" && + isRowTable(asNode(declarator?.init)) + ) { + names.add(id.name); + } + } + } + const describeCall = + intoDescribe && + node.type === "CallExpression" && + registrationRoot(asNode(node.callee)) === "describe"; + for (const child of childNodes(node)) { + visit(child, describeCall && isFunction(child)); + } + }; + visit(root, true); + return names; +} + +/** True for a non-empty array literal or an identifier bound to one by `const`. */ +function isStaticRows( + node: AstNode | undefined, + staticArrays: ReadonlySet<string> +): boolean { + const target = unwrap(node); + if (!target) return false; + if (target.type === "ArrayExpression") return isRowTable(target); + return ( + target.type === "Identifier" && + typeof target.name === "string" && + staticArrays.has(target.name) + ); +} + +const ITERATION_METHODS: ReadonlySet<string> = new Set([ + "forEach", + "map", + "flatMap", + "filter", + "some", + "every", +]); + +/** The receiver of a `<receiver>.forEach/map/...(...)` call, or undefined for any other call. */ +function iterationReceiver(call: AstNode): AstNode | undefined { + const callee = asNode(call.callee); + if (callee?.type !== "MemberExpression" || callee.computed === true) { + return undefined; + } + const property = asNode(callee.property); + return property?.type === "Identifier" && + typeof property.name === "string" && + ITERATION_METHODS.has(property.name) + ? asNode(callee.object) + : undefined; +} + +/** `expect(...)`, `expect.soft(...)`, `expect.assertions(...)`: not the outer `.toBe()` link. */ +function isExpectCall(callee: AstNode | undefined): boolean { + if (callee?.type === "Identifier") return callee.name === "expect"; + if (callee?.type !== "MemberExpression") return false; + const object = asNode(callee.object); + return object?.type === "Identifier" && object.name === "expect"; +} + +interface ReportDescriptor { + node: AstNode; + message: string; +} + +interface RuleContext { + report(descriptor: ReportDescriptor): void; +} + +interface ExpectSite { + /** Enclosing loops inside the test callback, outermost first. */ + loops: AstNode[]; +} + +interface TestRecord { + expects: ExpectSite[]; + /** Static row tables visible to this test callback. */ + statics: ReadonlySet<string>; +} + +interface WalkContext { + loops: AstNode[]; + test: TestRecord | undefined; +} + +const LOOP_TYPES: ReadonlySet<string> = new Set([ + "ForStatement", + "ForInStatement", + "ForOfStatement", + "WhileStatement", + "DoWhileStatement", +]); + +const LOOP_REGISTRATION_MESSAGE = + "`test()`/`it()`/`describe()` is registered inside a loop. Use `test.each()`/`describe.each()` so each case is its own named test (ARCH-025)."; +const STATIC_ROWS_MESSAGE = + "`expect()` runs once per iteration over rows that exist at registration time. Use `test.each(rows)` so each row is its own named test (ARCH-025)."; +const ONLY_ASSERTIONS_MESSAGE = + "This loop carries the test's only `expect()` calls, so an empty collection asserts nothing. Assert the collection itself (`toEqual([...])`, `toHaveLength(n)`) outside the loop (ARCH-025)."; + +const noLoopTestCases = { + create(context: RuleContext) { + return { + Program(root: AstNode) { + const moduleStatics = collectStaticArrays(root, true); + // The inner call of `test.each(rows)(name, fn)` is the callee of the + // outer one; only the outer call is a registration. + const innerCalls = new Set<AstNode>(); + + const finishTest = (record: TestRecord): void => { + const isStaticLoop = (loop: AstNode): boolean => + (loop.type === "ForOfStatement" && + isStaticRows(asNode(loop.right), record.statics)) || + (loop.type === "CallExpression" && + isStaticRows(iterationReceiver(loop), record.statics)); + const reported = new Set<AstNode>(); + for (const site of record.expects) { + const staticLoop = site.loops.find(isStaticLoop); + if (staticLoop && !reported.has(staticLoop)) { + reported.add(staticLoop); + context.report({ + node: staticLoop, + message: STATIC_ROWS_MESSAGE, + }); + } + } + if (record.expects.some((site) => site.loops.length === 0)) return; + for (const site of record.expects) { + if (site.loops.some((loop) => reported.has(loop))) continue; + const outermost = site.loops.at(0); + if (!outermost) continue; + reported.add(outermost); + context.report({ + node: outermost, + message: ONLY_ASSERTIONS_MESSAGE, + }); + } + }; + + const visit = (node: AstNode, ctx: WalkContext): void => { + if (LOOP_TYPES.has(node.type)) { + const inner = { ...ctx, loops: [...ctx.loops, node] }; + for (const child of childNodes(node)) visit(child, inner); + return; + } + if (node.type !== "CallExpression") { + for (const child of childNodes(node)) visit(child, ctx); + return; + } + + const callee = asNode(node.callee); + if (callee?.type === "CallExpression") innerCalls.add(callee); + + if (isExpectCall(callee) && ctx.test) { + ctx.test.expects.push({ loops: ctx.loops }); + } + + const base = registrationRoot(callee); + if (!innerCalls.has(node) && base !== undefined) { + if (ctx.loops.length > 0) { + context.report({ node, message: LOOP_REGISTRATION_MESSAGE }); + } + const callback = asArray(node.arguments) + .map((arg) => asNode(arg)) + .find((arg) => isFunction(arg)); + const record: TestRecord | undefined = + base === "describe" || !callback + ? undefined + : { + expects: [], + statics: new Set([ + ...moduleStatics, + ...collectStaticArrays(callback, false), + ]), + }; + for (const child of childNodes(node)) { + visit( + child, + child === callback ? { loops: [], test: record } : ctx + ); + } + if (record) finishTest(record); + return; + } + + const callback = iterationReceiver(node) + ? asNode(asArray(node.arguments)[0]) + : undefined; + for (const child of childNodes(node)) { + visit( + child, + child === callback && isFunction(callback) + ? { ...ctx, loops: [...ctx.loops, node] } + : ctx + ); + } + }; + + visit(root, { loops: [], test: undefined }); + }, + }; + }, +}; + +const plugin = { + meta: { name: "test-cases" }, + rules: { "no-loop-test-cases": noLoopTestCases }, +}; + +export default plugin; diff --git a/tests/engine/check-case.test.ts b/tests/engine/check-case.test.ts index 6d786b88..002f54e8 100644 --- a/tests/engine/check-case.test.ts +++ b/tests/engine/check-case.test.ts @@ -89,11 +89,12 @@ describe("checkCase", () => { } ); - test("empty string matches no scheme", () => { - for (const scheme of schemeKeys(cases)) { + test.each(schemeKeys(cases))( + "empty string matches no scheme (%s)", + (scheme) => { expect(checkCase("", scheme)).toBe(false); } - }); + ); test("non-ASCII letters are rejected", () => { expect(checkCase("café-menu", "kebab-case")).toBe(false); diff --git a/tests/formats/adr-fuzz.test.ts b/tests/formats/adr-fuzz.test.ts index 2c564497..b3274c0c 100644 --- a/tests/formats/adr-fuzz.test.ts +++ b/tests/formats/adr-fuzz.test.ts @@ -15,6 +15,17 @@ import { // --------------------------------------------------------------------------- /** Arbitrary that produces strings resembling kebab-case domain names. */ +/** Run one fuzz input through a parser, keeping the thrown value apart from a result. */ +function attempt<T>( + run: () => T +): { ok: true; value: T } | { ok: false; error: unknown } { + try { + return { ok: true, value: run() }; + } catch (error: unknown) { + return { ok: false, error }; + } +} + const domainArb = fc.oneof( fc.stringMatching(/^[a-z][a-z0-9-]{1,31}$/u), fc.constant(""), @@ -69,37 +80,33 @@ describe("parseFrontmatter fuzz", () => { ); }); - test("handles null bytes and control characters", () => { - const inputs = [ - "id: \0", - "title: ", - "domain: éñüßαβγ☃\u{1F680}​ ", - "rules:  true", - "​id: GEN-001", - "id: GEN-001\r\ntitle: test\r\nrules: true\r\ndomain: general", - ]; - for (const input of inputs) { - try { - parseFrontmatter(input); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); - } + test.each([ + "id: \0", + "title: ", + "domain: éñüßαβγ☃\u{1F680}​ ", + "rules:  true", + "​id: GEN-001", + "id: GEN-001\r\ntitle: test\r\nrules: true\r\ndomain: general", + ])("handles null bytes and control characters (%j)", (input) => { + const outcome = attempt(() => parseFrontmatter(input)); + if (outcome.ok) { + expect(outcome.value).toBeDefined(); + } else { + expect(outcome.error).toBeInstanceOf(Error); } }); - test("handles deeply nested YAML structures", () => { - const inputs = [ - "a: {b: {c: {d: {e: {f: {g: 1}}}}}}", - "a:\n b:\n c:\n d:\n e: 1", - "a: [[[[[[1]]]]]]", - "a: &anchor\n b: 1\nc: *anchor", - ]; - for (const input of inputs) { - try { - parseFrontmatter(input); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); - } + test.each([ + "a: {b: {c: {d: {e: {f: {g: 1}}}}}}", + "a:\n b:\n c:\n d:\n e: 1", + "a: [[[[[[1]]]]]]", + "a: &anchor\n b: 1\nc: *anchor", + ])("handles deeply nested YAML structures (%j)", (input) => { + const outcome = attempt(() => parseFrontmatter(input)); + if (outcome.ok) { + expect(outcome.value).toBeDefined(); + } else { + expect(outcome.error).toBeInstanceOf(Error); } }); }); @@ -135,46 +142,41 @@ describe("parseAdr fuzz", () => { ); }); - test("handles adversarial frontmatter delimiters", () => { - const inputs = [ - "------\nid: GEN-001\n------", - "---\n---", - "---\n\n---", - "---\r\nid: GEN-001\r\n---", - "--- \nid: GEN-001\n---", - "---\nid: GEN-001\n--- trailing", - "---\nid: GEN-001\n---\n---\nid: GEN-002\n---", - "\n---\nid: GEN-001\n---", - " ---\nid: GEN-001\n---", - "---\n".repeat(50), - ]; - for (const input of inputs) { - try { - parseAdr(input, "adversarial.md"); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); - } + test.each([ + "------\nid: GEN-001\n------", + "---\n---", + "---\n\n---", + "---\r\nid: GEN-001\r\n---", + "--- \nid: GEN-001\n---", + "---\nid: GEN-001\n--- trailing", + "---\nid: GEN-001\n---\n---\nid: GEN-002\n---", + "\n---\nid: GEN-001\n---", + " ---\nid: GEN-001\n---", + "---\n".repeat(50), + ])("handles adversarial frontmatter delimiters (%j)", (input) => { + const outcome = attempt(() => parseAdr(input, "adversarial.md")); + if (outcome.ok) { + expect(outcome.value).toBeDefined(); + } else { + expect(outcome.error).toBeInstanceOf(Error); } }); - test("handles YAML injection attempts", () => { - const inputs = [ - '---\nid: "GEN-001\\nrules: true"\ntitle: test\ndomain: general\nrules: false\n---', - "---\nid: !!python/object:os.system 'echo pwned'\ntitle: test\ndomain: general\nrules: true\n---", - "---\nid: !<!tag> value\ntitle: test\ndomain: general\nrules: true\n---", - "---\nid: &id GEN-001\ntitle: *id\ndomain: general\nrules: true\n---", - "---\nid: |\n multi\n line\ntitle: test\ndomain: general\nrules: true\n---", - "---\nid: >\n folded\n text\ntitle: test\ndomain: general\nrules: true\n---", - ]; - for (const input of inputs) { - try { - const doc = parseAdr(input, "injection.md"); - expect(doc).toHaveProperty("frontmatter"); - expect(doc).toHaveProperty("body"); - expect(doc).toHaveProperty("filePath"); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); - } + test.each([ + '---\nid: "GEN-001\\nrules: true"\ntitle: test\ndomain: general\nrules: false\n---', + "---\nid: !!python/object:os.system 'echo pwned'\ntitle: test\ndomain: general\nrules: true\n---", + "---\nid: !<!tag> value\ntitle: test\ndomain: general\nrules: true\n---", + "---\nid: &id GEN-001\ntitle: *id\ndomain: general\nrules: true\n---", + "---\nid: |\n multi\n line\ntitle: test\ndomain: general\nrules: true\n---", + "---\nid: >\n folded\n text\ntitle: test\ndomain: general\nrules: true\n---", + ])("handles YAML injection attempts (%j)", (input) => { + const outcome = attempt(() => parseAdr(input, "injection.md")); + if (outcome.ok) { + expect(outcome.value).toHaveProperty("frontmatter"); + expect(outcome.value).toHaveProperty("body"); + expect(outcome.value).toHaveProperty("filePath"); + } else { + expect(outcome.error).toBeInstanceOf(Error); } }); }); @@ -223,23 +225,20 @@ describe("AdrFrontmatterSchema fuzz", () => { ); }); - test("handles extreme field values", () => { - const cases = [ - { id: "", title: "", domain: "", rules: false }, - { - id: "a".repeat(10_000), - title: "b".repeat(10_000), - domain: "c".repeat(10_000), - rules: true, - }, - { id: "\0\0\0", title: "\n\r\t", domain: "​", rules: true }, - { id: 0, title: false, domain: [], rules: "yes" }, - { id: null, title: undefined, domain: NaN, rules: Infinity }, - { id: {}, title: [], domain: () => {}, rules: Symbol("test") }, - ]; - for (const c of cases) { - const result = AdrFrontmatterSchema.safeParse(c); - expect(result).toHaveProperty("success"); - } + test.each([ + { id: "", title: "", domain: "", rules: false }, + { + id: "a".repeat(10_000), + title: "b".repeat(10_000), + domain: "c".repeat(10_000), + rules: true, + }, + { id: "\0\0\0", title: "\n\r\t", domain: "​", rules: true }, + { id: 0, title: false, domain: [], rules: "yes" }, + { id: null, title: undefined, domain: NaN, rules: Infinity }, + { id: {}, title: [], domain: () => {}, rules: Symbol("test") }, + ])("handles extreme field values (case %#)", (c) => { + const result = AdrFrontmatterSchema.safeParse(c); + expect(result).toHaveProperty("success"); }); }); diff --git a/tests/lint/no-loop-test-cases.test.ts b/tests/lint/no-loop-test-cases.test.ts new file mode 100644 index 00000000..bcfa17d4 --- /dev/null +++ b/tests/lint/no-loop-test-cases.test.ts @@ -0,0 +1,339 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +import { expect, test } from "bun:test"; + +import plugin from "../../lint/no-loop-test-cases"; +import { parseJsModule } from "../../src/engine/js-parser"; + +/** Minimal ESTree-ish node shape, matching the plugin's own definition. */ +type AstNode = { type: string } & Record<string, unknown>; + +interface ReportedViolation { + message: string; +} + +/** + * Run the `test-cases/no-loop-test-cases` rule against a source snippet and + * return every reported violation. Parses via the in-process `meriyah` + * entry point (`parseJsModule`, ARCH-022) the engine itself uses. + */ +function lint(source: string): ReportedViolation[] { + // meriyah's Program type structurally satisfies the plugin's own loose + // AstNode shape; the plugin doesn't export a type to convert through. + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + const program = parseJsModule(source) as unknown as AstNode; + const violations: ReportedViolation[] = []; + const rule = plugin.rules["no-loop-test-cases"]; + const visitor = rule.create({ + report({ message }) { + violations.push({ message }); + }, + }); + visitor.Program(program); + return violations; +} + +const REGISTRATION = "registered inside a loop"; +const STATIC_ROWS = "rows that exist at registration time"; +const ONLY = "only `expect()` calls"; + +test.each([ + { + name: "for...of registering test()", + source: `for (const c of cases) { test("x " + c, () => { expect(c).toBe(1); }); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: ".forEach registering it()", + source: `cases.forEach((c) => { it("x", () => { expect(c).toBe(1); }); });`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "for registering describe() whose body registers test (one report)", + source: `for (let i = 0; i < 3; i++) { describe("d", () => { test("x", () => { expect(i).toBe(1); }); }); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "test.each(rows)(...) inside a for", + source: `for (const g of groups) { test.each(rows)("x %s", (r) => { expect(r).toBe(g); }); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "test.skipIf(c)(...) inside a while", + source: `while (more()) { test.skipIf(c)("x", () => { expect(1).toBe(1); }); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "describe.each(rows)(...) at top level", + source: `describe.each(rows)("d %s", (r) => { test("x", () => { expect(r).toBe(1); }); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "array literal for...of inside a test", + source: `test("t", () => { for (const c of [1, 2]) { expect(f(c)).toBe(c); } });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "const array identifier for...of inside a test", + source: `const CASES = [1, 2]; test("t", () => { for (const c of CASES) expect(f(c)).toBe(c); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: ".forEach over a const array inside a test", + source: `const CASES = [1, 2]; test("t", () => { CASES.forEach((c) => expect(f(c)).toBe(c)); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: ".forEach over an array literal inside a test", + source: `test("t", () => { [1, 2].forEach((c) => { expect(f(c)).toBe(c); }); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "static rows still flag with another expect outside the loop", + source: `test("t", () => { expect(g()).toBe(1); for (const c of [1, 2]) { expect(f(c)).toBe(c); } });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "negated expect over static rows", + source: `test("t", () => { for (const c of [1, 2]) { expect(f(c)).not.toBe(0); } });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "two expects in one static loop report once", + source: `test("t", () => { for (const c of [1, 2]) { expect(f(c)).toBe(c); expect(g(c)).toBe(c); } });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "runtime for...of carrying the only expect", + source: `test("t", () => { for (const r of results) { expect(r.ok).toBe(true); } });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "while loop carrying the only expect", + source: `test("t", () => { while (next()) { expect(cur()).toBe(1); } });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "do...while loop carrying the only expect", + source: `test("t", () => { do { expect(cur()).toBe(1); } while (next()); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: ".forEach over a non-literal identifier carrying the only expect", + source: `test("t", () => { results.forEach((r) => { expect(r.ok).toBe(true); }); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "nested runtime loops report the outermost once", + source: `test("t", () => { for (const a of as) { for (const b of a.items) { expect(b).toBe(1); } } });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "let-bound array carrying the only expect is a runtime loop", + source: `let rows = [1, 2]; test("t", () => { for (const r of rows) { expect(r).toBe(1); } });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "guarded shape: expect outside plus per-item expects", + source: `test("t", () => { expect(results).toHaveLength(3); for (const r of results) expect(r.ok).toBe(true); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "loop that only builds fixtures before the assertion", + source: `test("t", () => { const out = []; for (const r of results) { out.push(r.name); } expect(out).toEqual(["a"]); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "guarded Promise.all(items.map(...))", + source: `test("t", async () => { expect(items).toHaveLength(2); await Promise.all(items.map(async (i) => { expect(await f(i)).toBe(1); })); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "unguarded Promise.all(items.map(...)) carries the only expects", + source: `test("t", async () => { await Promise.all(items.map(async (i) => { expect(await f(i)).toBe(1); })); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: ".map callback carrying the only expect", + source: `test("t", () => { items.map((i) => expect(i).toBe(1)); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: ".some callback carrying the only expect", + source: `test("t", () => { items.some((i) => expect(i).toBe(1)); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: ".map over an array literal", + source: `test("t", () => { [1, 2].map((x) => expect(f(x)).toBe(x)); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "a callback outside the iteration methods is not a loop", + source: `test("t", () => { run(items, (i) => expect(i).toBe(1)); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "an `it` variable that is not a registration", + source: `for (const it of items) { it.run(); }`, + expectedCount: 0, + keyword: "", + }, + { + name: "a `test` variable that is not a registration", + source: `for (const test of suite.tests) { test.run(); }`, + expectedCount: 0, + keyword: "", + }, + { + name: "test.concurrent registered in a loop", + source: `for (const c of rows) { test.concurrent(c.name, () => {}); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "local const table in one test, runtime value of the same name in another", + source: `test("a", () => { const cases = [1, 2]; for (const c of cases) expect(c).toBe(c); }); test("b", async () => { const cases = await load(); expect(cases).toHaveLength(2); for (const c of cases) expect(c).toBe(c); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "empty array accumulator is a runtime collection", + source: `test("t", () => { const found = []; found.push(1); for (const f of found) expect(f).toBe(1); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "const table declared in a describe block", + source: `describe("d", () => { const ROWS = [1, 2]; test("t", () => { for (const r of ROWS) expect(r).toBe(r); }); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "expect.soft outside the loop guards a runtime loop", + source: `test("t", () => { expect.soft(results.length).toBeGreaterThan(0); for (const r of results) expect(r.ok).toBe(true); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "expect inside a loop in a beforeEach hook", + source: `beforeEach(() => { for (const r of results) { expect(r.ok).toBe(true); } });`, + expectedCount: 0, + keyword: "", + }, + { + name: "loop in a describe callback outside any test", + source: `describe("d", () => { for (const r of [1, 2]) { expect(r).toBe(1); } });`, + expectedCount: 0, + keyword: "", + }, + { + name: "test.each(rows) with a loop-free body", + source: `test.each(rows)("x %s", (r) => { expect(f(r)).toBe(1); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "expect inside a nested arrow helper that is not a loop", + source: `test("t", () => { const check = (x) => { expect(x).toBe(1); }; check(1); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "test with no loop at all", + source: `test("t", () => { expect(1).toBe(1); });`, + expectedCount: 0, + keyword: "", + }, +])("$name", ({ source, expectedCount, keyword }) => { + const violations = lint(source); + expect(violations).toHaveLength(expectedCount); + expect(violations.filter((v) => !v.message.includes(keyword))).toEqual([]); +}); + +test.each([ + { + shape: "registration inside a loop", + source: `for (const c of cases) { test("x", () => { expect(c).toBe(1); }); }`, + }, + { + shape: "expect over static rows", + source: `test("t", () => { for (const c of [1]) { expect(c).toBe(1); } });`, + }, + { + shape: "loop carrying the only expects", + source: `test("t", () => { for (const r of results) { expect(r).toBe(1); } });`, + }, +])("the $shape message names ARCH-025", ({ source }) => { + const violations = lint(source); + expect(violations).toHaveLength(1); + expect(violations[0]?.message).toContain("(ARCH-025)"); +}); + +function findDeclarator(node: AstNode): AstNode | undefined { + if (node.type === "VariableDeclarator") return node; + for (const value of Object.values(node)) { + for (const item of Array.isArray(value) ? value : [value]) { + if (typeof item === "object" && item !== null && "type" in item) { + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + const found = findDeclarator(item as AstNode); + if (found) return found; + } + } + } + return undefined; +} + +// oxlint's TypeScript AST wraps `[...] as const` and similar in a +// TSAsExpression; meriyah cannot parse those, so the wrapper is added to the +// parsed tree by hand. +test("static rows behind a TypeScript wrapper are still static", () => { + const program = parseJsModule( + `const CASES = [1, 2]; test("x", () => { for (const c of CASES) expect(c).toBe(c); });` + ); + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + const ast = program as unknown as AstNode; + const declarator = findDeclarator(ast); + if (!declarator) throw new Error("fixture has no declarator"); + declarator.init = { + type: "TSAsExpression", + expression: declarator.init, + typeAnnotation: { type: "TSTypeReference" }, + }; + const violations: ReportedViolation[] = []; + const visitor = plugin.rules["no-loop-test-cases"].create({ + report({ message }) { + violations.push({ message }); + }, + }); + visitor.Program(ast); + expect(violations).toHaveLength(1); + expect(violations[0]?.message).toContain(STATIC_ROWS); +});