diff --git a/.github/workflows/frontend-tests.yml b/.github/workflows/frontend-tests.yml index b0d6d9de2e..828daac897 100644 --- a/.github/workflows/frontend-tests.yml +++ b/.github/workflows/frontend-tests.yml @@ -209,6 +209,36 @@ jobs: node scripts/run-i18n-ratchet.mjs --base "${BASE_SHA}" + # New-code ratchet for E2E waits. `page.waitForTimeout(500)` and + # hand-rolled promise sleeps are the suite's most reliable source of flake: + # a hand-picked number that was long enough on the machine it was written + # on. The sanctioned forms (`dwell`, `injectLatency` in + # e2e/helpers/causal-waits.ts) take a category and a reason. + # + # The eslint rule is an error only on directories already converted to + # zero, so this judges the CHANGE instead of the FILE for everywhere else — + # added files whole, modified files only on touched lines. Same base + # handling as the i18n step above, and the same reason for it: DO NOT pass + # --base on pull_request. + - name: Run E2E sleep ratchet + shell: bash + working-directory: apps/web + run: | + set -euo pipefail + + if [[ "${{ github.event_name }}" == "pull_request" ]]; then + node scripts/check-new-e2e-sleeps.mjs + exit 0 + fi + + BASE_SHA="${{ github.event.before }}" + if [[ -z "${BASE_SHA}" || "${BASE_SHA}" == "0000000000000000000000000000000000000000" ]]; then + echo "No usable base SHA; skipping the E2E sleep ratchet." + exit 0 + fi + + node scripts/check-new-e2e-sleeps.mjs --base "${BASE_SHA}" + # Deliberately run under the variable that broke the suite, so a # regression fails here instead of only in a container. React exports # `act()` from its development build alone, `vitest.config.ts` pins diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 75407e4ffb..5eefbcc160 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -69,6 +69,19 @@ repos: files: ^apps/web/(components|app|hooks|lib|src)/.*\.tsx?$|^apps/web/eslint\.i18n\.options\.mjs$ pass_filenames: false + # New E2E waits must name their cause, or name a category and a reason via + # dwell()/injectLatency(). Raw page.waitForTimeout() and hand-rolled + # promise sleeps are the suite's most reliable source of flake. Same + # judge-the-change contract as the i18n hook above: added files whole, + # modified files only on touched lines, so the ~126 sleeps that predate it + # never block an unrelated commit. + - id: e2e-sleep-ratchet + name: E2E sleep guard (new code only) + entry: bash -c 'unset GIT_DIR GIT_WORK_TREE; cd apps/web && node scripts/check-new-e2e-sleeps.mjs' + language: system + files: ^apps/web/e2e/.*\.tsx?$|^apps/web/eslint-rules/no-unsanctioned-sleep\.mjs$ + pass_filenames: false + # Public copy hooks - repo: local hooks: diff --git a/apps/web/e2e/README.md b/apps/web/e2e/README.md index 4e9494e37b..79aa9fbf03 100644 --- a/apps/web/e2e/README.md +++ b/apps/web/e2e/README.md @@ -444,6 +444,153 @@ A ratchet banning raw sleeps needs to allow **both** tokens, `dwell(` and instead — a proximity heuristic misfires, and a wrong exemption silently unguards real sleeps. +### How this is enforced + +**The sanctioned forms are only sanctioned when they are actually imported.** +`dwell` and `injectLatency` must resolve to a binding from +`e2e/helpers/causal-waits`; a call to an unimported (or differently imported) +identifier of that name is reported like a raw sleep. It is not pedantry: the +repo turns `no-undef` off, nothing typechecks `e2e/`, and an import that +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. + +**1. 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; +- a file the change **modified** is judged only on the lines it touched. + +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. + +Two commands: + +```bash +pnpm run e2e:waits # debt report: raw sleeps, dwell by category +pnpm run lint:e2e-sleeps # preview the rule on a path not yet guarded +``` + +`e2e:waits` reports three numbers that must not be added together. Raw sleeps are +what the conversion drives to zero. **Dwell debt excludes `negative-assertion`, +which is permanent** — a test asserting something never happens has no event to +wait for, and deleting its wait removes the window the regression needs in order +to appear. `injectLatency` is counted apart from both, so deliberate fixture +configuration cannot inflate a number that is supposed to be shrinking. Inside +the debt, `unverified` is the sub-total to attack first. + +**What the rule does not flag, and why.** It is an AST rule rather than a grep +because `e2e/` holds ~700 `test.setTimeout(60_000)` calls — Playwright's per-test +timeout setter, unrelated to sleeping. It also leaves alone two shapes that look +almost identical to a sleep: + +- a **race guard**, where the timer is one of several resolution paths + (`socket.onopen = …; setTimeout(() => resolve(false), 3000)`); +- a **cancellable timer**, where the handle is kept for a later `clearTimeout`. + A sleep never keeps the handle. + +The same reasoning leaves `requestAnimationFrame(() => setTimeout(resolve, 0))` +alone: that promise resolves on the **frame**, and the `setTimeout(…, 0)` only +hops out of the rAF callback so a measurement reflects painted geometry. It is +not a duration wait. This falls out of the rule's shape rather than from a +carve-out — a blanket "any `setTimeout(…, 0)` is fine" exemption would have been +a loophole. + +## Verifying a converted wait + +Replacing a sleep with a causal wait is easy to do wrongly in a way that still +passes. These four checks are ordered by how often they actually catch +something. + +### 1. Verify against observed traffic, never a name that looks right + +Reading the components and picking the action that sounds correct is the single +most common way a conversion goes wrong. Attach a throwaway logger, run the spec +once, and read what actually arrived: + +```ts +page.on("response", (r) => console.log(r.request().method(), new URL(r.url()).pathname)); +page.on("websocket", (ws) => + ws.on("framereceived", (f) => console.log(String(f.payload).slice(0, 200))), +); +``` + +Three conversions in this suite were wrong despite careful code-reading: a spec +named after `kanban.update` whose path only ever delivers `task.updated`; a +branch list fetched on the workspace-scoped route rather than the +repository-scoped one the client's shape implies; and a `task.plan.get` that +never fires on first panel open at all. Each looked right in the source. + +### 2. Rule out the silent burn + +A wait that resolves on timeout instead of throwing converts a hard failure into +a slow pass — the test still goes green, having asserted nothing about timing. +Every armed wait must be awaited, and must reject rather than resolve when it +expires (the primitives in `helpers/causal-waits.ts` do). + +Then compare **runtime against the budget**. A spec carrying two 15s waits that +finishes in 6.3s cannot be burning either of them; one that finishes in 30.1s is +burning both. One fixed conversion here went from a 15s burn to 6.8s, and that +delta is what exposed it — not any assertion. + +### 3. Run it enough times to mean something + +```sh +pnpm run build:e2e # specs run against the prebuilt dist +pnpm e2e:raw tests//.spec.ts --repeat-each=10 +``` + +Read the **spread**, not the pass count. `1.4s-11.4s` across ten runs with zero +failures is evidence the wait absorbs load; a single green 1.4s run is evidence +of nothing. A component edit is invisible until `build:e2e` re-runs, so a spec +that "passes after the fix" without a rebuild has told you nothing. + +### 4. Mutate what you rely on + +Break the thing the test depends on, confirm the intended test fails, restore, +confirm the tree is byte-identical. Assert the expected number of tests was +**collected and executed** — a mutant that fails to compile makes the runner +report zero collected, which at a glance is indistinguishable from a clean run. + +### Two traps + +**Wrong Playwright project.** `mobile-*.spec.ts` exists only under +`mobile-chrome`, `tests/auth/**` under `auth`, `office-routing-*` under +`routing`, and `tests/{docker,ssh}/**` under `containers`. Selecting one under +`chromium` matches nothing, prints `No tests found`, and **exits non-zero** — so +the failure is real. What hides it is the idiom: `playwright test … | tail` +reports `tail`'s exit status, not Playwright's. **Never pipe a gate.** Derive the +expected test count before the run and compare it to Playwright's first line; +checking afterwards is a habit, deriving first is a check. + +**Nothing typechecks `e2e/**`.** `apps/web/tsconfig.json`excludes it, there is +no`e2e/tsconfig.json`, and eslint runs no type-aware rules here — a file +containing `const x: number = "string"`passes both`pnpm run typecheck`and`pnpm lint`. Anything load-bearing needs a runtime check or a test. `dwell` +validates its category at runtime for exactly this reason. + +### Why this matters + +**A sleep is where a test goes to stop asserting.** Converting them has already +found two specs that had quietly stopped testing anything. The clearest was a +test pressing a keybinding that is `UNBOUND_SHORTCUT` by default: the keypress +was a no-op, and the 250ms sleep after it made the test look like it was waiting +for a result it could never receive. + ## Adding a new spec 1. Pick a directory under `tests/` (or create one for a new feature). diff --git a/apps/web/eslint-rules/no-unsanctioned-sleep.mjs b/apps/web/eslint-rules/no-unsanctioned-sleep.mjs new file mode 100644 index 0000000000..2a28c51331 --- /dev/null +++ b/apps/web/eslint-rules/no-unsanctioned-sleep.mjs @@ -0,0 +1,333 @@ +/** + * `no-unsanctioned-sleep` — an unconditional wall-clock sleep in `e2e/` must go + * through `dwell()` or `injectLatency()` from `e2e/helpers/causal-waits.ts`. + * + * Those two helpers demand a category and a reason. A raw sleep demands nothing, + * which is why it is indistinguishable — at a glance and to a grep — from + * somebody who could not find the right event and reached for a number instead. + * The whole point of the causal-waits work is that the sanctioned forms are + * greppable and self-documenting; that only holds if the unsanctioned ones + * cannot be added silently. + * + * ## Why this is an AST rule and not a grep + * + * `e2e/` contains **701** `test.setTimeout(60_000)` calls — Playwright's + * per-test timeout setter, which has nothing to do with sleeping and must never + * be flagged — against ~38 real promise sleeps. Any `setTimeout` pattern that a + * regex can express either drowns in those 701 or misses the multi-line sleeps. + * It also cannot separate a sleep from a `Promise.race` timeout guard, which is + * correct code that looks almost identical. The AST separates all three exactly. + * + * ## What is reported + * + * 1. **`waitForTimeout`**, in any receiver position. Playwright exposes it on + * `Page`, `Frame` and `Locator.page()`, so the receiver is not fixed; the + * method name is. `dwell(page, ms, category, reason)` is the legal form. + * + * 2. **The promise-sleep idiom** — `new Promise((r) => setTimeout(r, ms))` — in + * the narrow shape where the timer is the promise's *only* resolution path. + * `dwell(ms, category, reason)` is the legal form where no `Page` exists. + * + * ## What is deliberately NOT reported, and why the narrowing is principled + * + * The executor must contain **exactly one statement**, that statement's value + * must be **discarded**, and its first argument must **resolve the promise**. + * Each clause excludes a real, correct pattern in this suite: + * + * - *More than one statement* excludes a race guard. `auth-lifecycle.spec.ts` + * registers `socket.onopen/onclose/onerror` and then a `setTimeout` fallback: + * the timer is one of four resolution paths, not a sleep. + * - *Value discarded* excludes a cancellable timer. `concurrency.spec.ts` has a + * lone `timer = setTimeout(() => resolve({kind:"timeout"}), ms)` inside its + * executor — one statement, but it keeps the handle because something else + * will `clearTimeout` it. **A sleep never keeps the handle**; keeping it is + * precisely the intent to cancel, which means another path resolves first. + * - *First argument resolves the promise* keeps the diagnostic honest: it makes + * the finding "this is a sleep" rather than merely "this has the right shape". + * "Resolves the promise" includes bound references — `resolve.bind(null)` is + * still `resolve`, and recursively so, since a form that laundered a sleep + * past the rule would be worth more to a determined author than to an honest + * one. + * + * The same three clauses dissolve the `requestAnimationFrame(() => + * setTimeout(resolve, 0))` frame-yield in `toggle-sidebar-shortcut.spec.ts` + * without a carve-out: the executor's one statement is a `requestAnimationFrame` + * call, not a `setTimeout` one. That is not a convenient exception — the promise + * there resolves on the **frame**, and the `setTimeout(…, 0)` only hops out of + * the rAF callback so the measurement reflects painted geometry. It is not a + * duration wait, so a rule about duration waits should not see it. A blanket + * "any `setTimeout(…, 0)` is fine" carve-out would have been a loophole; this + * needs no carve-out at all. + * + * ## 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. + * + * 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. + */ + +/** + * Files exempt from the rule, repo-relative from `apps/web`. + * + * `causal-waits.ts` implements `dwell` and `injectLatency` and therefore + * contains both banned forms by construction — `dwell` delegates to + * `page.waitForTimeout(ms)` when a page exists and to a promise sleep when one + * does not. Without this the sanctioned helper is the one file that can never + * satisfy the rule it exists to serve. + * + * Its own `*.test.ts` is exempt for the same reason: tests for a sleep helper + * necessarily construct sleeps. + * + * Exported so `eslint.config.mjs` and `eslint.e2e-sleeps.config.mjs` cannot + * drift — an exemption that applies in the editor but not in the gate (or the + * reverse) is a difference nobody would notice until it fired. + */ +export const SLEEP_EXEMPT_FILES = [ + "e2e/helpers/causal-waits.ts", + "e2e/helpers/causal-waits.test.ts", +]; + +/** + * 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. + * + * 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 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. + */ +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}", +]; + +/** + * The sanctioned waits. Calling one is only sanctioned if the identifier really + * IS that helper — see {@link resolveWaitBinding}. + */ +const SANCTIONED_WAITS = new Set(["dwell", "injectLatency"]); + +/** `…/causal-waits`, with or without an extension. */ +const CAUSAL_WAITS_MODULE = /(^|\/)causal-waits(\.[jt]s)?$/; + +/** The nearest binding for `name`, walking out through enclosing scopes. */ +function lookupVariable(scope, name) { + for (let current = scope; current; current = current.upper) { + const found = current.variables.find((variable) => variable.name === name); + if (found) return found; + } + return null; +} + +/** + * Whether `variable` is really the helper from `causal-waits`. + * + * An unresolved identifier is a `ReferenceError` waiting for the machine slow + * enough to reach it, and a `dwell` imported from somewhere else is not the + * helper this rule promotes. Both answer `false`. + */ +function isCausalWaitsBinding(variable) { + if (!variable) return false; + return variable.defs.some( + (def) => + def.type === "ImportBinding" && CAUSAL_WAITS_MODULE.test(String(def.parent.source.value)), + ); +} + +const WAIT_FOR_TIMEOUT = "waitForTimeout"; +const SET_TIMEOUT = "setTimeout"; + +/** `setTimeout`, `window.setTimeout`, `globalThis.setTimeout`. */ +function isSetTimeoutCallee(node) { + if (node.type === "Identifier") return node.name === SET_TIMEOUT; + if (node.type !== "MemberExpression" || node.computed) return false; + return node.property.type === "Identifier" && node.property.name === SET_TIMEOUT; +} + +/** The method name a call invokes, for `a.b()` and `a["b"]()` alike. */ +function calleeMethodName(node) { + if (node.type === "Identifier") return node.name; + if (node.type !== "MemberExpression") return null; + if (!node.computed) return node.property.type === "Identifier" ? node.property.name : null; + return node.property.type === "Literal" && typeof node.property.value === "string" + ? node.property.value + : null; +} + +/** + * The executor's own statements, or `null` if it is not a function we can read. + * + * An expression-bodied arrow (`(r) => setTimeout(r, 5)`) is normalised to a + * single-entry list so both forms flow through one check. Nested functions are + * NOT unwrapped: that is exactly what keeps the rAF frame-yield out. + */ +function executorStatements(executor) { + if (executor?.type !== "ArrowFunctionExpression" && executor?.type !== "FunctionExpression") { + return null; + } + if (executor.body.type !== "BlockStatement") return [executor.body]; + return executor.body.body.map((statement) => + statement.type === "ExpressionStatement" ? statement.expression : statement, + ); +} + +/** The single expression a function evaluates to, or `null` if it does more. */ +function soleBodyExpression(node) { + if (node.body.type !== "BlockStatement") return node.body; + if (node.body.body.length !== 1) return null; + const [only] = node.body.body; + return only.type === "ExpressionStatement" ? only.expression : null; +} + +/** Whether `node` is `X.bind(...)`, returning `X`; otherwise `null`. */ +function boundTarget(node) { + if (node.type !== "CallExpression") return null; + const { callee } = node; + if (callee.type !== "MemberExpression" || callee.computed) return null; + if (callee.property.type !== "Identifier" || callee.property.name !== "bind") return null; + return callee.object; +} + +/** Whether `node` is a call to `resolve`, or a function whose body only calls it. */ +function resolvesPromise(node, resolveName) { + if (!node) return false; + if (node.type === "Identifier") return node.name === resolveName; + // A bound reference to `resolve` is still `resolve`: + // `setTimeout(resolve.bind(null), ms)` sleeps exactly like `setTimeout(resolve, ms)`. + // Recursive, so `resolve.bind(a).bind(b)` does not launder it either. + const bound = boundTarget(node); + if (bound) return resolvesPromise(bound, resolveName); + if (node.type !== "ArrowFunctionExpression" && node.type !== "FunctionExpression") return false; + + const body = soleBodyExpression(node); + if (body?.type !== "CallExpression" || body.callee.type !== "Identifier") return false; + return body.callee.name === resolveName; +} + +/** + * The `setTimeout` call that makes `node` an unconditional sleep, or `null`. + * + * Every clause here is load-bearing — see the module comment for the real call + * site each one protects. + */ +function promiseSleepCall(node) { + if (node.callee.type !== "Identifier" || node.callee.name !== "Promise") return null; + + const executor = node.arguments[0]; + const resolveParam = executor?.params?.[0]; + if (resolveParam?.type !== "Identifier") return null; + + const statements = executorStatements(executor); + // Exactly one statement: more than one means the timer is a fallback among + // several resolution paths (a race guard), not the wait itself. + if (statements?.length !== 1) return null; + + const [only] = statements; + // A bare call, so the handle is discarded. `timer = setTimeout(...)` arrives + // as an AssignmentExpression and stops here: keeping the handle is the intent + // to cancel, and a sleep is never cancelled. + if (only.type !== "CallExpression" || !isSetTimeoutCallee(only.callee)) return null; + + return resolvesPromise(only.arguments[0], resolveParam.name) ? only : null; +} + +/** @type {import("eslint").Rule.RuleModule} */ +export const noUnsanctionedSleep = { + meta: { + type: "problem", + docs: { + description: + "Require dwell()/injectLatency() from e2e/helpers/causal-waits.ts instead of raw " + + "page.waitForTimeout() or hand-rolled promise sleeps.", + }, + schema: [], + messages: { + waitForTimeout: + "Raw `waitForTimeout()` is not a sanctioned wait. Use `dwell(page, ms, category, reason)` " + + "from `e2e/helpers/causal-waits.ts`, or better, wait on the cause with `waitForHttp` / " + + "`watchWs`. See e2e/README.md.", + unboundWait: + "`{{name}}()` is not imported from `e2e/helpers/causal-waits`, so this is an " + + "undefined identifier that throws at runtime — on exactly the slow, loaded shards a " + + "wait is there to survive. Import it, or use a causal wait instead.", + promiseSleep: + "Hand-rolled promise sleep is not a sanctioned wait. Use `dwell(ms, category, reason)` " + + "from `e2e/helpers/causal-waits.ts`, or `injectLatency(ms, reason)` inside a " + + "`page.route()` handler. See e2e/README.md.", + }, + }, + create(context) { + return { + CallExpression(node) { + // A sanctioned wait is only sanctioned if the identifier resolves to the + // real helper. `eslint.config.mjs` turns `no-undef` OFF repo-wide, and + // nothing typechecks `e2e/`, so an import that silently failed to apply + // leaves `dwell(...)` linting clean and throwing at runtime — inside + // whichever retry path only executes under load. Scope analysis catches + // this without any type information, which is how `no-undef` works. + if (node.callee.type === "Identifier" && SANCTIONED_WAITS.has(node.callee.name)) { + const variable = lookupVariable(context.sourceCode.getScope(node), node.callee.name); + if (!isCausalWaitsBinding(variable)) { + context.report({ + node: node.callee, + messageId: "unboundWait", + data: { name: node.callee.name }, + }); + } + return; + } + if (calleeMethodName(node.callee) !== WAIT_FOR_TIMEOUT) return; + // Report at the METHOD token, not the whole call. A finding's line is + // its node's start line, and the ratchet keeps only findings on lines + // the change added. For a wrapped call — + // + // page + // .waitForTimeout(500); + // + // — the CallExpression starts at `page`, so changing `.click(…)` to + // `.waitForTimeout(…)` would attribute the finding to an UNCHANGED line + // and the ratchet would silently drop it. The method token is on the + // line that actually changed. + const target = node.callee.type === "MemberExpression" ? node.callee.property : node.callee; + context.report({ node: target, messageId: "waitForTimeout" }); + }, + NewExpression(node) { + const sleep = promiseSleepCall(node); + if (sleep) context.report({ node: sleep, messageId: "promiseSleep" }); + }, + }; + }, +}; + +/** Flat-config plugin object, so both configs register the rule the same way. */ +export const e2eSleepPlugin = { + rules: { "no-unsanctioned-sleep": noUnsanctionedSleep }, +}; + +export const SLEEP_RULE_ID = "e2e-sleeps/no-unsanctioned-sleep"; diff --git a/apps/web/eslint-rules/no-unsanctioned-sleep.test.ts b/apps/web/eslint-rules/no-unsanctioned-sleep.test.ts new file mode 100644 index 0000000000..067b059c0b --- /dev/null +++ b/apps/web/eslint-rules/no-unsanctioned-sleep.test.ts @@ -0,0 +1,262 @@ +import { RuleTester } from "eslint"; +import tseslint from "typescript-eslint"; +import { describe, expect, it } from "vitest"; + +import { noUnsanctionedSleep, SLEEP_EXEMPT_FILES } from "./no-unsanctioned-sleep.mjs"; + +/** + * Every `invalid` case below is a shape that must FAIL, and every `valid` case + * is a shape from this suite that must PASS. The valid list is the load-bearing + * half: the rule's whole design problem is that `e2e/` holds 701 + * `test.setTimeout()` calls and a handful of race guards that look almost + * exactly like the ~38 sleeps it must catch. + * + * Each valid case names the real call site it stands in for, so a future + * loosening of the matcher fails here with the reason attached rather than + * silently flagging correct code. + */ +/** + * Hand vitest's `describe`/`it` to RuleTester explicitly. + * + * Without this, RuleTester finds no test globals (this project does not enable + * `globals: true`) and falls back to running every case *synchronously during + * collection*. The cases do execute, but vitest reports `Tests 1 passed (1)` — + * only the explicit `it` below — so "25 cases passed" and "0 cases ran" produce + * an identical, green-looking summary. Wiring the hooks makes each case a real + * test, which is what lets `EXPECTED_CASES` below be an assertion rather than a + * comment. + */ +RuleTester.describe = describe; +RuleTester.it = it; + +const ruleTester = new RuleTester({ + languageOptions: { + parser: tseslint.parser as never, + parserOptions: { ecmaVersion: 2022, sourceType: "module" }, + }, +}); + +ruleTester.run("no-unsanctioned-sleep", noUnsanctionedSleep, { + valid: [ + // --- The 701-site case. A regex ban on `setTimeout` dies here. --- + { code: "test.setTimeout(60_000);" }, + { code: "e2eTest.setTimeout(120_000);" }, + + // --- The sanctioned forms, which are only sanctioned when actually imported. --- + { + code: `import { dwell } from "../../helpers/causal-waits"; + await dwell(page, 300, "library-timer", "Radix open delay publishes nothing");`, + }, + { + code: `import { dwell } from "./causal-waits"; + await dwell(500, "poll-interval", "backend health poll; no page exists yet");`, + }, + { + code: `import { injectLatency } from "../helpers/causal-waits"; + await injectLatency(800, "make the in-flight spinner observable");`, + }, + // Aliased and namespace imports still resolve to the helper. + { + code: `import { dwell as sleep, injectLatency } from "../../helpers/causal-waits"; + await injectLatency(10, "x");`, + }, + + // --- Race guard: several resolution paths, timer is a fallback. --- + // e2e/tests/auth/auth-lifecycle.spec.ts:218 + { + code: `new Promise((resolve) => { + const socket = new WebSocket(url); + socket.onopen = () => resolve(false); + socket.onclose = () => resolve(true); + setTimeout(() => resolve(false), 3000); + });`, + }, + + // --- Cancellable timer: the handle is kept, so something clears it. --- + // e2e/tests/ssh/concurrency.spec.ts:15 — one statement, still not a sleep. + { + code: `new Promise<{ kind: "timeout" }>((resolve) => { + timer = setTimeout(() => resolve({ kind: "timeout" }), timeoutMs); + });`, + }, + { + code: `new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error("timeout")), 30_000); + ws.onmessage = () => { clearTimeout(timer); resolve(); }; + });`, + }, + + // --- Frame yield, not a duration wait. No carve-out: the executor's one + // --- statement is a requestAnimationFrame call, not a setTimeout one. + // e2e/tests/layout/toggle-sidebar-shortcut.spec.ts:123 + { + code: `new Promise((resolve) => { + requestAnimationFrame(() => setTimeout(resolve, 0)); + });`, + }, + + // --- In-page code: the discriminator is what the promise RESOLVES ON, + // --- not which context it runs in. A frame is an event; a duration is not. + // + // Instrumentation, sampling geometry frame by frame. The promise resolves on + // the FRAME — the `setTimeout(…, 0)` only hops out of the rAF callback so the + // measurement reflects painted geometry. Neither `dwell` nor `injectLatency` + // exists in the browser context, so flagging this would trip the gate with no + // legal way to comply. Excluded structurally (the executor's one statement is + // the rAF call), and pinned here so a future tightening of the nesting rule + // cannot silently break it. + // e2e/tests/layout/toggle-sidebar-shortcut.spec.ts:123 + { + code: `await page.evaluate(() => + new Promise((resolve) => { + requestAnimationFrame(() => setTimeout(resolve, 0)); + }), + );`, + }, + + // --- A promise that is not a sleep at all. --- + { code: "new Promise((resolve) => { socket.onopen = () => resolve(); });" }, + { code: "new Promise((resolve) => setTimeout(unrelatedCallback, 100));" }, + // Not the global Promise constructor shape the rule keys on. + { code: "new Deferred((resolve) => setTimeout(resolve, 100));" }, + ], + + invalid: [ + // --- waitForTimeout, in every receiver position Playwright offers. --- + { + code: "await page.waitForTimeout(500);", + errors: [{ messageId: "waitForTimeout" }], + }, + { + code: "await locator.page().waitForTimeout(1000);", + errors: [{ messageId: "waitForTimeout" }], + }, + { + code: 'await page["waitForTimeout"](500);', + errors: [{ messageId: "waitForTimeout" }], + }, + { + code: "await frame.waitForTimeout(HEALTH_POLL_MS);", + errors: [{ messageId: "waitForTimeout" }], + }, + + // --- Promise sleeps, every form the suite actually contains. --- + // The 26-site shape. + { + code: "await new Promise((r) => setTimeout(r, 500));", + errors: [{ messageId: "promiseSleep" }], + }, + // The 6-site shape. + { + code: "await new Promise((resolve) => setTimeout(resolve, 1000));", + errors: [{ messageId: "promiseSleep" }], + }, + // The typed shape. + { + code: "await new Promise((resolve) => setTimeout(resolve, 250));", + errors: [{ messageId: "promiseSleep" }], + }, + // Block-bodied, still exactly one statement. + { + code: "await new Promise((resolve) => { setTimeout(resolve, 300); });", + errors: [{ messageId: "promiseSleep" }], + }, + // A wrapping arrow that only calls resolve is still a sleep. + { + code: "await new Promise((resolve) => setTimeout(() => resolve(), 300));", + errors: [{ messageId: "promiseSleep" }], + }, + // Non-literal delays are sleeps too. + { + code: "await new Promise((r) => setTimeout(r, HEALTH_POLL_MS));", + errors: [{ messageId: "promiseSleep" }], + }, + // `window.`/`globalThis.` prefix must not launder it. + { + code: "await new Promise((r) => window.setTimeout(r, 100));", + errors: [{ messageId: "promiseSleep" }], + }, + // A function expression executor, not just an arrow. + { + code: "await new Promise(function (resolve) { setTimeout(resolve, 100); });", + errors: [{ messageId: "promiseSleep" }], + }, + // Both bans can fire in one file. + { + code: "await page.waitForTimeout(100); await new Promise((r) => setTimeout(r, 100));", + errors: [{ messageId: "waitForTimeout" }, { messageId: "promiseSleep" }], + }, + // A sanctioned wait that is not actually bound to the helper. Chunk 1 hit + // this for real: a patch anchored on an import line whose actual form was + // `{ type Page, expect }`, so the edit missed and the call site was left + // referencing an undefined identifier. `no-undef` is off repo-wide and + // nothing typechecks `e2e/`, so lint passed clean and it would have surfaced + // as `dwell is not defined` inside a retry path that only runs on slow + // shards. The token being allowed is not the same as the helper being there. + { + code: 'await dwell(page, 500, "unverified", "import silently did not apply");', + errors: [{ messageId: "unboundWait" }], + }, + { + code: 'await injectLatency(200, "import silently did not apply");', + errors: [{ messageId: "unboundWait" }], + }, + // Imported, but not from causal-waits: also not the sanctioned helper. + { + code: `import { dwell } from "./local-utils"; + await dwell(500, "unverified", "wrong module");`, + errors: [{ messageId: "unboundWait" }], + }, + // A locally defined look-alike is not the helper either. + { + code: `function dwell(ms) { return ms; } + await dwell(500, "unverified", "shadowed by a local");`, + errors: [{ messageId: "unboundWait" }], + }, + + // A DURATION wait is still a duration wait inside `page.evaluate`. + // `await page.evaluate(() => new Promise((r) => setTimeout(r, 500)))` blocks + // the test for 500ms exactly like `page.waitForTimeout(500)` — the browser + // context is a wrapper, not a different kind of wait. Exempting in-page code + // wholesale would make that one line a laundering wrapper for every sleep in + // the suite. There IS a legal path: hoist it to `dwell(page, ms, …)` and + // evaluate afterwards, or resolve on a frame/event as the rAF case above does. + { + code: "await page.evaluate(() => new Promise((r) => setTimeout(r, 500)));", + errors: [{ messageId: "promiseSleep" }], + }, + // A bound reference to `resolve` is still `resolve`. Flagged so the form + // cannot be used to launder a sleep past the rule (review finding). + { + code: "await new Promise((resolve) => setTimeout(resolve.bind(null), 100));", + errors: [{ messageId: "promiseSleep" }], + }, + { + code: "await new Promise((resolve) => setTimeout(resolve.bind(null).bind(null), 100));", + errors: [{ messageId: "promiseSleep" }], + }, + // The finding must land on the METHOD token, not the receiver. On a + // wrapped call those are different LINES, and the ratchet keeps only + // findings on lines the change added — so reporting the whole call would + // attribute this to line 1 and silently drop it (review finding). + { + code: "await page\n .waitForTimeout(500);", + errors: [{ messageId: "waitForTimeout", line: 2 }], + }, + ], +}); + +describe("no-unsanctioned-sleep exemptions", () => { + /** + * The exemption list is consumed by two separate eslint configs. Asserting the + * paths exist stops a rename from silently un-exempting the helper that + * implements both banned forms — which would surface as the sanctioned wait + * failing the rule that exists to promote it. + */ + it("exempts exactly the causal-waits implementation and its own test", () => { + expect(SLEEP_EXEMPT_FILES).toEqual([ + "e2e/helpers/causal-waits.ts", + "e2e/helpers/causal-waits.test.ts", + ]); + }); +}); diff --git a/apps/web/eslint.config.mjs b/apps/web/eslint.config.mjs index 38afc0b447..f4e060ccf7 100644 --- a/apps/web/eslint.config.mjs +++ b/apps/web/eslint.config.mjs @@ -6,6 +6,11 @@ import unusedImports from "eslint-plugin-unused-imports"; import i18next from "eslint-plugin-i18next"; import tseslint from "typescript-eslint"; import { i18nGuardFiles, noLiteralStringOptions } from "./eslint.i18n.options.mjs"; +import { + e2eSleepGuardFiles, + e2eSleepPlugin, + SLEEP_EXEMPT_FILES, +} from "./eslint-rules/no-unsanctioned-sleep.mjs"; const eslintConfig = defineConfig([ { @@ -77,6 +82,21 @@ 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. + { + files: e2eSleepGuardFiles, + // `causal-waits.ts` implements both banned forms — see SLEEP_EXEMPT_FILES. + ignores: SLEEP_EXEMPT_FILES, + plugins: { "e2e-sleeps": e2eSleepPlugin }, + rules: { "e2e-sleeps/no-unsanctioned-sleep": "error" }, + }, ]); export default eslintConfig; diff --git a/apps/web/eslint.e2e-sleeps.config.mjs b/apps/web/eslint.e2e-sleeps.config.mjs new file mode 100644 index 0000000000..8c997f1bb5 --- /dev/null +++ b/apps/web/eslint.e2e-sleeps.config.mjs @@ -0,0 +1,40 @@ +import { defineConfig } from "eslint/config"; +import tseslint from "typescript-eslint"; + +import { e2eSleepPlugin, SLEEP_EXEMPT_FILES } from "./eslint-rules/no-unsanctioned-sleep.mjs"; + +/** + * 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: + * + * - `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. + * + * 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. + */ +export default defineConfig([ + { + // BOTH extensions. `isE2eCandidate` selects `.tsx` too, so a config matching + // only `.ts` left a selected file matching no config at all — and + // `warnIgnored: false` in the ratchet suppressed the one signal that would + // have shown it, so the gate passed silently. Keep this in step with the + // selector. + files: ["**/*.ts", "**/*.tsx"], + ignores: [...SLEEP_EXEMPT_FILES, "node_modules/**", "dist/**"], + languageOptions: { parser: tseslint.parser }, + plugins: { "e2e-sleeps": e2eSleepPlugin }, + rules: { "e2e-sleeps/no-unsanctioned-sleep": "error" }, + }, +]); diff --git a/apps/web/package.json b/apps/web/package.json index 2b76590695..4753f5cd63 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -12,6 +12,7 @@ "start": "vite preview --host 0.0.0.0", "lint": "eslint --max-warnings 0", "lint:i18n": "eslint --config eslint.i18n.config.mjs", + "lint:e2e-sleeps": "eslint --config eslint.e2e-sleeps.config.mjs", "pretypecheck": "node scripts/generate-release-notes.mjs && node scripts/generate-changelog.mjs", "typecheck": "tsc --noEmit", "pretest": "node scripts/generate-release-notes.mjs && node scripts/generate-changelog.mjs", @@ -23,6 +24,8 @@ "e2e:ui": "playwright test --config e2e/playwright.config.ts --ui", "e2e:headed": "playwright test --config e2e/playwright.config.ts --headed", "e2e:capture": "cross-env CAPTURE_PR_ASSETS=true playwright test --config e2e/playwright.config.ts && tsx e2e/scripts/convert-assets.ts", + "e2e:sleep-ratchet": "node scripts/check-new-e2e-sleeps.mjs", + "e2e:waits": "node scripts/report-e2e-waits.mjs", "licenses:gen": "tsx scripts/generate-licenses.ts", "i18n:pseudo": "node scripts/generate-pseudo-locale.mjs", "i18n:check": "node scripts/check-i18n-keys.mjs && node scripts/check-trans-indices.mjs && node scripts/check-inline-plurals.mjs && node scripts/check-module-scope-t.mjs && node scripts/check-no-em-dash-ui.mjs", diff --git a/apps/web/scripts/check-new-e2e-sleeps.mjs b/apps/web/scripts/check-new-e2e-sleeps.mjs new file mode 100644 index 0000000000..8ad9cc31b6 --- /dev/null +++ b/apps/web/scripts/check-new-e2e-sleeps.mjs @@ -0,0 +1,77 @@ +#!/usr/bin/env node +/** + * Ratchet: NEW unconditional sleeps in `apps/web/e2e` must go through `dwell()` + * or `injectLatency()` from `e2e/helpers/causal-waits.ts`. + * + * `page.waitForTimeout(500)` and `await new Promise((r) => setTimeout(r, 500))` + * are the suite's most reliable source of flake: a hand-picked number that + * happened to be long enough on the machine it was written on. The sanctioned + * forms take a category and a reason, so a wall-clock wait that survives review + * has to say why no event exists to wait on instead. + * + * ~126 raw sleeps predate this check, and `pnpm lint` is + * `eslint --max-warnings 0`, so registering the rule repo-wide at any severity + * would break every unrelated PR until the conversion finishes — the mistake the + * first i18n migration attempt made. Hence judging the CHANGE, not the FILE: + * + * - a file the change ADDED must be clean outright. New code carries no legacy + * debt, so requiring zero sleeps costs nothing. + * - a file the change MODIFIED is judged only on the lines it touched. Sleeps + * elsewhere in the file belong to the conversion effort, not to this PR. + * + * Same contract, same plumbing and same fork-point handling as + * `check-new-code-i18n.mjs`, which mirrors `golangci-lint --new-from-rev`. + * + * The decision logic lives in `lib/e2e-sleep-ratchet.mjs` so it can be driven + * against real git topologies in a test; this file is the shell that supplies + * the real repository and turns the result into output and an exit code. + * + * Usage: + * node scripts/check-new-e2e-sleeps.mjs [--base ] + * + * Prefer passing no --base; see lib/git-base.mjs for why an explicit one is + * floored at the fork point. + */ +import path from "node:path"; + +import { findNewSleeps } from "./lib/e2e-sleep-ratchet.mjs"; +import { REPO_ROOT, WEB_DIR } from "./lib/git-base.mjs"; + +const result = await findNewSleeps({ + repoRoot: REPO_ROOT, + webDir: WEB_DIR, + configFile: path.join(WEB_DIR, "eslint.e2e-sleeps.config.mjs"), + argv: process.argv, +}); + +if (result.skip) { + console.log(`⚠ e2e sleep ratchet skipped — ${result.skip}`); + process.exit(0); +} + +const { added, modified, violations } = result; + +if (added.length === 0 && modified.length === 0) { + console.log("✓ e2e sleep ratchet — no e2e source added or modified"); + process.exit(0); +} + +if (violations.length === 0) { + console.log( + `✓ e2e sleep ratchet — ${added.length} added + ${modified.length} modified e2e file(s) clean`, + ); + process.exit(0); +} + +console.error( + `\n✖ ${violations.length} unconditional sleep(s) in new e2e code.\n` + + ` A wall-clock wait must name its category and its reason, so use dwell() or\n` + + ` injectLatency() from e2e/helpers/causal-waits.ts. Better still, wait on the\n` + + ` cause: waitForHttp() / watchWs(). See apps/web/e2e/README.md.\n`, +); +for (const violation of violations) { + const scope = violation.whole ? "new file" : "changed line"; + console.error(` ${violation.repoPath}:${violation.line} [${scope}] ${violation.text}`); +} +console.error(""); +process.exit(1); diff --git a/apps/web/scripts/lib/changed-files.mjs b/apps/web/scripts/lib/changed-files.mjs index d7dd024c38..0c3437f79a 100644 --- a/apps/web/scripts/lib/changed-files.mjs +++ b/apps/web/scripts/lib/changed-files.mjs @@ -21,18 +21,56 @@ export function isCandidate(repoPath) { } /** - * Candidate paths matching `filter` (a `--diff-filter` value) between `base` and - * the working tree. + * Only Playwright E2E sources, for the sleep ratchet. + * + * The exact complement of the `e2e/` exclusion in {@link isCandidate}: the i18n + * guard skips this tree because tests build fixtures out of literals on purpose, + * and the sleep guard exists only for this tree. `.spec.ts` files are the + * *primary* target here rather than an exclusion, which is why this cannot be + * expressed as options on the other predicate. + * + * `causal-waits.ts` is not excluded here. Exemption belongs to the eslint config + * (`SLEEP_EXEMPT_FILES`), so the editor, the preview and the gate all read one + * list; filtering it out at selection time as well would be a second place to + * keep in sync, and the one that fails silently. + */ +export function isE2eCandidate(repoPath) { + if (!repoPath.startsWith(WEB_PREFIX)) return false; + const rel = repoPath.slice(WEB_PREFIX.length); + return rel.startsWith("e2e/") && /\.tsx?$/.test(rel); +} + +/** + * Every path matching `filter` (a `--diff-filter` value) between `base` and the + * working tree, unfiltered. * * `--find-renames` explicitly: rename detection defaults on since Git 2.9, but a * repo with diff.renames=false would report a moved file as ADDED, and an added * file is judged whole — demanding a full migration for a plain `git mv`. */ +export function changedPaths(base, filter, cwd) { + return ( + git(["diff", "--name-only", "--find-renames", `--diff-filter=${filter}`, base], { cwd }) + .split("\n") + .map((line) => line.trim()) + // `"".split("\n")` is `[""]`, so a no-match diff would otherwise yield one + // empty "path". Both predicates reject it, but this function is exported and + // its next caller should not have to know that. + .filter(Boolean) + ); +} + +/** + * Candidate UI-source paths matching `filter`. + * + * A pure delegation to {@link changedPaths} + {@link isCandidate}: the i18n + * ratchet's file selection is byte-for-byte what it was before the sleep ratchet + * needed the same git invocation with a different predicate. Kept as its own + * export rather than inlined at the call site so that stays true by + * construction. + */ export function changedFiles(base, filter, cwd) { - return git(["diff", "--name-only", "--find-renames", `--diff-filter=${filter}`, base], { cwd }) - .split("\n") - .map((line) => line.trim()) - .filter(isCandidate); + return changedPaths(base, filter, cwd).filter(isCandidate); } /** diff --git a/apps/web/scripts/lib/changed-files.test.ts b/apps/web/scripts/lib/changed-files.test.ts new file mode 100644 index 0000000000..c3635be9e3 --- /dev/null +++ b/apps/web/scripts/lib/changed-files.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from "vitest"; + +import { isCandidate, isE2eCandidate } from "./changed-files.mjs"; + +/** + * `isCandidate` decides which files the **live i18n ratchet** judges, and had no + * test until the sleep ratchet needed the same git plumbing with a different + * predicate. These cases pin the existing behaviour so that extraction is + * provably behaviour-preserving, and pin the new predicate as its exact + * complement over the `e2e/` tree. + */ +describe("isCandidate (i18n ratchet file selection)", () => { + it.each([ + "apps/web/components/task/card.tsx", + "apps/web/app/settings/page.tsx", + "apps/web/hooks/domains/kanban/use-board.ts", + "apps/web/lib/state/store.ts", + "apps/web/src/settings-routes.tsx", + ])("includes UI source %s", (path) => { + expect(isCandidate(path)).toBe(true); + }); + + it.each([ + // Tests build fixtures out of literals on purpose. + ["apps/web/components/task/card.test.tsx", "unit test"], + ["apps/web/lib/util.spec.ts", "spec file"], + // Not UI source. + ["apps/web/e2e/tests/task/chat.spec.ts", "e2e tree"], + ["apps/web/scripts/check-i18n-keys.mjs", "build script"], + ["apps/web/eslint.config.mjs", "config, not .ts"], + ["apps/web/components/task/styles.css", "not TypeScript"], + ["apps/backend/internal/office/router.go", "outside apps/web"], + ["apps/packages/ui/src/button.tsx", "outside apps/web"], + ])("excludes %s (%s)", (path) => { + expect(isCandidate(path)).toBe(false); + }); +}); + +describe("isE2eCandidate (sleep ratchet file selection)", () => { + it.each([ + "apps/web/e2e/tests/task/chat.spec.ts", + "apps/web/e2e/helpers/api-client.ts", + "apps/web/e2e/fixtures/backend.ts", + "apps/web/e2e/pages/task-page.ts", + // Exemption is the eslint config's job, not the selector's — this path is + // deliberately still selected here. + "apps/web/e2e/helpers/causal-waits.ts", + ])("includes %s", (path) => { + expect(isE2eCandidate(path)).toBe(true); + }); + + it.each([ + ["apps/web/components/task/card.tsx", "UI source, not e2e"], + ["apps/web/scripts/check-new-e2e-sleeps.mjs", "build script"], + ["apps/web/e2e/README.md", "not TypeScript"], + ["apps/web/e2e/scripts/seed.js", "plain JS is not parsed by the rule config"], + ["apps/backend/internal/office/router.go", "outside apps/web"], + ])("excludes %s (%s)", (path) => { + expect(isE2eCandidate(path)).toBe(false); + }); + + /** + * The two predicates must not both claim a file: the i18n guard would demand + * catalog keys for test fixture strings, which is why `e2e/` was excluded from + * it in the first place. + */ + it("never overlaps with the i18n predicate", () => { + const paths = [ + "apps/web/e2e/tests/task/chat.spec.ts", + "apps/web/e2e/helpers/api-client.ts", + "apps/web/components/task/card.tsx", + "apps/web/lib/state/store.ts", + ]; + for (const path of paths) { + expect(isCandidate(path) && isE2eCandidate(path)).toBe(false); + } + }); +}); diff --git a/apps/web/scripts/lib/e2e-sleep-ratchet.mjs b/apps/web/scripts/lib/e2e-sleep-ratchet.mjs new file mode 100644 index 0000000000..c684f4c40d --- /dev/null +++ b/apps/web/scripts/lib/e2e-sleep-ratchet.mjs @@ -0,0 +1,79 @@ +/** + * The sleep ratchet's decision logic, separated from its CLI shell. + * + * Split out so the ratchet can be driven against real git topologies in a test + * — a temp repository with the same `apps/web/e2e/…` layout — rather than only + * through its individual helpers. Testing `parseAddedLineRanges` and the eslint + * rule proves each part works; it does not prove the parts are wired together, + * and a gate that is perfectly tested but never actually consulted reports a + * clean tree forever. `findNewSleeps` is the seam where those two claims meet, + * so it is the thing the tests drive. + * + * Everything path-shaped is a parameter for the same reason. The CLI supplies + * the real repository; the tests supply a throwaway one. + */ +import path from "node:path"; + +import { ESLint } from "eslint"; + +import { SLEEP_RULE_ID } from "../../eslint-rules/no-unsanctioned-sleep.mjs"; +import { changedPaths, isE2eCandidate, webDiff } from "./changed-files.mjs"; +import { lineInRanges, parseAddedLineRanges } from "./diff-ranges.mjs"; +import { resolveBase, toPosixPath } from "./git-base.mjs"; + +/** + * Sleeps introduced by the change between the resolved base and the working + * tree. + * + * Added files are judged whole; modified files only on the lines they touched. + * + * @returns `{ skip }` when no base could be resolved and skipping is allowed, + * otherwise `{ base, added, modified, violations }`. + */ +export async function findNewSleeps({ repoRoot, webDir, configFile, argv = [] }) { + const resolved = resolveBase(argv, repoRoot); + if (resolved.skip) return { skip: resolved.skip }; + const { base } = resolved; + + const added = changedPaths(base, "A", repoRoot).filter(isE2eCandidate); + // Renames are judged like modifications: moving a legacy spec must not + // suddenly demand that every sleep in it be converted. A pure rename adds no + // lines and so reports nothing. + const modified = changedPaths(base, "MRC", repoRoot).filter(isE2eCandidate); + const targets = [...added, ...modified]; + if (targets.length === 0) return { base, added, modified, violations: [] }; + + const addedRanges = + modified.length === 0 ? new Map() : parseAddedLineRanges(webDiff(base, repoRoot)); + + const eslint = new ESLint({ + cwd: webDir, + overrideConfigFile: configFile, + // `causal-waits.ts` is in the config's `ignores`, and passing an ignored + // file explicitly is a warning that would otherwise print on every run that + // touches it. + warnIgnored: false, + }); + const results = await eslint.lintFiles(targets.map((file) => path.join(repoRoot, file))); + + const addedSet = new Set(added); + const violations = []; + for (const result of results) { + const repoPath = toPosixPath(path.relative(repoRoot, result.filePath)); + const whole = addedSet.has(repoPath); + const ranges = addedRanges.get(repoPath); + for (const message of result.messages) { + if (message.ruleId !== SLEEP_RULE_ID) continue; + if (!whole && !lineInRanges(ranges, message.line)) continue; + violations.push({ + repoPath, + whole, + line: message.line, + messageId: message.messageId, + text: message.message, + }); + } + } + + return { base, added, modified, violations }; +} diff --git a/apps/web/scripts/lib/e2e-sleep-ratchet.test.ts b/apps/web/scripts/lib/e2e-sleep-ratchet.test.ts new file mode 100644 index 0000000000..e077bdb2b9 --- /dev/null +++ b/apps/web/scripts/lib/e2e-sleep-ratchet.test.ts @@ -0,0 +1,312 @@ +/** + * The sleep ratchet driven end to end against real git topologies. + * + * The rule has its own RuleTester suite and the diff parser has its own; those + * prove the parts. This proves they are *connected* — that a sleep on a new line + * actually reaches an exit code, and that a sleep on an untouched line actually + * does not. A gate whose matcher is perfect but whose wiring is broken reports a + * clean tree forever, which is the one failure mode nobody goes looking for. + * + * Every scenario is a real repository with the real `apps/web/e2e/…` layout and + * the real eslint config, so nothing here can pass by mocking the thing under + * test. + */ +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterAll, describe, expect, it } from "vitest"; + +import { findNewSleeps } from "./e2e-sleep-ratchet.mjs"; + +const REAL_WEB_DIR = path.resolve(import.meta.dirname, "../.."); +const CONFIG_FILE = path.join(REAL_WEB_DIR, "eslint.e2e-sleeps.config.mjs"); + +const SPEC = "apps/web/e2e/tests/demo/demo.spec.ts"; + +/** The line each scenario inserts before, extracted because it appears in five. */ +const ANCHOR = ' await page.click("#done");'; + +/** A spec with a sleep at a known line, plus filler to edit elsewhere. */ +const LEGACY_SPEC = [ + 'import { test } from "@playwright/test";', // 1 + "", // 2 + 'test("legacy", async ({ page }) => {', // 3 + ' await page.goto("/");', // 4 + " await page.waitForTimeout(500);", // 5 <- the pre-existing sleep + ANCHOR, // 6 + "});", // 7 + "", // 8 +].join("\n"); + +const tmpRoots: string[] = []; +afterAll(() => { + for (const dir of tmpRoots) fs.rmSync(dir, { recursive: true, force: true }); +}); + +function git(cwd: string, args: string[]) { + return execFileSync( + "git", + [ + "-c", + "user.email=ratchet@example.test", + "-c", + "user.name=Ratchet Test", + "-c", + "commit.gpgsign=false", + // A developer machine may set core.hooksPath globally; an inherited + // pre-commit hook would fail these fixture commits for unrelated reasons. + "-c", + "core.hooksPath=/nonexistent", + ...args, + ], + { cwd, encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] }, + ); +} + +function write(dir: string, rel: string, body: string) { + const file = path.join(dir, rel); + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, body); +} + +/** + * Stage everything, which is what makes a NEW file visible to the ratchet. + * + * `git diff ` compares a commit against the working tree and does not + * report untracked paths at all, so an unstaged new file is invisible here. That + * is not a gap: pre-commit runs its hooks after staging, and CI judges commits, + * so every path that reaches this gate for real is tracked. Modified files need + * no staging, and are deliberately left unstaged below to prove the ratchet does + * not depend on it. + */ +function stage(dir: string) { + git(dir, ["add", "-A"]); +} + +/** + * A repo whose `main` already contains `LEGACY_SPEC`, with `origin/main` + * pointing at it — the shape `resolveBase` expects — and HEAD on a branch off + * that commit, so working-tree edits read as this change's own. + */ +function repoWithLegacySleep() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "kandev-sleep-ratchet-")); + tmpRoots.push(dir); + git(dir, ["init", "-q", "-b", "main"]); + write(dir, SPEC, LEGACY_SPEC); + git(dir, ["add", "-A"]); + git(dir, ["commit", "-q", "-m", "base"]); + // `origin/main` without a remote: a local ref under refs/remotes is all + // `merge-base origin/main HEAD` needs. + git(dir, ["update-ref", "refs/remotes/origin/main", "HEAD"]); + git(dir, ["checkout", "-q", "-b", "feature"]); + return dir; +} + +const run = (dir: string) => + findNewSleeps({ + repoRoot: dir, + webDir: path.join(dir, "apps/web"), + configFile: CONFIG_FILE, + }) as Promise<{ + violations: Array<{ repoPath: string; line: number; whole: boolean; messageId: string }>; + added: string[]; + modified: string[]; + }>; + +describe("e2e sleep ratchet", () => { + it("FAILS on a waitForTimeout added on a new line", async () => { + const dir = repoWithLegacySleep(); + write( + dir, + SPEC, + LEGACY_SPEC.replace( + ANCHOR, + ' await page.waitForTimeout(250);\n await page.click("#done");', + ), + ); + + const { violations } = await run(dir); + expect(violations).toHaveLength(1); + expect(violations[0]).toMatchObject({ line: 6, whole: false, messageId: "waitForTimeout" }); + }); + + it("FAILS on a raw promise sleep added on a new line", async () => { + const dir = repoWithLegacySleep(); + write( + dir, + SPEC, + LEGACY_SPEC.replace( + ANCHOR, + ' await new Promise((r) => setTimeout(r, 250));\n await page.click("#done");', + ), + ); + + const { violations } = await run(dir); + expect(violations).toHaveLength(1); + expect(violations[0]).toMatchObject({ line: 6, whole: false, messageId: "promiseSleep" }); + }); + + /** + * The property that makes this mergeable at all: touching a line next to + * somebody else's sleep must not make it yours. Without it the ratchet is a + * migration treadmill and the first PR to edit a legacy spec is blocked. + */ + it("PASSES when a line is edited right next to an existing untouched sleep", async () => { + const dir = repoWithLegacySleep(); + write( + dir, + SPEC, + LEGACY_SPEC.replace(' await page.goto("/");', ' await page.goto("/tasks");'), + ); + + const { modified, violations } = await run(dir); + expect(modified).toEqual([SPEC]); + expect(violations).toEqual([]); + }); + + it("PASSES on an existing untouched sleep with no changes at all", async () => { + const { modified, added, violations } = await run(repoWithLegacySleep()); + expect(added).toEqual([]); + expect(modified).toEqual([]); + expect(violations).toEqual([]); + }); + + it("PASSES on the sanctioned dwell() and injectLatency() forms", async () => { + const dir = repoWithLegacySleep(); + write( + dir, + SPEC, + LEGACY_SPEC.replace( + 'import { test } from "@playwright/test";', + 'import { test } from "@playwright/test";\nimport { dwell, injectLatency } from "../../helpers/causal-waits";', + ).replace( + ANCHOR, + [ + ' await dwell(page, 300, "library-timer", "Radix open delay publishes nothing");', + ' await dwell(300, "poll-interval", "backend health poll; no page here");', + ' await injectLatency(800, "make the in-flight spinner observable");', + ANCHOR, + ].join("\n"), + ), + ); + + const { violations } = await run(dir); + expect(violations).toEqual([]); + }); +}); + +/** + * File-level selection: which paths the gate judges whole, which it judges by + * line, and which it does not judge at all. + */ +describe("e2e sleep ratchet — file selection", () => { + /** An added file carries no legacy debt, so it is judged whole. */ + it("FAILS on a sleep anywhere in a newly ADDED file", async () => { + const dir = repoWithLegacySleep(); + write(dir, "apps/web/e2e/tests/demo/new.spec.ts", LEGACY_SPEC); + stage(dir); + + const { added, violations } = await run(dir); + expect(added).toEqual(["apps/web/e2e/tests/demo/new.spec.ts"]); + expect(violations).toHaveLength(1); + expect(violations[0]).toMatchObject({ line: 5, whole: true }); + }); + + /** + * A `git mv` of a legacy spec adds no lines, so it must report nothing. + * Judging a rename as an addition would demand a full conversion for a move. + */ + it("PASSES on a pure rename of a file full of legacy sleeps", async () => { + const dir = repoWithLegacySleep(); + git(dir, ["mv", SPEC, "apps/web/e2e/tests/demo/renamed.spec.ts"]); + + const { violations } = await run(dir); + expect(violations).toEqual([]); + }); + + /** + * `.tsx` is selected by `isE2eCandidate`, so the eslint config must match it + * too. It did not: the file was linted against no config at all, and + * `warnIgnored: false` suppressed the only signal that would have shown it, so + * the gate PASSED on a file full of sleeps. Found in review; this is the + * regression test, and it fails against a `files: ["**\/*.ts"]` config. + */ + it("FAILS on a sleep in a .tsx file, which the selector also picks up", async () => { + const dir = repoWithLegacySleep(); + write(dir, "apps/web/e2e/fixtures/probe-component.tsx", LEGACY_SPEC); + stage(dir); + + const { added, violations } = await run(dir); + expect(added).toEqual(["apps/web/e2e/fixtures/probe-component.tsx"]); + expect(violations).toHaveLength(1); + expect(violations[0]).toMatchObject({ line: 5, whole: true, messageId: "waitForTimeout" }); + }); + + /** + * The finding must land on the changed line. Reporting the whole + * `CallExpression` put it on the receiver, so converting `.click(…)` to + * `.waitForTimeout(…)` in a wrapped chain left the finding on an UNCHANGED + * line and the ratchet dropped it. Found in review. + */ + it("FAILS when only the method line of a wrapped call changes", async () => { + const dir = repoWithLegacySleep(); + const wrapped = [ + 'import { test } from "@playwright/test";', + "", + 'test("wrapped", async ({ page }) => {', + " await page", + ' .click("#a");', + "});", + "", + ].join("\n"); + write(dir, SPEC, wrapped); + git(dir, ["add", "-A"]); + git(dir, ["commit", "-q", "-m", "wrapped chain"]); + git(dir, ["update-ref", "refs/remotes/origin/main", "HEAD"]); + // Only line 5 changes; line 4 (`await page`) is untouched. + write(dir, SPEC, wrapped.replace(' .click("#a");', " .waitForTimeout(500);")); + + const { violations } = await run(dir); + expect(violations).toHaveLength(1); + expect(violations[0]).toMatchObject({ line: 5, whole: false, messageId: "waitForTimeout" }); + }); + + /** Non-e2e changes are none of this gate's business. */ + it("ignores changes outside apps/web/e2e", async () => { + const dir = repoWithLegacySleep(); + write(dir, "apps/web/components/card.tsx", "export const C = () => null;\n"); + write(dir, "apps/backend/main.go", "package main\n"); + + const { added, modified, violations } = await run(dir); + expect(added).toEqual([]); + expect(modified).toEqual([]); + expect(violations).toEqual([]); + }); + + /** + * `causal-waits.ts` implements both banned forms. If the exemption ever stops + * applying, the helper the whole effort promotes becomes unlandable. + */ + it("exempts the causal-waits helper even when added whole", async () => { + const dir = repoWithLegacySleep(); + write( + dir, + "apps/web/e2e/helpers/causal-waits.ts", + [ + "export function dwell(page: Page, ms: number): Promise {", + " return page.waitForTimeout(ms);", + "}", + "export function injectLatency(ms: number): Promise {", + " return new Promise((resolve) => setTimeout(resolve, ms));", + "}", + "", + ].join("\n"), + ); + stage(dir); + + const { added, violations } = await run(dir); + expect(added).toEqual(["apps/web/e2e/helpers/causal-waits.ts"]); + expect(violations).toEqual([]); + }); +}); diff --git a/apps/web/scripts/lib/e2e-sleep-wiring.test.ts b/apps/web/scripts/lib/e2e-sleep-wiring.test.ts new file mode 100644 index 0000000000..a5df5fb993 --- /dev/null +++ b/apps/web/scripts/lib/e2e-sleep-wiring.test.ts @@ -0,0 +1,134 @@ +/** + * The ratchet is actually WIRED UP. + * + * Every other test here proves the matcher is right and the diff attribution is + * right. None of them would notice if the CI step, the pre-commit hook or the + * script's own call to `findNewSleeps` were deleted — the rule would still be + * correct, its tests would still pass, and the gate would silently never run + * again. That is the worst failure mode available to a check: it reports a clean + * tree forever, in the one place nobody thinks to look. + * + * So these assert the call sites, not the logic. They are the mutants + * "delete the CI step", "delete the hook", and "delete the call while keeping + * the function". + * + * Matching is on the script FILENAME rather than on surrounding YAML structure, + * so reformatting, renaming a step or reordering the workflow does not break + * them — only actually removing the invocation does. + */ +import fs from "node:fs"; +import path from "node:path"; + +import { describe, expect, it } from "vitest"; + +const WEB_DIR = path.resolve(import.meta.dirname, "../.."); +const REPO_ROOT = path.resolve(WEB_DIR, "../.."); + +const RATCHET_SCRIPT = "check-new-e2e-sleeps.mjs"; + +const read = (rel: string) => fs.readFileSync(path.join(REPO_ROOT, rel), "utf8"); + +describe("sleep ratchet wiring", () => { + /** + * Both invocations, not just the filename. + * + * `toContain(RATCHET_SCRIPT)` alone was too weak: the step has a + * `pull_request` branch and a `push` branch, so deleting either one left the + * other's mention of the filename and the assertion still passed. A mutation + * run caught it. Count the call sites instead. + */ + it("runs in CI, in the frontend workflow, on both event paths", () => { + const workflow = read(".github/workflows/frontend-tests.yml"); + expect(workflow).toMatch(/^\s+- name: Run E2E sleep ratchet$/m); + const calls = workflow.match(new RegExp(`node scripts/${RATCHET_SCRIPT}`, "g")) ?? []; + expect(calls).toHaveLength(2); + }); + + /** + * `pull_request` must NOT pass `--base`. The checkout is the merge ref and + * `pull_request.base.sha` is an ancestor of it, so handing it in makes every + * commit main landed since the PR opened read as this PR's new code (#2160 + * did exactly this to the i18n ratchet). + */ + it("does not pass --base on pull_request in CI", () => { + const workflow = read(".github/workflows/frontend-tests.yml"); + const step = workflow.slice(workflow.indexOf("Run E2E sleep ratchet")); + // Match the pull_request branch structurally rather than slicing at the + // first `BASE_SHA=` literal: a future YAML comment mentioning that name + // would move the slice boundary and quietly shrink what is asserted + // (review finding). This is strictly tighter than the slice was — it pins + // the invocation, its bare form, and the early exit, in order. + expect(step).toMatch( + new RegExp( + `if \\[\\[ "\\$\\{\\{ github.event_name \\}\\}" == "pull_request" \\]\\];? then\\n` + + `\\s+node scripts/${RATCHET_SCRIPT}\\n` + + `\\s+exit 0\\n`, + ), + ); + }); + + /** + * The hook id is matched to end-of-line. `toContain("id: e2e-sleep-ratchet")` + * was satisfied by `id: e2e-sleep-ratchet-disabled`, so renaming the hook to + * switch it off passed the test — a mutation run caught it. + */ + it("runs as a pre-commit hook on e2e changes", () => { + const config = read(".pre-commit-config.yaml"); + expect(config).toMatch(/^\s+- id: e2e-sleep-ratchet$/m); + expect(config).toContain(RATCHET_SCRIPT); + // The hook must fire for the tree it guards. + expect(config).toContain("^apps/web/e2e/.*\\.tsx?$"); + }); + + it("is reachable as a package script", () => { + const pkg = JSON.parse(read("apps/web/package.json")); + expect(pkg.scripts["e2e:sleep-ratchet"]).toContain(RATCHET_SCRIPT); + expect(pkg.scripts["e2e:waits"]).toContain("report-e2e-waits.mjs"); + expect(pkg.scripts["lint:e2e-sleeps"]).toContain("eslint.e2e-sleeps.config.mjs"); + }); + + /** + * The CLI must actually invoke the decision function. A shell that imports it + * and then exits 0 unconditionally is the "delete the call, keep the function" + * mutant, and it looks completely healthy from the outside. + */ + it("the CLI calls findNewSleeps and exits non-zero on violations", () => { + const cli = read(`apps/web/scripts/${RATCHET_SCRIPT}`); + expect(cli).toContain("await findNewSleeps("); + expect(cli).toContain("process.exit(1)"); + // The failure exit must be reachable from the violations branch, not + // orphaned below an unconditional success exit. + expect(cli.indexOf("violations.length === 0")).toBeLessThan(cli.indexOf("process.exit(1)")); + }); + + /** + * The eslint rule is registered in the main config too, on the directories + * already converted. Losing this registration removes editor feedback and the + * graduation path, without any test elsewhere noticing. + */ + it("registers the rule in the main eslint config", () => { + const config = read("apps/web/eslint.config.mjs"); + expect(config).toContain("e2eSleepGuardFiles"); + expect(config).toContain('"e2e-sleeps/no-unsanctioned-sleep": "error"'); + }); + + /** + * 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. + */ + 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}"`); + } + }); +}); diff --git a/apps/web/scripts/lib/e2e-wait-inventory.mjs b/apps/web/scripts/lib/e2e-wait-inventory.mjs new file mode 100644 index 0000000000..e3b96072c0 --- /dev/null +++ b/apps/web/scripts/lib/e2e-wait-inventory.mjs @@ -0,0 +1,152 @@ +/** + * Inventory of the *sanctioned* waits in `apps/web/e2e` — the debt report's + * data layer. + * + * Counting raw sleeps is not this module's job: those come from the eslint rule + * (`eslint-rules/no-unsanctioned-sleep.mjs`), so there is exactly one definition + * of what a banned sleep is. This module only reads the two legal forms, which + * the rule never sees. + * + * ## What the numbers mean + * + * `dwell` carries a category, and the categories are not interchangeable: + * + * - **`negative-assertion` is permanent and is NOT debt.** A test asserting + * that something never happens has no event to wait for. "Fixing" one by + * deleting the wait removes the window in which the regression could occur + * and leaves a test that passes for free. It is reported separately so + * nobody optimises it toward zero. + * - **`unverified` is the debt to drive to zero.** It means the author could + * not name the timer. Every other category names one. + * - The five named-timer categories are compromises of varying fixability, + * reported so a conversion effort can pick targets. + * + * `injectLatency` is counted entirely separately. It delays the system under + * test on purpose — the delay is the stimulus, and removing it destroys the + * scenario. Folding it into a sleep total would let deliberate fixture + * configuration inflate a number that is supposed to be shrinking. + */ +// `typescript-eslint`, not `@typescript-eslint/typescript-estree` directly: +// pnpm's strict layout does not hoist the transitive package, so importing it +// fails with ERR_MODULE_NOT_FOUND. The umbrella package is a declared dependency +// and re-exports the same parser. +import tseslint from "typescript-eslint"; + +/** + * The category list, read out of `causal-waits.ts` itself rather than restated. + * + * A restated copy drifts silently: a category added to the helper would be + * counted as unknown here, and the report would under-count the very thing it + * exists to track. Throws rather than falling back to a hardcoded list, because + * a silently-empty category set makes every dwell read as uncategorised. + */ +export function readDwellCategories(source) { + const match = /export const DWELL_CATEGORIES = \[([\s\S]*?)\] as const;/.exec(source); + if (!match) { + throw new Error( + "e2e-wait-inventory: could not find `export const DWELL_CATEGORIES = [...] as const;` in " + + "causal-waits.ts. The report derives categories from the helper so the two cannot drift; " + + "update this parser rather than hardcoding a list.", + ); + } + const categories = [...match[1].matchAll(/"([a-z-]+)"/g)].map((entry) => entry[1]); + if (categories.length === 0) { + throw new Error("e2e-wait-inventory: DWELL_CATEGORIES parsed to an empty list"); + } + return categories; +} + +function walk(node, visit) { + if (!node || typeof node.type !== "string") return; + visit(node); + for (const key of Object.keys(node)) { + if (key === "parent") continue; + const value = node[key]; + if (Array.isArray(value)) { + for (const child of value) walk(child, visit); + } else if (value && typeof value === "object") { + walk(value, visit); + } + } +} + +function calleeName(node) { + return node.callee?.type === "Identifier" ? node.callee.name : null; +} + +/** + * The category argument of a `dwell()` call. + * + * `dwell(ms, category, reason)` and `dwell(page, ms, category, reason)` are + * overloads on one name, so the category sits at index 1 or 2 depending on the + * form. It is always the FIRST string-literal argument either way — `ms` is + * numeric and `page` is an identifier — and `reason` is the second. Matching by + * position would need the two forms told apart; matching the first string does + * not, and `dwell` validates the value at runtime against the same list, so an + * unrecognised one here is a real miscategorisation rather than a parse gap. + */ +function dwellCategory(node, categories) { + const strings = node.arguments.filter( + (argument) => argument.type === "Literal" && typeof argument.value === "string", + ); + const category = strings[0]?.value; + if (category === undefined) return "«no literal category»"; + return categories.includes(category) ? category : `«unknown: ${category}»`; +} + +/** + * Sanctioned waits in one file. + * + * @returns {{ dwells: Array<{category: string, line: number}>, injectLatency: number }} + */ +export function inventorySource(source, filePath, categories) { + const { ast } = tseslint.parser.parseForESLint(source, { loc: true, range: true }); + const dwells = []; + let injectLatency = 0; + + walk(ast, (node) => { + if (node.type !== "CallExpression") return; + const name = calleeName(node); + if (name === "dwell") { + dwells.push({ category: dwellCategory(node, categories), line: node.loc.start.line }); + } else if (name === "injectLatency") { + injectLatency += 1; + } + }); + + return { filePath, dwells, injectLatency }; +} + +/** + * Fold per-file inventories into the report's totals. + * + * `negativeAssertion` is excluded from `debt` on purpose — see the module + * comment. `debt` is the number a conversion effort is trying to move. + */ +export function summarize(inventories, categories) { + const byCategory = Object.fromEntries(categories.map((category) => [category, 0])); + let unknown = 0; + let injectLatency = 0; + + for (const inventory of inventories) { + injectLatency += inventory.injectLatency; + for (const dwell of inventory.dwells) { + if (dwell.category in byCategory) byCategory[dwell.category] += 1; + else unknown += 1; + } + } + + const permanent = byCategory["negative-assertion"] ?? 0; + const total = Object.values(byCategory).reduce((sum, count) => sum + count, 0) + unknown; + + return { + byCategory, + unknown, + injectLatency, + totalDwells: total, + permanent, + // Everything that is a compromise rather than a fact of the test. + debt: total - permanent, + unverified: byCategory.unverified ?? 0, + }; +} diff --git a/apps/web/scripts/lib/e2e-wait-inventory.test.ts b/apps/web/scripts/lib/e2e-wait-inventory.test.ts new file mode 100644 index 0000000000..b991b61e18 --- /dev/null +++ b/apps/web/scripts/lib/e2e-wait-inventory.test.ts @@ -0,0 +1,163 @@ +import fs from "node:fs"; +import path from "node:path"; + +import { describe, expect, it } from "vitest"; + +import { inventorySource, readDwellCategories, summarize } from "./e2e-wait-inventory.mjs"; + +const CAUSAL_WAITS = path.resolve(import.meta.dirname, "../../e2e/helpers/causal-waits.ts"); +const CATEGORIES = readDwellCategories(fs.readFileSync(CAUSAL_WAITS, "utf8")) as string[]; + +/** Extracted because it is the one category that is permanent rather than debt. */ +const NEGATIVE = "negative-assertion"; + +const inventory = (code: string) => inventorySource(code, "fixture.ts", CATEGORIES); + +describe("readDwellCategories", () => { + /** + * Read from the helper rather than restated here. If `causal-waits.ts` grows a + * category, this picks it up; if the export is renamed or reshaped, the parse + * throws instead of silently reporting every dwell as uncategorised. + */ + it("reads the live category list out of causal-waits.ts", () => { + expect(CATEGORIES).toEqual([ + NEGATIVE, + "product-timer", + "library-timer", + "clock-separation", + "poll-interval", + "browser-chrome", + "unverified", + ]); + }); + + it("throws rather than guessing when the export cannot be found", () => { + expect(() => readDwellCategories("export const SOMETHING_ELSE = [];")).toThrow( + /could not find/i, + ); + }); + + it("throws when the list parses to nothing", () => { + expect(() => readDwellCategories("export const DWELL_CATEGORIES = [] as const;")).toThrow( + /empty list/i, + ); + }); +}); + +describe("inventorySource", () => { + /** + * The two overloads put `category` at a different index — 1 without a page, 2 + * with one. Both must land in the same bucket, or the report splits one + * category across two rows depending on which form the author used. + */ + it("reads the category from the page-less form", () => { + const found = inventory('await dwell(500, "poll-interval", "backend health poll");'); + expect(found.dwells).toEqual([{ category: "poll-interval", line: 1 }]); + }); + + it("reads the category from the page form", () => { + const found = inventory('await dwell(page, 300, "library-timer", "Radix open delay");'); + expect(found.dwells).toEqual([{ category: "library-timer", line: 1 }]); + }); + + it("counts every category separately across a file", () => { + const found = inventory(` + await dwell(page, 300, "${NEGATIVE}", "asserting the toast never appears"); + await dwell(100, "unverified", "pre-existing spacing, no timer identified"); + await dwell(page, 50, "clock-separation", "keep the two writes distinguishable"); + `); + expect(found.dwells.map((entry: { category: string }) => entry.category)).toEqual([ + NEGATIVE, + "unverified", + "clock-separation", + ]); + }); + + it("flags a category that is not in the live list rather than dropping it", () => { + const found = inventory('await dwell(500, "made-up-category", "why");'); + expect(found.dwells[0].category).toBe("«unknown: made-up-category»"); + }); + + it("flags a dwell whose category is not a literal", () => { + const found = inventory("await dwell(500, category, reason);"); + expect(found.dwells[0].category).toBe("«no literal category»"); + }); + + it("counts injectLatency apart from dwell", () => { + const found = inventory(` + await injectLatency(800, "make the spinner observable"); + await dwell(100, "unverified", "no timer identified"); + `); + expect(found.injectLatency).toBe(1); + expect(found.dwells).toHaveLength(1); + }); + + it("parses TypeScript syntax without a type checker", () => { + const found = inventory(` + const wait = async (page: Page): Promise => { + await dwell(page, 10, "browser-chrome", "native print dialog"); + }; + `); + expect(found.dwells).toHaveLength(1); + }); +}); + +describe("summarize", () => { + const build = (categories: string[], injectLatency = 0) => [ + { + filePath: "fixture.ts", + dwells: categories.map((category) => ({ category, line: 1 })), + injectLatency, + }, + ]; + + /** + * The load-bearing assertion of the whole report: `negative-assertion` is + * permanent and must never appear in the number a conversion effort is trying + * to reduce. A test asserting something never happens has no event to wait + * for, and deleting its wait leaves a test that passes for free. + */ + it("excludes negative-assertion from debt", () => { + const summary = summarize(build([NEGATIVE, NEGATIVE]), CATEGORIES); + expect(summary.totalDwells).toBe(2); + expect(summary.permanent).toBe(2); + expect(summary.debt).toBe(0); + }); + + it("counts every other category as debt", () => { + const summary = summarize( + build([NEGATIVE, "unverified", "library-timer", "poll-interval"]), + CATEGORIES, + ); + expect(summary.totalDwells).toBe(4); + expect(summary.permanent).toBe(1); + expect(summary.debt).toBe(3); + expect(summary.unverified).toBe(1); + }); + + /** + * injectLatency is deliberate fixture configuration, correct by construction. + * Folding it in would make adding a legitimate latency simulation read as a + * regression in a number that is supposed to be shrinking. + */ + it("keeps injectLatency out of both dwell totals", () => { + const summary = summarize(build(["unverified"], 5), CATEGORIES); + expect(summary.injectLatency).toBe(5); + expect(summary.totalDwells).toBe(1); + expect(summary.debt).toBe(1); + }); + + it("surfaces uncategorised dwells instead of silently dropping them", () => { + const summary = summarize(build(["«unknown: nope»", "unverified"]), CATEGORIES); + expect(summary.unknown).toBe(1); + expect(summary.totalDwells).toBe(2); + expect(summary.debt).toBe(2); + }); + + it("reports zeros for a suite with no dwells at all", () => { + const summary = summarize([], CATEGORIES); + expect(summary.totalDwells).toBe(0); + expect(summary.debt).toBe(0); + expect(summary.byCategory.unverified).toBe(0); + }); +}); diff --git a/apps/web/scripts/report-e2e-waits.mjs b/apps/web/scripts/report-e2e-waits.mjs new file mode 100644 index 0000000000..54fd584f99 --- /dev/null +++ b/apps/web/scripts/report-e2e-waits.mjs @@ -0,0 +1,129 @@ +#!/usr/bin/env node +/** + * Debt report for wall-clock waits in `apps/web/e2e`. + * + * **This is a report, not a gate.** It never exits non-zero on a count. The + * conversion effort is actively *adding* `unverified` dwells as it replaces raw + * sleeps with categorised ones — that is progress, because an `unverified` + * dwell states its ignorance where a bare `waitForTimeout(500)` hid it — so a + * ratchet on the number would fight the migration it exists to measure. The + * gate is `check-new-e2e-sleeps.mjs`, and it gates the *form*, not the count. + * + * Three numbers, deliberately kept apart: + * + * - **raw sleeps**, counted by the same eslint rule the gate uses, so the + * report and the gate cannot disagree about what a sleep is. This is what + * the conversion drives to zero, and reaching zero is what makes the + * graduation flip (rule to `"error"` on all of `e2e/**`) possible. + * - **dwell debt**, every categorised wait EXCEPT `negative-assertion`. + * `unverified` is called out inside it as the sub-total to attack first. + * - **injectLatency**, counted on its own. It is deliberate fixture + * configuration, correct by construction, and must never be converted; if + * it were folded into the sleep total, adding a legitimate latency + * simulation would look like a regression. + * + * `negative-assertion` is printed under a heading that says it is permanent. + * A test asserting something never happens has no event to wait on, and + * deleting its wait removes the window the regression needs in order to appear. + * + * Usage: node scripts/report-e2e-waits.mjs [--json] + */ +import fs from "node:fs"; +import path from "node:path"; + +import { ESLint } from "eslint"; + +import { SLEEP_EXEMPT_FILES, SLEEP_RULE_ID } from "../eslint-rules/no-unsanctioned-sleep.mjs"; +import { inventorySource, readDwellCategories, summarize } from "./lib/e2e-wait-inventory.mjs"; +import { WEB_DIR } from "./lib/git-base.mjs"; + +const E2E_DIR = path.join(WEB_DIR, "e2e"); +const CAUSAL_WAITS = path.join(E2E_DIR, "helpers", "causal-waits.ts"); + +function collectTsFiles(dir) { + const out = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === "node_modules" || entry.name === "test-results") continue; + out.push(...collectTsFiles(full)); + } else if (entry.name.endsWith(".ts")) { + out.push(full); + } + } + return out; +} + +const categories = readDwellCategories(fs.readFileSync(CAUSAL_WAITS, "utf8")); +const files = collectTsFiles(E2E_DIR); + +// The helper's own definitions and its unit test would otherwise be counted as +// usage sites, inflating every category by the examples that define it. Same +// list the eslint configs use, so "exempt from the rule" and "not a usage site" +// cannot drift apart. +const exempt = new Set(SLEEP_EXEMPT_FILES.map((file) => path.join(WEB_DIR, file))); + +// Sanctioned waits: our own AST walk, since the rule never sees them. +const inventories = files + .filter((file) => !exempt.has(file)) + .map((file) => inventorySource(fs.readFileSync(file, "utf8"), file, categories)); +const dwell = summarize(inventories, categories); + +// Raw sleeps: the gate's own rule, so there is one definition of "a sleep". +const eslint = new ESLint({ + cwd: WEB_DIR, + overrideConfigFile: path.join(WEB_DIR, "eslint.e2e-sleeps.config.mjs"), + warnIgnored: false, +}); +const results = await eslint.lintFiles(files); + +const raw = { waitForTimeout: 0, promiseSleep: 0, byFile: {} }; +for (const result of results) { + const messages = result.messages.filter((message) => message.ruleId === SLEEP_RULE_ID); + if (messages.length === 0) continue; + raw.byFile[path.relative(WEB_DIR, result.filePath)] = messages.length; + for (const message of messages) { + if (message.messageId === "waitForTimeout") raw.waitForTimeout += 1; + else raw.promiseSleep += 1; + } +} +const rawTotal = raw.waitForTimeout + raw.promiseSleep; + +if (process.argv.includes("--json")) { + console.log(JSON.stringify({ raw: { ...raw, total: rawTotal }, dwell }, null, 2)); + process.exit(0); +} + +const pad = (label) => label.padEnd(22); +console.log("\nE2E wall-clock wait inventory\n"); + +console.log(` UNSANCTIONED — drive to zero, then graduate the rule to "error"`); +console.log(` ${pad("waitForTimeout")}${raw.waitForTimeout}`); +console.log(` ${pad("promise sleep")}${raw.promiseSleep}`); +console.log(` ${pad("total")}${rawTotal}`); + +console.log(`\n DWELL DEBT — categorised, ${dwell.debt} of ${dwell.totalDwells} are debt`); +for (const category of categories) { + if (category === "negative-assertion") continue; + const count = dwell.byCategory[category] ?? 0; + const note = category === "unverified" ? " <- attack this first" : ""; + console.log(` ${pad(category)}${count}${note}`); +} +if (dwell.unknown > 0) { + console.log(` ${pad("uncategorised")}${dwell.unknown} <- miscategorised or unparsed`); +} + +console.log(`\n PERMANENT — not debt, do not drive down`); +console.log(` ${pad("negative-assertion")}${dwell.permanent}`); + +console.log(`\n DELIBERATE STIMULUS — not a wait, counted apart from sleep debt`); +console.log(` ${pad("injectLatency")}${dwell.injectLatency}`); + +if (rawTotal > 0) { + const worst = Object.entries(raw.byFile) + .sort(([, a], [, b]) => b - a) + .slice(0, 10); + console.log(`\n Top files by remaining raw sleeps (${Object.keys(raw.byFile).length} total):`); + for (const [file, count] of worst) console.log(` ${String(count).padStart(3)} ${file}`); +} +console.log("");