diff --git a/e2e/mobile-density.spec.ts b/e2e/mobile-density.spec.ts index af0e0836d..00f13fbe9 100644 --- a/e2e/mobile-density.spec.ts +++ b/e2e/mobile-density.spec.ts @@ -1,6 +1,7 @@ import { expect, test } from "./setup/test"; import { STORAGE_STATE_PATH } from "./setup/global-setup"; +import { settleBeforeMeasure } from "./utils/settle"; import { LONG_HEADLINE_BRIEFING, MOCK_SCORE_RINGS, @@ -162,7 +163,16 @@ test.describe("phone-width density guard", () => { await mockMoodInsights(page); } await page.goto(route, { waitUntil: "domcontentloaded" }); - await page.waitForLoadState("networkidle"); + + if (route === "/insights") { + // The one route here that had no readiness signal, while its two + // siblings below both have one. An overflow guard against a page + // that has not finished rendering passes for the same reason an + // empty page passes: there is nothing wide on it yet. + await expect( + page.locator('[data-slot="section-heading"]').first(), + ).toBeVisible(); + } if (route === "/") { // The guard must not pass vacuously — the Today hero and its @@ -186,6 +196,8 @@ test.describe("phone-width density guard", () => { ).toBeVisible(); } + await settleBeforeMeasure(page, page.locator("main#main-content")); + // The document/body pair alone cannot see the app's sideways scroll: // `AuthShell` scrolls inside `
`, whose // `overflow-y:auto` makes `overflow-x` compute to `auto`, so an diff --git a/e2e/mobile-viewport.spec.ts b/e2e/mobile-viewport.spec.ts index 200bf4c28..1565fa86d 100644 --- a/e2e/mobile-viewport.spec.ts +++ b/e2e/mobile-viewport.spec.ts @@ -1,6 +1,7 @@ import { expect, test } from "./setup/test"; import { STORAGE_STATE_PATH } from "./setup/global-setup"; +import { settleBeforeMeasure } from "./utils/settle"; import { mockDashboardSnapshot, WEIGHT_ONLY_SUMMARIES, @@ -78,7 +79,10 @@ test.describe("mobile-viewport smoke", () => { ); await page.goto("/", { waitUntil: "domcontentloaded" }); - await page.waitForLoadState("networkidle"); + // Gate on the dashboard's own content, not on the network. Every read + // below is geometric and happens exactly once, so measuring a tree that + // is still mounting reports a layout nobody will ever see. + await settleBeforeMeasure(page, page.locator("main#main-content")); // 1) No horizontal scroll const dims = await page.evaluate(() => ({ diff --git a/e2e/settings-overscroll.spec.ts b/e2e/settings-overscroll.spec.ts index b55eb33cc..dc7ab9ec3 100644 --- a/e2e/settings-overscroll.spec.ts +++ b/e2e/settings-overscroll.spec.ts @@ -1,6 +1,7 @@ import { expect, test } from "./setup/test"; import { STORAGE_STATE_PATH } from "./setup/global-setup"; +import { settleBeforeMeasure } from "./utils/settle"; /** * Vertical over-scroll regression guard (the recurring class behind the @@ -63,9 +64,11 @@ test.describe("settings + admin vertical over-scroll guard", () => { test(`no over-scroll on ${route} (${vp.name})`, async ({ page }) => { await page.setViewportSize({ width: vp.width, height: vp.height }); await page.goto(route, { waitUntil: "domcontentloaded" }); - await page.waitForLoadState("networkidle"); - // Let entry transitions / late queries settle before measuring. - await page.waitForTimeout(500); + // Was a fixed 500 ms sleep, which is the wrong shape twice over: too + // long when the page is ready and too short on a loaded runner, and + // it never says what it is waiting for. Gate on the element the + // measurement reads instead. + await settleBeforeMeasure(page, page.locator("#main-content")); const dims = await page.evaluate(() => { const main = document.getElementById("main-content"); diff --git a/e2e/utils/settle.ts b/e2e/utils/settle.ts new file mode 100644 index 000000000..14480ab7b --- /dev/null +++ b/e2e/utils/settle.ts @@ -0,0 +1,39 @@ +import { expect, type Locator, type Page } from "@playwright/test"; + +/** + * Gate a one-shot DOM read on the element it is about to measure. + * + * The failure this exists to prevent: a spec navigates with + * `waitUntil: "networkidle"`, then reads the DOM in a single + * `page.evaluate` / `evaluateAll` that does not retry. An idle network says + * nothing about whether React has rendered, so the read can land on a tree + * that is still mounting and the assertion then blames the application for + * something the test raced — "language + dob fields must exist" when the + * fields were simply not there yet, or a measurement helper accused of + * measuring the wrong element when it was measuring the right one too early. + * + * Two such tests went red on consecutive CI runs during v1.33.0. Neither + * reproduces locally: a developer machine is fast enough that the render + * always wins the race, so the only place this shows up is a loaded runner, + * and `failOnFlakyTests` means every occurrence costs a whole red pipeline + * even when the retry passes. + * + * `sentinel` must be something the measurement actually depends on, not just + * any element on the page. Gating on a layout shell that renders before the + * data proves nothing. + */ +export async function settleBeforeMeasure( + page: Page, + sentinel: Locator, + options: { attachedOnly?: boolean } = {}, +): Promise { + if (options.attachedOnly) { + await expect(sentinel).toBeAttached(); + } else { + await expect(sentinel).toBeVisible(); + } + // Fonts and late CSS change geometry after the element is visible, and + // every measurement in this suite is geometric. + await page.waitForLoadState("networkidle").catch(() => {}); + await page.evaluate(() => document.fonts?.ready); +} diff --git a/src/__tests__/e2e-measurement-gating-guard.test.ts b/src/__tests__/e2e-measurement-gating-guard.test.ts new file mode 100644 index 000000000..6f4970f75 --- /dev/null +++ b/src/__tests__/e2e-measurement-gating-guard.test.ts @@ -0,0 +1,175 @@ +import { readdirSync, readFileSync } from "node:fs"; +import { join, resolve } from "node:path"; + +import { describe, expect, it } from "vitest"; + +/** + * A one-shot DOM read must be gated on the element it measures, not on the + * network. + * + * The failure this catches, twice on consecutive CI runs during v1.33.0: + * a spec navigates with `waitUntil: "networkidle"` and then reads the DOM in + * a single `page.evaluate` / `evaluateAll` that never retries. An idle + * network says nothing about whether React has rendered, so the read can land + * on a tree that is still mounting. The assertion then blames the application + * — "language + dob fields must exist", "the helper is measuring the wrong + * element" — for what is a race inside the test. + * + * Why this guard lives in the unit suite rather than in Playwright: e2e runs + * at PR-CI only, and `playwright.config.ts` sets `failOnFlakyTests` on CI, so + * one unguarded read costs a whole red pipeline before anyone sees it. Here it + * is red in seconds, on the machine that wrote it. + * + * It is deliberately a source-shape check rather than a runtime one. The + * shape is what a developer gets wrong; the runtime symptom only appears on a + * loaded runner, which is precisely where it is most expensive to discover. + */ + +const E2E_DIR = resolve(__dirname, "../../e2e"); + +/** + * Sites where a `goto` → `evaluate` with no locator gate is correct, each with + * the reason. Keep this list short: it is an inventory of exceptions, not a + * place to put anything inconvenient. Past ~15 entries the guard is asking the + * wrong question and should be rethought rather than extended. + */ +const ALLOWLIST: ReadonlyArray<{ file: string; test: string; why: string }> = [ + { + file: "ipad-viewport.spec.ts", + test: "no horizontal overflow and full-width content on", + why: "Deliberate swallowed waitForFunction. The rail settle can land just after networkidle on a loaded runner, but a genuine regression — the rail never engaging — still fails at the width assertion below, which reports the actual measured number. A hard gate here would replace that number with a generic timeout message, so the swallow buys a better diagnostic rather than hiding a race.", + }, +]; + +interface Offence { + file: string; + testName: string; + line: number; +} + +/** Source positions of every `test(` / `test.only(` block opener. */ +function testBlocks( + source: string, +): Array<{ name: string; start: number; end: number }> { + const blocks: Array<{ name: string; start: number; end: number }> = []; + const opener = /\btest(?:\.only|\.fixme|\.skip)?\s*\(\s*(`|"|')([\s\S]*?)\1/g; + const starts: Array<{ name: string; index: number }> = []; + for (let m = opener.exec(source); m !== null; m = opener.exec(source)) { + starts.push({ name: m[2], index: m.index }); + } + for (let i = 0; i < starts.length; i += 1) { + blocks.push({ + name: starts[i].name, + start: starts[i].index, + end: i + 1 < starts.length ? starts[i + 1].index : source.length, + }); + } + return blocks; +} + +const GOTO = /\bpage\.goto\s*\(/g; +/** A read that samples the DOM once and does not retry. */ +const ONE_SHOT_READ = + /\b(?:page|\w+)\.(?:evaluate|evaluateAll|evaluateHandle|\$\$eval|\$eval)\s*\(/; +/** + * Anything that waits for the DOM to reach a state before the read. + * + * No leading `\b`: the common form is `page.locator(…).first().waitFor(…)`, + * where the character before the dot is `)`. There is no word boundary + * between `)` and `.`, so a `\b` here silently fails to match the most + * frequent gate in the suite and reports correct tests as offenders. That is + * worse than no guard, because the fix someone reaches for is to allowlist a + * test that was never wrong. + */ +const GATE = + /(?:await\s+expect(?:\.poll|\.soft)?\s*\(|\.waitFor\s*\(|waitForSelector\s*\(|waitForFunction\s*\(|settleBeforeMeasure\s*\(|settleForOverflowMeasurement\s*\(|toBeVisible\s*\(|toBeAttached\s*\()/; + +function lineOf(source: string, index: number): number { + return source.slice(0, index).split("\n").length; +} + +/** + * A wait whose rejection is thrown away is not a gate. + * + * `await page.waitForFunction(...).catch(() => {})` reads like a guarantee and + * is not one: on timeout it proceeds to the measurement exactly as if it had + * never waited. Counting it would also leave the guard trivially defeatable — + * append `.catch(() => {})` to any wait and the check goes quiet. Where the + * swallow is deliberate, the allowlist is the place to say so. + */ +const SWALLOWED_WAIT = + /(?:waitForFunction|waitForSelector|waitFor)\s*\([\s\S]*?\)\s*\n?\s*\.catch\s*\(/; + +function stripSwallowedWaits(segment: string): string { + return segment.replace(new RegExp(SWALLOWED_WAIT, "g"), "«swallowed»"); +} + +function scan(file: string, source: string): Offence[] { + const offences: Offence[] = []; + for (const block of testBlocks(source)) { + const body = source.slice(block.start, block.end); + GOTO.lastIndex = 0; + for (let g = GOTO.exec(body); g !== null; g = GOTO.exec(body)) { + const after = body.slice(g.index); + const read = ONE_SHOT_READ.exec(after); + if (!read) continue; + const between = stripSwallowedWaits(after.slice(0, read.index)); + if (GATE.test(between)) continue; + offences.push({ + file, + testName: block.name, + line: lineOf(source, block.start + g.index), + }); + break; + } + } + return offences; +} + +describe("e2e specs gate their measurements on the element, not the network", () => { + const specs = readdirSync(E2E_DIR).filter((f) => f.endsWith(".spec.ts")); + + it("finds spec files to check (the guard must not pass vacuously)", () => { + expect(specs.length).toBeGreaterThan(10); + }); + + it("has no ungated goto → one-shot DOM read outside the allowlist", () => { + const offences: Offence[] = []; + for (const spec of specs) { + const source = readFileSync(join(E2E_DIR, spec), "utf8"); + offences.push(...scan(spec, source)); + } + + const unexplained = offences.filter( + (o) => + !ALLOWLIST.some( + (a) => a.file === o.file && o.testName.includes(a.test), + ), + ); + + if (unexplained.length > 0) { + throw new Error( + `${unexplained.length} e2e test(s) navigate and then read the DOM once, with nothing in between that waits for the element:\n\n` + + unexplained + .map((o) => ` ${o.file}:${o.line} — "${o.testName}"`) + .join("\n") + + "\n\nAn idle network does not mean React has rendered. Gate on the element the " + + "measurement is about (await expect(locator).toBeVisible(), or settleBeforeMeasure " + + "from e2e/utils/settle.ts). If the read genuinely does not need a gate, add it to " + + "ALLOWLIST in this file with the reason.", + ); + } + }); + + it("carries no allowlist entry for a spec file that no longer exists", () => { + for (const entry of ALLOWLIST) { + expect(specs, `stale allowlist entry: ${entry.file}`).toContain( + entry.file, + ); + } + }); + + it("keeps the allowlist small enough to still be an inventory of exceptions", () => { + expect(ALLOWLIST.length).toBeLessThanOrEqual(15); + }); +});