test(e2e): graduate the sleep guard to all of e2e now the conversion is done (#2653)

Co-authored-by: Kandev Agent <agent@kandev.dev>
This commit is contained in:
João Salavisa
2026-08-18 00:14:14 +00:00
committed by GitHub
parent 4cccb9861c
commit 66cadc5963
6 changed files with 194 additions and 87 deletions
+1 -1
View File
@@ -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.
+23 -15
View File
@@ -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:
+48 -34
View File
@@ -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",
];
/**
+10 -8
View File
@@ -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.
+18 -12
View File
@@ -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 <path>`, the work-list generator for whoever is
* converting a directory next.
* - `pnpm run lint:e2e-sleeps <path>`, 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" },
},
]);
+94 -17
View File
@@ -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,
);
});