diff --git a/apps/web/AGENTS.md b/apps/web/AGENTS.md index dc3a5d8c4a..928fb38cd5 100644 --- a/apps/web/AGENTS.md +++ b/apps/web/AGENTS.md @@ -296,5 +296,5 @@ plugin leaks a stale registration. ## Testing notes - `vitest.config.ts` pins `process.env.NODE_ENV = "test"`, and that line is load-bearing. React exports `act()` only from its development build, so under `NODE_ENV=production` — which the runtime image sets (`Dockerfile`) and every container/agent shell inherits, while CI's image does not — `@testing-library/react` falls back to `react-dom/test-utils` and **every** `render()`/`renderHook()` throws `TypeError: React.act is not a function` before any assertion, with CI still green. `vitest-environment.test.tsx` and the `vitest.setup.ts` preflight fail by name if the pin goes, and the `Run tests` step in `.github/workflows/frontend-tests.yml` exports `NODE_ENV=production` on purpose so those guards fire in CI too rather than only in a container. -- **E2E waits name the cause, they do not budget for the effect.** The suite's dominant flake is `await expect(x).toBeEnabled({ timeout: 30_000 })` — an assertion on a UI shadow of a backend event whose hand-picked budget expires under load. `e2e/helpers/causal-waits.ts` has one primitive per transport (`waitForHttp`, `watchWs().waitForEvent`, `watchWs().waitForResponse`): arm it before the action, await it after, then assert the UI with its **default** timeout. "The backend reached state X" needs no primitive — `expect.poll` against `e2e/helpers/api-client.ts` already reads the backend, not the DOM. `watchWs(page)` only sees sockets opened after it is called, so it goes before the first `page.goto()`. Confirm a causal chain by probing a live run with a throwaway `page.on("response")` logger; reading the components predicts it wrong often enough to matter. The only sanctioned wall-clock wait is `dwell(page, ms, category, reason)`, or `dwell(ms, category, reason)` where no `Page` exists (fixtures, api-client retries) — `category` is the closed `DwellCategory` union (`negative-assertion` is permanent, `unverified` is debt to drive to zero), `reason` says why no event exists, no options; a delay inside a `page.route()` handler is `injectLatency(ms, reason)` instead, since the delay is the stimulus rather than a wait; raw `page.waitForTimeout` and promise sleeps are not sanctioned. Guide and worked examples: `e2e/README.md`. +- **E2E waits name the cause, they do not budget for the effect.** The suite's dominant flake is `await expect(x).toBeEnabled({ timeout: 30_000 })` — an assertion on a UI shadow of a backend event whose hand-picked budget expires under load. `e2e/helpers/causal-waits.ts` has one primitive per transport (`waitForHttp`, `watchWs().waitForEvent`, `watchWs().waitForResponse`): arm it before the action, await it after, then assert the UI with its **default** timeout. "The backend reached state X" needs no primitive — `expect.poll` against `e2e/helpers/api-client.ts` already reads the backend, not the DOM. `watchWs(page)` only sees sockets opened after it is called, so it goes before the first `page.goto()`. Confirm a causal chain by probing a live run with a throwaway `page.on("response")` logger; reading the components predicts it wrong often enough to matter. The only sanctioned wall-clock wait is `dwell(page, ms, category, reason)`, or `dwell(ms, category, reason)` where no `Page` exists (fixtures, api-client retries) — `category` is the closed `DwellCategory` union (`negative-assertion` is permanent, `unverified` is debt to drive to zero), `reason` says why no event exists, no options; a delay inside a `page.route()` handler is `injectLatency(ms, reason)` instead, since the delay is the stimulus rather than a wait; raw `page.waitForTimeout` and promise sleeps are not sanctioned. Guide and worked examples: `e2e/README.md`. **The sleep ban is enforced in two layers**, the same shape as the i18n guards, and the conversion is complete so both cover the whole tree. `eslint-rules/no-unsanctioned-sleep.mjs` is an AST rule (`e2e/` has ~700 `test.setTimeout()` calls, Playwright's per-test timeout setter, which a regex cannot separate from a sleep — nor a sleep from a `Promise.race` guard); it is an **error across all of `e2e/`** via `e2eSleepGuardFiles`, and it also rejects a `dwell`/`injectLatency` not imported from `e2e/helpers/causal-waits`, since a failed import otherwise lints clean and throws on exactly the loaded shard the wait existed to survive. `pnpm run e2e:sleep-ratchet` (CI + pre-commit) is the second layer and judges the **change, not the file**. The guard **only ever widens** — never narrow it to make a build pass; `e2e-sleep-wiring.test.ts` asserts coverage via ESLint's own config resolution and fails if the CI step or hook disappears. Details: `e2e/README.md` ("How this is enforced"). - jsdom secure cookies need cookie-setter interception; Radix Tooltip tests use keyboard focus, while Playwright covers pointer hover with `locator.hover()`. Scope terminal selectors to the active panel/container; mobile and dockview may mount multiple instances, so shared helpers must not use global selectors. diff --git a/apps/web/e2e/README.md b/apps/web/e2e/README.md index 79aa9fbf03..67eaea7070 100644 --- a/apps/web/e2e/README.md +++ b/apps/web/e2e/README.md @@ -455,11 +455,25 @@ silently fails to apply leaves a call site that lints clean and throws `dwell is not defined` at runtime, inside whichever retry path only executes on a loaded shard. That has already happened once. -Two checks, deliberately scoped differently, because ~126 raw sleeps predate -them and `pnpm lint` is `eslint --max-warnings 0` — a repo-wide rule at _any_ -severity would break every unrelated PR until the conversion finishes. +Two checks, scoped differently. They were scoped differently out of necessity — +~153 raw sleeps predated them, and `pnpm lint` is `eslint --max-warnings 0`, so a +repo-wide rule at _any_ severity would have broken every unrelated PR until the +conversion finished. The conversion has finished, so both now cover the tree. -**1. The new-code ratchet** (`scripts/check-new-e2e-sleeps.mjs`) runs in CI and +**1. The eslint rule** (`eslint-rules/no-unsanctioned-sleep.mjs`) is an **error** +across all of `e2e/` via `e2eSleepGuardFiles`, so `pnpm lint` fails on a new +sleep. It began as an allowlist of seven directories measured at zero and +**graduated** once `pnpm run lint:e2e-sleeps e2e` reported zero across the tree. + +The guard **only ever widens**. **Never narrow it to make a build pass**: a +narrowing means a sleep was added and the guard deleted instead of the sleep. +`scripts/lib/e2e-sleep-wiring.test.ts` asserts coverage through ESLint's own +config resolution rather than by grepping the config text, so it fails on a +narrowing however the globs are rewritten — and it fails if the CI step or the +pre-commit hook disappears, because a gate nobody calls reports a clean tree +forever. + +**2. The new-code ratchet** (`scripts/check-new-e2e-sleeps.mjs`) runs in CI and pre-commit and judges the **change**, not the file: - a file the change **added** must be clean outright; @@ -467,17 +481,11 @@ pre-commit and judges the **change**, not the file: So editing a line next to somebody else's `waitForTimeout` never becomes your problem, and there is no migration treadmill. Same model as -`golangci-lint --new-from-rev` and the i18n ratchet. - -**2. The eslint rule** (`eslint-rules/no-unsanctioned-sleep.mjs`) is an -**error**, but only on `e2eSleepGuardFiles` — directories the conversion has -already driven to zero. Append a directory there in the same PR that clears it. -**Never remove an entry to make a build pass**: a removal means a sleep was added -to a clean directory. - -When the list would cover all of `e2e/`, replace it with `e2e/**/*.ts`. That is -the graduation, and from then on the ratchet is a second line of defence rather -than the only one. +`golangci-lint --new-from-rev` and the i18n ratchet. Now that the rule covers the +tree this is the second line of defence rather than the only one, and it is still +worth keeping: it catches a sleep added under any path a future `ignores` entry +carves out, and it attributes a finding to the line the change added rather than +to the file it happens to live in. Two commands: diff --git a/apps/web/eslint-rules/no-unsanctioned-sleep.mjs b/apps/web/eslint-rules/no-unsanctioned-sleep.mjs index 2a28c51331..bdeb592f3b 100644 --- a/apps/web/eslint-rules/no-unsanctioned-sleep.mjs +++ b/apps/web/eslint-rules/no-unsanctioned-sleep.mjs @@ -61,15 +61,17 @@ * * ## Severity * - * An **error**, but only on {@link e2eSleepGuardFiles} — the directories already - * converted to zero. Not a warning either: `pnpm lint` is - * `eslint --max-warnings 0`, so a repo-wide registration at *any* severity would - * fail the build on the ~126 sleeps that predate this rule and block every - * unrelated PR, which is the mistake the first i18n attempt made. + * An **error** across {@link e2eSleepGuardFiles}, which now covers all of + * `e2e/`. It began as a narrow allowlist of directories already converted to + * zero — `pnpm lint` is `eslint --max-warnings 0`, so a repo-wide registration + * at *any* severity would have failed the build on the sleeps that predated the + * rule and blocked every unrelated PR, the mistake the first i18n attempt made. + * The conversion has finished, so that cost is gone and the list has graduated. * - * Everywhere else the gate is `scripts/check-new-e2e-sleeps.mjs`, which judges - * only the lines a change added. Once the conversion reaches zero the guard list - * collapses to `e2e/**\/*.{ts,tsx}` and the ratchet becomes belt-and-braces. + * `scripts/check-new-e2e-sleeps.mjs` still judges only the lines a change added. + * It is the belt to this rule's braces: it covers anything a future `ignores` + * entry carves out, and it attributes a finding to the change rather than to the + * file it lives in. */ /** @@ -96,35 +98,47 @@ export const SLEEP_EXEMPT_FILES = [ /** * Paths where this rule is an **error** in `eslint.config.mjs`. * - * Same opt-in shape as `i18nGuardFiles`, and for the same reason: `pnpm lint` is - * `eslint --max-warnings 0`, so a repo-wide registration at any severity fails - * the build on the ~126 sleeps that predate the rule and blocks every unrelated - * PR. Listing only directories already at zero means this can only ever tighten. + * **Graduated.** This started as an opt-in allowlist of the handful of + * directories the causal-waits conversion had already driven to zero, for the + * same reason `i18nGuardFiles` is one: `pnpm lint` is `eslint --max-warnings 0`, + * so a repo-wide registration at any severity would have failed the build on the + * ~153 sleeps that predated the rule and blocked every unrelated PR — the + * mistake the first i18n attempt made. * - * The seed was measured, not guessed — `pnpm run e2e:waits` reports remaining - * raw sleeps per file, and every directory below had none at the time it was - * added. Adding a path is the only safe edit: **never remove one to make a build - * pass.** A removal means somebody added a sleep to a directory that had none, - * which is exactly the regression this exists to catch. + * The conversion has since finished. `pnpm run lint:e2e-sleeps e2e` reports zero + * findings across the whole tree, so the allowlist has collapsed to all of + * `e2e/`, which is what the list was always going to become. The cost that + * justified the narrow seed no longer exists: a repo-wide error now fails only + * a PR that *adds* a sleep, which is the entire point. * - * The conversion effort is working through the directories that are *not* here - * (`tests/{auth,chat,layout,office,session,settings,ssh,system,task,terminal, - * workflow,…}`); each should be appended as it reaches zero. When the list would - * cover all of `e2e/`, replace it with a plain `["e2e/**\/*.{ts,tsx}"]` — that is the - * graduation, and at that point the diff-scanning ratchet becomes a second line - * of defence rather than the only one. + * With the rule covering the tree, `scripts/check-new-e2e-sleeps.mjs` becomes a + * second line of defence rather than the only one. It is still worth keeping: + * it is what catches a sleep added to a path a future `ignores` entry carves + * out, and it reports against the fork point rather than the file, so its + * message names the line the change added. + * + * **Never narrow this back to make a build pass.** A narrowing means somebody + * added a sleep and deleted the guard rather than the sleep — exactly the + * regression this exists to catch. `e2e-sleep-wiring.test.ts` asserts coverage + * behaviourally, through ESLint's own config resolution, so it fails on a + * narrowing however the glob is rewritten. */ -export const e2eSleepGuardFiles = [ - // Shard planning, flake and timing reports. Tooling rather than specs, and not - // in any conversion chunk's path. - "e2e/scripts/**/*.{ts,tsx}", - // Test directories measured at zero raw sleeps. - "e2e/tests/cli-mode/**/*.{ts,tsx}", - "e2e/tests/docker/**/*.{ts,tsx}", - "e2e/tests/github/**/*.{ts,tsx}", - "e2e/tests/i18n/**/*.{ts,tsx}", - "e2e/tests/kanban/**/*.{ts,tsx}", - "e2e/tests/preview/**/*.{ts,tsx}", +export const e2eSleepGuardFiles = ["e2e/**/*.{ts,tsx}"]; + +/** + * Directories that were measured at zero and listed individually before the + * graduation above. Retained as the wiring test's regression corpus: each must + * still resolve to an error, so collapsing the list cannot silently drop one. + */ +export const e2eSleepSeededGuardDirs = [ + // Shard planning, flake and timing reports. Tooling rather than specs. + "e2e/scripts", + "e2e/tests/cli-mode", + "e2e/tests/docker", + "e2e/tests/github", + "e2e/tests/i18n", + "e2e/tests/kanban", + "e2e/tests/preview", ]; /** diff --git a/apps/web/eslint.config.mjs b/apps/web/eslint.config.mjs index f2cc3128b3..366fc77b35 100644 --- a/apps/web/eslint.config.mjs +++ b/apps/web/eslint.config.mjs @@ -98,14 +98,16 @@ const eslintConfig = defineConfig([ "sonarjs/no-duplicate-string": "off", }, }, - // Unconditional wall-clock sleeps in E2E specs. An ERROR, but only on - // `e2eSleepGuardFiles` — the directories the causal-waits conversion has - // already driven to zero. `pnpm lint` is `eslint --max-warnings 0`, so a - // repo-wide registration at ANY severity (warning included) would fail the - // build on the ~126 sleeps that predate the rule. Everything not listed is - // covered by `scripts/check-new-e2e-sleeps.mjs`, which judges only the lines a - // change added, so new sleeps are caught everywhere without a treadmill. - // Append a directory here when it reaches zero; see the list's own comment. + // Unconditional wall-clock sleeps in E2E specs. An ERROR across all of `e2e/`. + // This was a narrow allowlist while the causal-waits conversion was running — + // `pnpm lint` is `eslint --max-warnings 0`, so a repo-wide registration at ANY + // severity (warning included) would have failed the build on the sleeps that + // predated the rule and blocked every unrelated PR. The conversion reached + // zero, so `e2eSleepGuardFiles` has graduated to the whole tree and this now + // fails only a PR that ADDS a sleep. + // `scripts/check-new-e2e-sleeps.mjs` remains as the second line of defence: it + // judges only the lines a change added, and covers anything `ignores` carves + // out. Never narrow the guard to make a build pass; see the list's own comment. { files: e2eSleepGuardFiles, // `causal-waits.ts` implements both banned forms — see SLEEP_EXEMPT_FILES. diff --git a/apps/web/eslint.e2e-sleeps.config.mjs b/apps/web/eslint.e2e-sleeps.config.mjs index 8c997f1bb5..c7b21eab75 100644 --- a/apps/web/eslint.e2e-sleeps.config.mjs +++ b/apps/web/eslint.e2e-sleeps.config.mjs @@ -6,20 +6,18 @@ import { e2eSleepPlugin, SLEEP_EXEMPT_FILES } from "./eslint-rules/no-unsanction /** * Standalone config for the sleep ratchet and its preview command. * - * The main config (eslint.config.mjs) errors only on `e2eSleepGuardFiles` — the - * directories the causal-waits conversion has already driven to zero. That is - * deliberate: `pnpm run lint` is `eslint --max-warnings 0`, so registering this - * rule repo-wide at *any* severity, warning included, would fail the build on - * the ~130 sleeps that predate it and break every unrelated PR until the - * conversion finishes. - * - * This config applies the same rule to whatever paths it is given, regardless of - * that allowlist. Two consumers: + * This isolates the sleep rule from every other rule in the repo, so a finding + * here is unambiguously a sleep. Two consumers: * * - `scripts/check-new-e2e-sleeps.mjs`, the ratchet, which filters findings * down to the lines a change added. - * - `pnpm run lint:e2e-sleeps `, the work-list generator for whoever is - * converting a directory next. + * - `pnpm run lint:e2e-sleeps `, the preview — now mainly a way to + * confirm a path is at zero before widening a guard, since the main config + * covers all of `e2e/` after the graduation. + * + * It stays separate from `eslint.config.mjs` rather than being folded into it + * because the ratchet needs to lint paths the main config's `ignores` may carve + * out, and needs the result to contain nothing it has to filter by rule id. * * The rule and the exemption list are imported, not restated, so the preview, * the gate and the editor cannot disagree about what counts as a sleep. @@ -34,7 +32,15 @@ export default defineConfig([ files: ["**/*.ts", "**/*.tsx"], ignores: [...SLEEP_EXEMPT_FILES, "node_modules/**", "dist/**"], languageOptions: { parser: tseslint.parser }, - plugins: { "e2e-sleeps": e2eSleepPlugin }, + // `@typescript-eslint` is registered for its RULE DEFINITIONS only — none are + // switched on below. Specs carry inline `eslint-disable` comments naming its + // rules, and ESLint 9 reports a disable directive for an unknown rule as an + // error ("Definition for rule … was not found"). Without the plugin the + // preview exited 1 on nine files that contain no sleep at all, which trains + // whoever runs it to read a red exit as noise — the habit that lets a real + // finding through. The ratchet itself filtered those out by rule id, so this + // never affected the gate's verdict. + plugins: { "e2e-sleeps": e2eSleepPlugin, "@typescript-eslint": tseslint.plugin }, rules: { "e2e-sleeps/no-unsanctioned-sleep": "error" }, }, ]); diff --git a/apps/web/scripts/lib/e2e-sleep-wiring.test.ts b/apps/web/scripts/lib/e2e-sleep-wiring.test.ts index 69712ad43a..9de7586125 100644 --- a/apps/web/scripts/lib/e2e-sleep-wiring.test.ts +++ b/apps/web/scripts/lib/e2e-sleep-wiring.test.ts @@ -19,13 +19,45 @@ import fs from "node:fs"; import path from "node:path"; +import { ESLint } from "eslint"; import { describe, expect, it } from "vitest"; +import { + e2eSleepSeededGuardDirs, + SLEEP_EXEMPT_FILES, + SLEEP_RULE_ID, +} from "../../eslint-rules/no-unsanctioned-sleep.mjs"; + const WEB_DIR = path.resolve(import.meta.dirname, "../.."); const REPO_ROOT = path.resolve(WEB_DIR, "../.."); const RATCHET_SCRIPT = "check-new-e2e-sleeps.mjs"; +/** ESLint reports a resolved severity numerically; 2 is `"error"`. */ +const RULE_ERROR = 2; + +/** + * One ESLint instance for every config-resolution test below. + * + * Resolving the first file loads the whole flat config and its plugins, which + * costs several seconds; later calls hit the instance's cache. Building one + * instance per test pushed the first past vitest's 5s default and failed it on + * timeout — a red that says nothing about the guard. Hence a shared instance and + * an explicit timeout on the tests that use it. + */ +const configFor = (() => { + let eslint: ESLint | null = null; + return (file: string) => (eslint ??= new ESLint({ cwd: WEB_DIR })).calculateConfigForFile(file); +})(); + +const SLEEP_GUARD_TIMEOUT = 60_000; + +/** The resolved severity of the sleep rule for `file`, or `undefined` if unset. */ +async function sleepSeverity(file: string) { + const config = await configFor(file); + return config.rules?.[SLEEP_RULE_ID]; +} + const read = (rel: string) => fs.readFileSync(path.join(REPO_ROOT, rel), "utf8"); describe("sleep ratchet wiring", () => { @@ -120,22 +152,67 @@ describe("sleep ratchet wiring", () => { }); /** - * The guard list may only grow. A directory listed here was measured at zero, - * so removing an entry means somebody added a sleep to a clean directory and - * deleted the guard rather than the sleep. + * The guard may only grow. + * + * This asked ESLint's own config resolution rather than grepping the config + * text for glob literals. The string form only held while the guard was a + * list of directory globs: the graduation to a single `e2e/**` entry STRICTLY + * WIDENED coverage, yet a containment assertion reads that as seven deletions + * and fails. A test that fails on a tightening is one somebody edits to make + * the build pass, which is precisely the pressure this is meant to resist. + * + * `calculateConfigForFile` answers the question the guard actually exists to + * answer — "is a sleep in this file an error?" — so it survives any rewrite of + * the globs and still fails on a real narrowing. */ - it("keeps every seeded guard directory", () => { - const config = read("apps/web/eslint-rules/no-unsanctioned-sleep.mjs"); - for (const dir of [ - "e2e/scripts/**/*.{ts,tsx}", - "e2e/tests/cli-mode/**/*.{ts,tsx}", - "e2e/tests/docker/**/*.{ts,tsx}", - "e2e/tests/github/**/*.{ts,tsx}", - "e2e/tests/i18n/**/*.{ts,tsx}", - "e2e/tests/kanban/**/*.{ts,tsx}", - "e2e/tests/preview/**/*.{ts,tsx}", - ]) { - expect(config).toContain(`"${dir}"`); - } - }); + it( + "makes a sleep an error in every seeded guard directory", + async () => { + for (const dir of e2eSleepSeededGuardDirs) { + expect(await sleepSeverity(`${dir}/probe.spec.ts`), `${dir} lost its sleep guard`).toEqual([ + RULE_ERROR, + ]); + } + }, + SLEEP_GUARD_TIMEOUT, + ); + + /** + * The graduation itself: the directories that were NOT on the original seed + * list are covered now. Without this, narrowing the guard back to the old + * seven-entry allowlist would pass every other test in this file. + */ + it( + "covers the directories the conversion added after the seed", + async () => { + for (const dir of [ + "e2e/tests/task", + "e2e/tests/session", + "e2e/tests/settings", + "e2e/helpers", + "e2e/fixtures", + "e2e/pages", + ]) { + expect(await sleepSeverity(`${dir}/probe.spec.ts`), `${dir} is not guarded`).toEqual([ + RULE_ERROR, + ]); + } + }, + SLEEP_GUARD_TIMEOUT, + ); + + /** + * The exemption still applies. `causal-waits.ts` implements both banned forms, + * so a guard that covered it would make the sanctioned helper the one file + * that can never satisfy the rule it exists to serve. + */ + it( + "still exempts the causal-waits helper and its own test", + async () => { + for (const file of SLEEP_EXEMPT_FILES) { + expect(await sleepSeverity(file), `${file} should be exempt`).toBeUndefined(); + } + }, + SLEEP_GUARD_TIMEOUT, + ); });