From 7ee8a14e56731b08d95c8c5db5823218f7c1bd84 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 10:46:28 -0400 Subject: [PATCH] fix: address Copilot review round 1 on #2323 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All nine findings taken. Two budgets I had lowered are restored as named exceptions rather than folded into the shared value. cli's e2e child timer goes back to 15s and the two tests that spawn it get E2E_SPAWN_MS = 25s, so the bound carrying the diagnostic still wins without taking load tolerance off the one suite that spawns a real process. App.test.tsx's three-modal sequence keeps THREE_MODAL_SEQUENCE_MS = 20s — a value chosen against an observed failure, which the shared ceiling would have replaced with a general one. Three real false-positive paths in the guard are closed. It now discovers every Vitest config under clients/ and fails on one it is not configured to check — without which "a project with no row is an error" was vacuous, since an undiscovered config yields no project to reject. It strips comments before looking for configure() and requires the exact expected value, so neither a commented-out call nor asyncUtilTimeout: 1 passes. And it compares each resolved project's setupFiles against the site it claims to configure, so deleting a setupFiles entry can no longer leave both projects reported as configured while Testing Library silently reverts to 1000ms. Each has a test, and the setupFiles check was observed to fire against a real deletion. Three stale comments the change had invalidated: main.yml still counted four durable guards, PreviewAnnotations.stories.tsx still said the storybook project had no setupFiles, and the new AGENTS.md rule read as though no Group F row had been settled when this PR settles F6. browser-timeouts.test.mjs gains the purpose header this repo's files carry. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- .github/workflows/main.yml | 5 +- AGENTS.md | 5 +- clients/cli/__tests__/e2e.test.ts | 61 ++++--- clients/web/src/App.test.tsx | 107 ++++++------ .../src/test/PreviewAnnotations.stories.tsx | 11 +- scripts/lib/browser-timeouts.test.mjs | 13 ++ scripts/verify-test-timeouts.mjs | 158 +++++++++++++++++- scripts/verify-test-timeouts.test.mjs | 97 +++++++++++ 8 files changed, 376 insertions(+), 81 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f2f4c0f5..96efe0c1 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -38,8 +38,9 @@ jobs: run: npm install - name: Validate (coverage guards, format, lint, typecheck, build, fast tests) - # Runs the four durable guards first (verify:format-coverage, - # verify:skills, verify:typecheck-coverage, verify:dep-lockstep), then + # Runs the five durable guards first (verify:format-coverage, + # verify:skills, verify:typecheck-coverage, verify:dep-lockstep, + # verify:test-timeouts), then # test:scripts, then validate:core, then each client's # self-validation: format:check + lint + typecheck + build + test (no # coverage instrumentation — fast). This also builds every client bundle diff --git a/AGENTS.md b/AGENTS.md index b645dff6..eb0b13ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -431,7 +431,10 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race. - **`retry` stays unset.** A retry turns a load-induced red into a silent green on the only pre-push gate this repo has, and hides a real race behind a second attempt. The guard enforces this. -- **Group F's smoke and CI budgets are tracked on #2333**, not settled here. +- **The Playwright locator budgets the web smokes use are named constants in + `scripts/lib/browser-timeouts.mjs`** — raise one there, not at a call site. + The *remaining* smoke-helper budgets and CI's missing `timeout-minutes` are + tracked on #2333 rather than settled here. - **Do not scale a fixed sleep.** A `setTimeout(r, N)` with no condition is not a timeout: it always waits the full window, so raising it slows every passing run and still races on a loaded one. Replace one with a condition wait when it diff --git a/clients/cli/__tests__/e2e.test.ts b/clients/cli/__tests__/e2e.test.ts index b6273ae0..e12932c8 100644 --- a/clients/cli/__tests__/e2e.test.ts +++ b/clients/cli/__tests__/e2e.test.ts @@ -8,15 +8,25 @@ const here = dirname(fileURLToPath(import.meta.url)); const BIN = resolve(here, "../build/index.js"); /** - * In-test bound on the spawned child, deliberately BELOW the cli project's own - * `testTimeout` (15000, from `vitest.shared.mts`). The two used to be equal, so - * which one fired was arbitrary — and they do different things: this one kills - * the child's process group and rejects with a message naming the CLI, while - * the project budget just fails the test with a generic timeout and leaves a - * detached `node` behind. The useful diagnostic is here, so this is the bound - * that must win (#2323). + * The two bounds on a spawned child, which used to be the identical 15000 — + * so which one fired was arbitrary, and they do different things (#2323). + * + * `CHILD_TIMEOUT_MS` kills the child's process group and rejects with a message + * naming the CLI. The enclosing per-test budget just fails the test generically + * and leaves a detached `node` behind. The useful diagnostic is the child + * timer's, so it has to be the bound that wins — and the way to arrange that is + * to raise the test's budget above it, not to shrink the child's. Shrinking it + * would take load tolerance away from the one suite here that spawns a real + * process, which is the opposite of this change's purpose (Copilot). + * + * `E2E_SPAWN_MS` is therefore an exception to the cli project's shared 15000, + * and one of the few sites that should be: booting the built binary in a fresh + * `node` and waiting for it to speak to an MCP server is elapsed work, not + * slack. It is the child's own budget plus room for the spawn and the reject to + * be observed. */ -const CHILD_TIMEOUT_MS = 12_000; +const CHILD_TIMEOUT_MS = 15_000; +const E2E_SPAWN_MS = 25_000; interface SpawnResult { exitCode: number | null; @@ -64,18 +74,31 @@ function spawnCli(args: string[]): Promise { describe("CLI binary (out-of-process E2E)", () => { const { command, args } = getTestMcpServerCommand(); - it("exits 0 and prints tools JSON on a successful run", async () => { - const result = await spawnCli([command, ...args, "--method", "tools/list"]); + it( + "exits 0 and prints tools JSON on a successful run", + async () => { + const result = await spawnCli([ + command, + ...args, + "--method", + "tools/list", + ]); - expect(result.exitCode).toBe(0); - const json = JSON.parse(result.stdout); - expect(Array.isArray(json.tools)).toBe(true); - }); + expect(result.exitCode).toBe(0); + const json = JSON.parse(result.stdout); + expect(Array.isArray(json.tools)).toBe(true); + }, + E2E_SPAWN_MS, + ); - it("exits non-zero when required --method is missing", async () => { - const result = await spawnCli([command, ...args]); + it( + "exits non-zero when required --method is missing", + async () => { + const result = await spawnCli([command, ...args]); - expect(result.exitCode).not.toBe(0); - expect(result.stderr).toContain("Method is required"); - }); + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain("Method is required"); + }, + E2E_SPAWN_MS, + ); }); diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 40f54d91..64bb3cfc 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -4295,58 +4295,67 @@ describe("App dedupes concurrent OAuth clears that share a storage key (#2217)", ); } - // Three full modal interaction sequences against the whole App tree. This - // used to carry `{ timeout: 20000 }` because the project ran on Vitest's 5s - // default and this test tripped it under load; the project now states 15000 - // itself (#2323), so the raise has nothing left to say that the shared value - // does not. - it("suppresses a second clear for another entry with the same URL, and allows one after it settles", async () => { - // `delay: null` drops userEvent's inter-event waits, which dominate here. - const user = userEvent.setup({ delay: null }); - renderWithMantine(); + // Three full modal interaction sequences against the whole App tree — three + // times the work of any other test in this file, and the only one here that + // ever tripped the old 5s default under load. It keeps its own budget after + // #2323 rather than folding into the shared 15000: this is the "genuinely + // different work" exception that change establishes, and the 20000 was a + // value somebody had already chosen against an observed failure. Lowering it + // to the shared ceiling would trade a measured number for a general one and + // could reintroduce the flake (Copilot). + const THREE_MODAL_SEQUENCE_MS = 20_000; - await user.click(screen.getByText("connect")); - await waitFor(() => expect(clientInstances).toHaveLength(1)); - // The instances are typed `EventTarget`; an intersection names the - // test-only spy without erasing that (Copilot). - const client = clientInstances[0] as EventTarget & { - clearOAuthTokens: ReturnType; - }; + it( + "suppresses a second clear for another entry with the same URL, and allows one after it settles", + async () => { + // `delay: null` drops userEvent's inter-event waits, which dominate here. + const user = userEvent.setup({ delay: null }); + renderWithMantine(); - // Hold the first clear open, as a pending RFC 7009 request would. - let settle: (v: { status: string; reason: string }) => void = () => {}; - client.clearOAuthTokens.mockImplementation( - () => - new Promise((resolve) => { - settle = resolve as typeof settle; - }), - ); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + // The instances are typed `EventTarget`; an intersection names the + // test-only spy without erasing that (Copilot). + const client = clientInstances[0] as EventTarget & { + clearOAuthTokens: ReturnType; + }; - await clearFromSettings(user, "open-settings"); - await waitFor(() => - expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1), - ); + // Hold the first clear open, as a pending RFC 7009 request would. + let settle: (v: { status: string; reason: string }) => void = () => {}; + client.clearOAuthTokens.mockImplementation( + () => + new Promise((resolve) => { + settle = resolve as typeof settle; + }), + ); - // Entry B: a different catalog id, the same OAuth storage key. Both clears - // route through the live client precisely because they share that key, so - // an id-keyed guard would let this second one straight through. - await clearFromSettings(user, "open-settings-b"); - expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1); + await clearFromSettings(user, "open-settings"); + await waitFor(() => + expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1), + ); - // Suppression is for the duration of the in-flight clear only — the key - // must be released when it settles, or the control is dead for the rest of - // the session. - await act(async () => { - settle({ status: "skipped", reason: "no_endpoint" }); - await Promise.resolve(); - }); - client.clearOAuthTokens.mockResolvedValue({ - status: "skipped", - reason: "no_endpoint", - }); - await clearFromSettings(user, "open-settings-b"); - await waitFor(() => - expect(client.clearOAuthTokens).toHaveBeenCalledTimes(2), - ); - }); + // Entry B: a different catalog id, the same OAuth storage key. Both clears + // route through the live client precisely because they share that key, so + // an id-keyed guard would let this second one straight through. + await clearFromSettings(user, "open-settings-b"); + expect(client.clearOAuthTokens).toHaveBeenCalledTimes(1); + + // Suppression is for the duration of the in-flight clear only — the key + // must be released when it settles, or the control is dead for the rest of + // the session. + await act(async () => { + settle({ status: "skipped", reason: "no_endpoint" }); + await Promise.resolve(); + }); + client.clearOAuthTokens.mockResolvedValue({ + status: "skipped", + reason: "no_endpoint", + }); + await clearFromSettings(user, "open-settings-b"); + await waitFor(() => + expect(client.clearOAuthTokens).toHaveBeenCalledTimes(2), + ); + }, + THREE_MODAL_SEQUENCE_MS, + ); }); diff --git a/clients/web/src/test/PreviewAnnotations.stories.tsx b/clients/web/src/test/PreviewAnnotations.stories.tsx index c05b0ae4..14e01cc0 100644 --- a/clients/web/src/test/PreviewAnnotations.stories.tsx +++ b/clients/web/src/test/PreviewAnnotations.stories.tsx @@ -8,8 +8,15 @@ import { theme } from "../theme/theme"; // `.storybook/vitest.setup.ts` used to call `setProjectAnnotations([...])` to // hand the preview annotations to the Storybook vitest project. Since Storybook // 10.3 `@storybook/addon-vitest` applies them automatically — and *skips* doing -// so when it finds such a setup file — so the file was removed and the -// `setupFiles` entry dropped from the `storybook` project in `vite.config.ts`. +// so when it finds a setup file that is both inside `configDir` and calls +// `setProjectAnnotations` — so that file was removed. +// +// The project does have a `setupFiles` entry again (#2323), and it is a +// different thing: `src/test/storybookSetup.ts` configures Testing Library's +// `asyncUtilTimeout` and nothing else. It satisfies *neither* half of the +// condition above — it lives outside `.storybook/` and names no annotations — +// so the automatic path is still the one in use, and this guard still asserts +// that it worked. // // A green suite is not evidence that the automatic path works. Two things the // setup file used to provision are silently losable: diff --git a/scripts/lib/browser-timeouts.test.mjs b/scripts/lib/browser-timeouts.test.mjs index f646e779..bdab308b 100644 --- a/scripts/lib/browser-timeouts.test.mjs +++ b/scripts/lib/browser-timeouts.test.mjs @@ -1,3 +1,16 @@ +/** + * Pins the invariants of `browser-timeouts.mjs` — a constants module, so what + * is testable about it is the shape of the values rather than any logic. + * + * Worth pinning anyway, because the values are the module's whole contract and + * a smoke cannot report a bad one. A budget that arrived as `undefined` or a + * string reaches Playwright as "no timeout" or NaN and a smoke then hangs + * against the job's own ceiling rather than failing with a cause. And the + * ordering below IS the semantics documented on each key: the names say what + * each budget has to absorb, so a `nested` wait outliving its enclosing + * `roundTrip` one would mean the names had stopped describing the values. + */ + import test from "node:test"; import assert from "node:assert/strict"; diff --git a/scripts/verify-test-timeouts.mjs b/scripts/verify-test-timeouts.mjs index b4ed7e53..6fab43b4 100644 --- a/scripts/verify-test-timeouts.mjs +++ b/scripts/verify-test-timeouts.mjs @@ -39,8 +39,8 @@ * both `asyncUtilTimeout` assertions unmet. */ -import { existsSync, readFileSync } from "node:fs"; -import { dirname, join, resolve } from "node:path"; +import { existsSync, readdirSync, readFileSync } from "node:fs"; +import { dirname, join, relative, resolve } from "node:path"; import { fileURLToPath } from "node:url"; const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); @@ -114,6 +114,19 @@ export const ASYNC_UTIL_SITES = Object.freeze([ }, ]); +/** + * The value those sites must configure — stated here for the same reason the + * Vitest budgets are: a guard that accepts any number at all would pass on + * `asyncUtilTimeout: 1` (Copilot). + */ +export const EXPECTED_ASYNC_UTIL_TIMEOUT = 5_000; + +/** + * Directory every Vitest config in this repo lives one level under. Discovery + * (below) walks it rather than trusting a hand-written list. + */ +export const CLIENTS_DIR = "clients"; + /** * Compare one resolved project against its row. * @@ -152,25 +165,151 @@ export function checkProject(name, config, expected = EXPECTED_PROJECTS) { } /** - * Does this source configure `asyncUtilTimeout`, through the right import? + * Strip `//` and block comments so a commented-out call cannot satisfy a check. + * + * Comment-aware rather than exact, deliberately: a naive scan of the raw source + * would accept a `// configure({ asyncUtilTimeout: 5000 })` left behind by + * someone disabling it, which is the most likely way this stops being + * configured (Copilot). String literals are not parsed out — a `configure(` in + * a string would still be accepted — but these two files are fifteen lines each + * and the false-positive that matters is the commented one. + * + * @param {string} source + * @returns {string} + */ +export function stripComments(source) { + return source + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/(^|\n)\s*\/\/[^\n]*/g, "$1"); +} + +/** + * Does this source configure `asyncUtilTimeout` at the expected value, through + * the right import? * * Deliberately a source check and not a runtime one: the value only exists * inside a running test environment, and a `configure` call reached by no - * project would satisfy a runtime probe of this module just as well. + * project would satisfy a runtime probe of this module just as well. What + * closes that second gap is `checkSetupFilesWiring` below, which asks the + * resolved project whether it actually loads the file. * * @param {string} source * @param {string} from module the `configure` must come from + * @param {number} [expected] the value it must configure * @returns {string[]} */ -export function checkAsyncUtilSource(source, from) { +export function checkAsyncUtilSource( + source, + from, + expected = EXPECTED_ASYNC_UTIL_TIMEOUT, +) { + const code = stripComments(source); const failures = []; - if (!new RegExp(`from\\s+["']${from.replace("/", "\\/")}["']`).test(source)) { + if (!new RegExp(`from\\s+["']${from.replace(/\//g, "\\/")}["']`).test(code)) { failures.push(`does not import from "${from}"`); } - if (!/\bconfigure\s*\(/.test(source)) { + if (!/\bconfigure\s*\(/.test(code)) { failures.push("does not call configure()"); - } else if (!/asyncUtilTimeout\s*:\s*\d+/.test(source)) { + return failures; + } + const match = /asyncUtilTimeout\s*:\s*(\d[\d_]*)/.exec(code); + if (!match) { failures.push("calls configure() without an asyncUtilTimeout"); + } else if (Number(match[1].replace(/_/g, "")) !== expected) { + failures.push( + `configures asyncUtilTimeout as ${match[1]}, expected ${expected}`, + ); + } + return failures; +} + +/** + * Is each declared site actually loaded by the project it claims to configure? + * + * Without this the guard reads two files and reports both projects configured + * while `setupFiles` had been deleted from `vite.config.ts` and Testing Library + * had silently gone back to its 1000ms default (Copilot). Vitest resolves + * `setupFiles` to absolute paths, so compare by suffix against the declared + * repo-relative path. + * + * @param {string} name project name as this guard knows it + * @param {unknown} setupFiles the project's resolved `setupFiles` + * @param {readonly {file: string, project: string}[]} [sites] + * @returns {string[]} + */ +export function checkSetupFilesWiring( + name, + setupFiles, + sites = ASYNC_UTIL_SITES, +) { + const site = sites.find((s) => s.project === name); + if (!site) return []; + const loaded = Array.isArray(setupFiles) ? setupFiles : []; + const wanted = site.file.split("/").join("/"); + const found = loaded.some( + (f) => typeof f === "string" && f.split("\\").join("/").endsWith(wanted), + ); + return found + ? [] + : [ + `project "${name}" does not load ${site.file} as a setupFile, so its ` + + `asyncUtilTimeout never takes effect`, + ]; +} + +/** + * Every Vitest config under `clients/`, as repo-relative directories. + * + * Discovery rather than the hand-written `CONFIG_ROOTS` list, so that a new + * client with its own config cannot go unwatched — which would have made the + * "a project with no row is an error" promise vacuous, since an undiscovered + * config yields no project to reject (Copilot). `CONFIG_ROOTS` still exists to + * say which project names each config must produce; this is what proves the + * list is complete. + * + * @param {string} [root] absolute repo root + * @returns {string[]} + */ +export function discoverConfigRoots(root = repoRoot) { + const clients = resolve(root, CLIENTS_DIR); + if (!existsSync(clients)) return []; + return readdirSync(clients, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => join(clients, e.name)) + .filter((dir) => + ["vitest.config.ts", "vite.config.ts"].some((f) => + existsSync(join(dir, f)), + ), + ) + .map((dir) => relative(root, dir).split("\\").join("/")) + .sort(); +} + +/** + * Compare what is on disk against what this guard is configured to check. + * + * @param {string[]} discovered + * @param {readonly {root: string}[]} [configured] + * @returns {string[]} + */ +export function checkConfigRootCoverage(discovered, configured = CONFIG_ROOTS) { + const known = new Set(configured.map((c) => c.root)); + const failures = []; + for (const root of discovered) { + if (!known.has(root)) { + failures.push( + `${root} has a Vitest config that this guard does not check — add it to ` + + `CONFIG_ROOTS and EXPECTED_PROJECTS rather than leaving its budgets unstated`, + ); + } + } + for (const root of known) { + if (!discovered.includes(root)) { + failures.push( + `CONFIG_ROOTS names ${root}, which has no Vitest config on disk — a stale ` + + `row silently stops checking anything`, + ); + } } return failures; } @@ -243,6 +382,9 @@ async function main() { checked += 1; failures.push( ...checkProject(name, project.config).map((f) => `${root}: ${f}`), + ...checkSetupFilesWiring(name, project.config.setupFiles).map( + (f) => `${root}: ${f}`, + ), ); } for (const name of expectedNames) { diff --git a/scripts/verify-test-timeouts.test.mjs b/scripts/verify-test-timeouts.test.mjs index e9fffede..b5cc6cb2 100644 --- a/scripts/verify-test-timeouts.test.mjs +++ b/scripts/verify-test-timeouts.test.mjs @@ -18,12 +18,17 @@ import assert from "node:assert/strict"; import { ASYNC_UTIL_SITES, CONFIG_ROOTS, + EXPECTED_ASYNC_UTIL_TIMEOUT, EXPECTED_INTEGRATION_TIMEOUTS, EXPECTED_PROJECTS, EXPECTED_TIMEOUTS, checkAsyncUtilSource, + checkConfigRootCoverage, checkProject, + checkSetupFilesWiring, + discoverConfigRoots, identifyProject, + stripComments, } from "./verify-test-timeouts.mjs"; const ok = { ...EXPECTED_TIMEOUTS }; @@ -148,3 +153,95 @@ test("both web projects have an asyncUtilTimeout site", () => { "unit", ]); }); + +test("a commented-out configure() does not satisfy the check", () => { + // The most likely way this stops being configured is someone disabling it in + // place, and a raw-source scan would read that as configured (Copilot). + const lineComment = `import { configure } from "storybook/test"; +// configure({ asyncUtilTimeout: 5000 });`; + assert.deepEqual(checkAsyncUtilSource(lineComment, "storybook/test"), [ + "does not call configure()", + ]); + + const blockComment = `import { configure } from "storybook/test"; +/* configure({ asyncUtilTimeout: 5000 }); */`; + assert.deepEqual(checkAsyncUtilSource(blockComment, "storybook/test"), [ + "does not call configure()", + ]); +}); + +test("the configured value must be the expected one, not merely present", () => { + const source = `import { configure } from "storybook/test"; +configure({ asyncUtilTimeout: 1 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), [ + `configures asyncUtilTimeout as 1, expected ${EXPECTED_ASYNC_UTIL_TIMEOUT}`, + ]); +}); + +test("a numeric separator in the configured value is read, not rejected", () => { + const source = `import { configure } from "storybook/test"; +configure({ asyncUtilTimeout: 5_000 });`; + assert.deepEqual(checkAsyncUtilSource(source, "storybook/test"), []); +}); + +test("stripComments leaves executable code alone", () => { + const source = `/** header */ +import { configure } from "storybook/test"; // trailing +configure({ asyncUtilTimeout: 5000 });`; + const code = stripComments(source); + assert.match(code, /configure\(\{ asyncUtilTimeout: 5000 \}\)/); + assert.doesNotMatch(code, /header/); +}); + +test("a project that does not load its declared setup file is caught", () => { + // Reading the file proves it configures the timeout; only this proves the + // project ever loads it. Deleting the setupFiles entry used to leave the + // guard reporting both projects configured (Copilot). + const failures = checkSetupFilesWiring("unit", []); + assert.equal(failures.length, 1); + assert.match(failures[0], /does not load .*setup\.ts as a setupFile/); +}); + +test("a setup file loaded under an absolute path is recognized", () => { + assert.deepEqual( + checkSetupFilesWiring("unit", [ + "/repo/clients/web/src/test/setup.ts", + "/repo/other.ts", + ]), + [], + ); +}); + +test("a project with no declared site is not required to load one", () => { + assert.deepEqual(checkSetupFilesWiring("cli", []), []); + assert.deepEqual(checkSetupFilesWiring("cli", undefined), []); +}); + +test("a Vitest config this guard does not check is an error", () => { + const failures = checkConfigRootCoverage([ + "clients/web", + "clients/cli", + "clients/tui", + "clients/launcher", + "clients/desktop", + ]); + assert.equal(failures.length, 1); + assert.match(failures[0], /clients\/desktop has a Vitest config/); +}); + +test("a stale row naming a config that no longer exists is an error", () => { + const failures = checkConfigRootCoverage(["clients/web", "clients/cli"]); + assert.equal(failures.length, 2); + for (const f of failures) assert.match(f, /has no Vitest config on disk/); +}); + +test("discovery finds exactly the configs this guard is set up to check", () => { + // The two halves have to agree against the real repo, not only against + // fixtures — that agreement is what makes the unknown-project check below + // mean anything. + assert.deepEqual( + discoverConfigRoots().sort(), + CONFIG_ROOTS.map((c) => c.root).sort(), + ); + assert.deepEqual(checkConfigRootCoverage(discoverConfigRoots()), []); +});