From ed05772a8e25f9a059c2222965c512439c022eed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 27 Sep 2026 04:48:08 +0200 Subject: [PATCH] test(e2e): run the delegated-writes journey and fail on silent skips The delegated-writes spec checked for the invitation form's level control right after page load, before the form had painted, found nothing and skipped every test on every run, CI included. - Give the journey its own account pair, seeded and reset in global setup, so it no longer invites the same pair as the account-sharing journey (a second live grant between two accounts is refused with 409). - Wait for the form through its data-slot and fail when the WRITE level control is missing instead of skipping. - Revoke the journey's grant in afterAll. - Keep it out of the mobile project in the config rather than through a runtime skip. Add two checks so a skip cannot hide a spec again: - e2e-conditional-skip-guard.test.ts requires every conditional test.skip/test.fixme in e2e/ to carry a reason and refuses a bare test.skip(). - scripts/check-e2e-skipped-specs.mjs reads the Playwright JSON reports of both shards in the e2e gate job and fails when a spec file skipped all of its tests across every project, apart from an allowlist with reasons. --- .github/workflows/e2e.yml | 27 +++ .gitignore | 1 + e2e/delegated-writes.spec.ts | 157 ++++++++------ e2e/setup/global-setup.ts | 104 +++++++++- e2e/setup/test-helpers.ts | 5 + playwright.config.ts | 26 ++- .../__tests__/check-e2e-skipped-specs.test.ts | 192 ++++++++++++++++++ scripts/__tests__/ci-determinism.test.ts | 6 +- scripts/check-e2e-skipped-specs.mjs | 119 +++++++++++ .../e2e-conditional-skip-guard.test.ts | 150 ++++++++++++++ 10 files changed, 719 insertions(+), 68 deletions(-) create mode 100644 scripts/__tests__/check-e2e-skipped-specs.test.ts create mode 100644 scripts/check-e2e-skipped-specs.mjs create mode 100644 src/__tests__/e2e-conditional-skip-guard.test.ts diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 2d058b109..4b0e8a28e 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -110,6 +110,17 @@ jobs: retention-days: 7 if-no-files-found: ignore + # The machine-readable report, for the skipped-spec check in the gate + # job below. Only uploaded when the shard ran to completion, so a missing + # artifact there means a shard that did not finish, not an empty suite. + - name: Upload Playwright JSON report + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: playwright-json-shard-${{ matrix.shard }} + path: playwright-json/results.json + retention-days: 7 + if-no-files-found: error + # Branch protection requires a check named exactly "e2e". This job carries # that name and answers for both shards: it fails when any shard failed or # was cancelled, so a half-green matrix cannot satisfy the gate. @@ -125,3 +136,19 @@ jobs: echo "shard result: ${{ needs.shard.result }}" exit 1 fi + + # A spec file that skipped every one of its tests reports green. The + # check aggregates both shards' reports, because Playwright splits a + # file's tests across shards; see the script's header for the allowlist. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + sparse-checkout: scripts + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: 22 + - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + pattern: playwright-json-shard-* + path: playwright-json + - name: No spec file skipped all of its tests + run: node scripts/check-e2e-skipped-specs.mjs playwright-json/*/results.json diff --git a/.gitignore b/.gitignore index f446e2043..9cd46eebf 100644 --- a/.gitignore +++ b/.gitignore @@ -17,6 +17,7 @@ # playwright e2e /test-results /playwright-report +/playwright-json /playwright/.cache /e2e/setup/storageState*.json diff --git a/e2e/delegated-writes.spec.ts b/e2e/delegated-writes.spec.ts index de3bffcd4..68150fffc 100644 --- a/e2e/delegated-writes.spec.ts +++ b/e2e/delegated-writes.spec.ts @@ -7,24 +7,24 @@ * other half — a real form, a real POST, a real row in somebody else's record, * and the owner seeing it afterwards. * - * ## Why it gates itself instead of seeding a WRITE grant directly + * ## Why it no longer gates itself * - * The grant level is chosen in the invitation form. Rather than mint a WRITE - * row behind the UI's back — which would prove the journey works for a grant - * no person can create — the journey looks for the level control and stands - * down when it is not there yet. + * It used to look for the invitation form's level control right after + * `goto()` and skip every test when it found none. The count ran before the + * form had painted, so it found none on every run, CI included, and the whole + * journey skipped for as long as that guard stood. A quiet skip and a passing + * suite look identical in a CI summary. The form is now awaited through its + * own `data-slot`, and a missing level control fails the first test instead of + * standing the file down. `e2e-conditional-skip-guard.test.ts` and + * `scripts/check-e2e-skipped-specs.mjs` hold that line for every spec. * - * That guard was written against a `data-slot` the form never shipped under - * (`grant-invite-level`; the control landed as `grant-invite-access-option`), - * so from the day the control arrived until 2026-08-03 every test in this file - * skipped and the whole delegated-write journey ran nowhere. The file said out - * loud what to change and nobody changed it, which is the standing lesson - * about a check that cannot fail: the skip is quiet, and a quiet skip and a - * passing suite look identical in a CI summary. + * ## Its own pair * - * The constant is the real one now. If it moves again, change that one string: - * nothing else here assumes anything about the control except that choosing - * WRITE and submitting mints a WRITE grant. + * The journey invites `E2E_WRITE_DELEGATE` from `E2E_WRITE_OWNER`, accounts no + * other spec uses. Only one live grant can stand between two accounts, so + * sharing a pair with the account-sharing journey made whichever file invited + * second meet a 409. The grant is revoked in `afterAll` so a local re-run + * starts clean even before global setup resets the pair. * * ## What this spec cannot cover * @@ -39,17 +39,14 @@ import type { BrowserContext, Page } from "@playwright/test"; import { actWithReproof, useStaleSession } from "./setup/recent-proof"; import { expect, test } from "./setup/test"; import { - DELEGATE_STORAGE_STATE_PATH, - E2E_OWNER, - E2E_OWNER_FULL_NAME, - E2E_USER, - OWNER_STORAGE_STATE_PATH, + E2E_WRITE_DELEGATE, + E2E_WRITE_OWNER, + E2E_WRITE_OWNER_FULL_NAME, + WRITE_DELEGATE_STORAGE_STATE_PATH, + WRITE_OWNER_STORAGE_STATE_PATH, } from "./setup/test-helpers"; -/** - * The invitation form's grant-level control. The journey runs when this is on - * the page and stands down when it is not. One string, one place. - */ +/** The invitation form's grant-level control. One string, one place. */ const GRANT_LEVEL_SLOT = "grant-invite-access-option"; /** The value the level control carries for a grant that may add entries. */ @@ -58,55 +55,89 @@ const WRITE_LEVEL_VALUE = "WRITE"; test.describe("delegated writes", () => { // One journey in order, like the read-only sibling: each step is the next // one's precondition, and the invitation endpoint is rate-limited. + // Desktop only: `playwright.config.ts` keeps it out of the mobile project, + // because a second project would drive the same pair in parallel. test.describe.configure({ mode: "serial" }); - // The `page` fixture is the DELEGATE throughout, on this journey's own - // cookie jar — the switch is stamped on the session row, so switching a - // shared jar would switch it for every spec holding the same cookie. - test.use({ storageState: DELEGATE_STORAGE_STATE_PATH }); - - // Desktop only: the journey mutates shared rows between two seeded accounts, - // so two projects running it at once race each other and the second - // invitation is refused as a duplicate. - test.beforeEach(({}, testInfo) => { - test.skip( - testInfo.project.name !== "chromium-desktop", - "shared-fixture journey — runs once", - ); - }); + // The `page` fixture is the DELEGATE throughout, on its own account's jar. + test.use({ storageState: WRITE_DELEGATE_STORAGE_STATE_PATH }); let ownerContext: BrowserContext; let ownerPage: Page; - let levelControlPresent = false; let endOwnerSession: (() => Promise) | null = null; test.beforeAll(async ({ browser }) => { ownerContext = await browser.newContext({ - storageState: OWNER_STORAGE_STATE_PATH, + storageState: WRITE_OWNER_STORAGE_STATE_PATH, }); ownerPage = await ownerContext.newPage(); // The invitation asks for a recent proof; see `useStaleSession`. - endOwnerSession = await useStaleSession(ownerPage, E2E_OWNER.username); - await ownerPage.goto("/settings/access"); - levelControlPresent = - (await ownerPage.locator(`[data-slot="${GRANT_LEVEL_SLOT}"]`).count()) > - 0; - }); - - test.afterAll(async () => { - await endOwnerSession?.(); - await ownerContext.close(); - }); - - test.beforeEach(() => { - test.skip( - !levelControlPresent, - `no [data-slot="${GRANT_LEVEL_SLOT}"] in the invitation form — a WRITE grant cannot be created through the UI yet`, + endOwnerSession = await useStaleSession( + ownerPage, + E2E_WRITE_OWNER.username, ); }); - test("the owner invites at a level that may add entries", async () => { + test.afterAll(async () => { + // End every live grant this journey minted, through the owner's own + // session. Revocation needs no re-proof (reducing access is never gated), + // and it clears the delegate's switch stamp in the same transaction. + try { + // On the app's origin, whatever step the journey stopped at. + await ownerPage.goto("/settings/access"); + const revoked = await ownerPage.evaluate(async (grantee: string) => { + const res = await fetch("/api/account/grants"); + if (!res.ok) return -res.status; + const body = (await res.json()) as { + data: { + given: Array<{ + id: string; + state: string; + account: { username: string }; + }>; + }; + }; + let count = 0; + for (const grant of body.data.given) { + if (grant.account.username !== grantee) continue; + if (grant.state !== "ACTIVE" && grant.state !== "PENDING") continue; + const del = await fetch(`/api/account/grants/${grant.id}`, { + method: "DELETE", + }); + if (!del.ok) return -del.status; + count += 1; + } + return count; + }, E2E_WRITE_DELEGATE.username); + expect(revoked, "the journey's grant was revoked").toBeGreaterThanOrEqual( + 0, + ); + } finally { + await endOwnerSession?.(); + await ownerContext.close(); + } + }); + + /** + * The invitation form, painted. The card's `data-slot` is on the server + * render; the level control inside it is what the journey needs, so wait for + * that and fail loudly when it is gone rather than standing the file down. + */ + async function openInviteForm(): Promise { await ownerPage.goto("/settings/access"); + await expect( + ownerPage.locator('[data-slot="grant-invite-card"]'), + ).toBeVisible(); + await expect( + ownerPage.locator( + `[data-slot="${GRANT_LEVEL_SLOT}"][data-access="${WRITE_LEVEL_VALUE}"]`, + ), + "the invitation form offers a level that may add entries", + ).toBeVisible(); + } + + test("the owner invites at a level that may add entries", async () => { + await openInviteForm(); const identifier = ownerPage.locator( '[data-slot="grant-invite-identifier"]', @@ -124,7 +155,7 @@ test.describe("delegated writes", () => { // The controlled input keeps the submit disabled until React has attached, // so retry the pair rather than waiting a fixed time and hoping. await expect(async () => { - await identifier.fill(E2E_USER.username); + await identifier.fill(E2E_WRITE_DELEGATE.username); await expect(submit).toBeEnabled({ timeout: 1000 }); }).toPass({ timeout: 15_000 }); @@ -140,7 +171,9 @@ test.describe("delegated writes", () => { (req) => req.method() === "POST" && req.url().endsWith("/api/account/grants"), ); - await actWithReproof(ownerPage, E2E_OWNER.password, () => submit.click()); + await actWithReproof(ownerPage, E2E_WRITE_OWNER.password, () => + submit.click(), + ); const posted = JSON.parse((await invitePost).postData() ?? "{}") as { access?: string; }; @@ -164,8 +197,8 @@ test.describe("delegated writes", () => { const banner = page.locator('[data-slot="shared-record-banner"]'); await expect(banner).toBeVisible(); - // v1.37.2 — the owner set a full name, so the banner names them by it. - await expect(banner).toContainText(E2E_OWNER_FULL_NAME); + // The owner set a full name, so the banner names them by it. + await expect(banner).toContainText(E2E_WRITE_OWNER_FULL_NAME); // The banner drops its read-only clause on its own once the level says so. await expect(banner).not.toContainText("read it, not change it"); }); @@ -248,6 +281,6 @@ test.describe("delegated writes", () => { await ownerPage.goto("/settings/access"); const rows = ownerPage.locator('[data-slot="record-activity-row"]'); await expect(rows.first()).toBeVisible(); - await expect(rows.first()).toContainText(E2E_USER.username); + await expect(rows.first()).toContainText(E2E_WRITE_DELEGATE.username); }); }); diff --git a/e2e/setup/global-setup.ts b/e2e/setup/global-setup.ts index bfd62f548..550980d24 100644 --- a/e2e/setup/global-setup.ts +++ b/e2e/setup/global-setup.ts @@ -104,6 +104,32 @@ export const E2E_BACKUP_DELEGATE = { role: "USER", } as const; +/** + * The delegated-writes journey's own pair. + * + * That journey used to invite `E2E_USER` from `E2E_OWNER`, the same pair the + * account-sharing journey invites. Only one live grant can stand between two + * accounts, so whichever of the two files reached its invitation second met a + * 409, and on CI the two can run in parallel. A pair of its own removes the + * collision instead of ordering around it. + */ +export const E2E_WRITE_OWNER = { + email: "e2e-write-owner@healthlog.test", + username: "e2e-write-owner", + password: "Rk5!Wq8tHn3zPv6D", + role: "USER", +} as const; + +/** Generic on purpose, like `E2E_OWNER_FULL_NAME`: fixtures hold no real name. */ +export const E2E_WRITE_OWNER_FULL_NAME = "Test Write Owner"; + +export const E2E_WRITE_DELEGATE = { + email: "e2e-write-delegate@healthlog.test", + username: "e2e-write-delegate", + password: "Tm4!Xc9vBs2kLw7J", + role: "USER", +} as const; + /** * v1.37.0 — the account that creates and administers managed profiles. * @@ -404,6 +430,17 @@ export const CROSS_TAB_STORAGE_STATE_PATH = resolve( "e2e/setup/storageStateCrossTab.json", ); +/** The delegated-writes journey's jars, one per side of its own pair. */ +export const WRITE_OWNER_STORAGE_STATE_PATH = resolve( + process.cwd(), + "e2e/setup/storageStateWriteOwner.json", +); + +export const WRITE_DELEGATE_STORAGE_STATE_PATH = resolve( + process.cwd(), + "e2e/setup/storageStateWriteDelegate.json", +); + /** The backup/restore journey's jar, for the account it owns outright. */ export const BACKUP_STORAGE_STATE_PATH = resolve( process.cwd(), @@ -1125,7 +1162,7 @@ export default async function globalSetup(config: FullConfig): Promise { WHERE key LIKE 'sharing:%'`, ); - // The login bucket, for the same reason. This setup signs in SEVENTEEN times now + // The login bucket, for the same reason. This setup signs in more than twenty times // (the shared jar, the owner, and one jar apiece for every spec that moves // a session's record selector), and the ceiling is five attempts per IP per // quarter-hour — so two local runs in a row would otherwise end with a 429 @@ -1185,6 +1222,65 @@ export default async function globalSetup(config: FullConfig): Promise { ["e2e-level-write"], ); + // ── the delegated-writes journey's pair ────────────────────────────── + // + // Seeded like the sharing pair above and reset the same way: no grant + // between them, no switch stamp on either, so the journey always starts + // from the invitation. The spec revokes its grant when it ends; the delete + // here covers a run that died before getting there. + for (const [account, fullName] of [ + [E2E_WRITE_OWNER, E2E_WRITE_OWNER_FULL_NAME], + [E2E_WRITE_DELEGATE, null], + ] as const) { + await pool.query( + `INSERT INTO users + (id, username, email, password_hash, role, + created_at, updated_at, + onboarding_completed_at, onboarding_tour_completed, full_name) + VALUES ($1, $2, $3, $4, $5, $6, $6, $6, true, $7) + ON CONFLICT (username) DO UPDATE SET + email = EXCLUDED.email, + password_hash = EXCLUDED.password_hash, + role = EXCLUDED.role, + updated_at = EXCLUDED.updated_at, + onboarding_completed_at = EXCLUDED.onboarding_completed_at, + onboarding_tour_completed = EXCLUDED.onboarding_tour_completed, + full_name = EXCLUDED.full_name, + totp_confirmed_at = NULL`, + [ + cuid(), + account.username, + account.email, + await hashPassword(account.password), + account.role, + now, + fullName, + ], + ); + } + await pool.query( + `DELETE FROM account_grants + WHERE grantor_id IN (SELECT id FROM users WHERE username = ANY($1)) + OR grantee_id IN (SELECT id FROM users WHERE username = ANY($1))`, + [[E2E_WRITE_OWNER.username, E2E_WRITE_DELEGATE.username]], + ); + await pool.query( + `UPDATE sessions SET acting_as_user_id = NULL + WHERE user_id IN (SELECT id FROM users WHERE username = ANY($1))`, + [[E2E_WRITE_OWNER.username, E2E_WRITE_DELEGATE.username]], + ); + // The reading the delegate saves into the owner's record carries a + // minute-rounded `measuredAt`, so a second run inside the same minute would + // meet the natural-key 409 — the same reason `e2e-level-write` is cleared + // above. + await pool.query( + `DELETE FROM measurements + WHERE user_id = (SELECT id FROM users WHERE username = $1) + AND source = 'MANUAL' + AND external_id IS NULL`, + [E2E_WRITE_OWNER.username], + ); + // ── the backup/restore journey's pair ──────────────────────────────── // // The account the journey owns, with the three modules its readings need @@ -1308,7 +1404,7 @@ export default async function globalSetup(config: FullConfig): Promise { * Log one account in, clearing the login bucket first. * * The ceiling is FIVE attempts per IP per quarter-hour and this setup now - * signs in eighteen times, so clearing once before the batch is no longer + * signs in more than twenty times, so clearing once before the batch is no longer * enough — the sixth would be answered by the fixture's own 429 rather * than by the product. Only the auth surfaces' buckets are touched, and * only between logins this setup is itself performing. @@ -1366,6 +1462,10 @@ export default async function globalSetup(config: FullConfig): Promise { // E2E_REPORT_OWNER: generating a report writes to the account row. await capture(E2E_REPORT_OWNER, REPORT_OWNER_STORAGE_STATE_PATH); + // The delegated-writes journey's pair. + await capture(E2E_WRITE_OWNER, WRITE_OWNER_STORAGE_STATE_PATH); + await capture(E2E_WRITE_DELEGATE, WRITE_DELEGATE_STORAGE_STATE_PATH); + // The backup/restore journey's pair. await capture(E2E_BACKUP_ADMIN, BACKUP_STORAGE_STATE_PATH); await capture(E2E_BACKUP_DELEGATE, BACKUP_DELEGATE_STORAGE_STATE_PATH); diff --git a/e2e/setup/test-helpers.ts b/e2e/setup/test-helpers.ts index 6d9f4c65d..f0058b6cb 100644 --- a/e2e/setup/test-helpers.ts +++ b/e2e/setup/test-helpers.ts @@ -28,4 +28,9 @@ export { REPORT_DELEGATE_STORAGE_STATE_PATH, REPORT_OWNER_STORAGE_STATE_PATH, GUARDIAN_STORAGE_STATE_PATH, + E2E_WRITE_OWNER, + E2E_WRITE_OWNER_FULL_NAME, + E2E_WRITE_DELEGATE, + WRITE_OWNER_STORAGE_STATE_PATH, + WRITE_DELEGATE_STORAGE_STATE_PATH, } from "./global-setup"; diff --git a/playwright.config.ts b/playwright.config.ts index 626d35b11..bf46cf31d 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -53,9 +53,25 @@ export default defineConfig({ // timeout is lifted from 5s → 10s for the same settle headroom. retries: process.env.CI ? 2 : 0, workers: process.env.CI ? 2 : undefined, - reporter: process.env.CI - ? [["github"], ["html", { open: "never" }]] - : [["list"], ["html", { open: "never" }]], + // The JSON report feeds `scripts/check-e2e-skipped-specs.mjs`, which the + // e2e workflow's gate job runs over every shard's report to fail a spec file + // that skipped all of its tests. Set E2E_JSON_REPORT to get one locally. + reporter: [ + ...(process.env.CI + ? ([["github"], ["html", { open: "never" }]] as const) + : ([["list"], ["html", { open: "never" }]] as const)), + ...(process.env.CI || process.env.E2E_JSON_REPORT + ? ([ + [ + "json", + { + outputFile: + process.env.E2E_JSON_REPORT ?? "playwright-json/results.json", + }, + ], + ] as const) + : []), + ], use: { baseURL: process.env.E2E_BASE_URL ?? "http://localhost:3000", @@ -241,6 +257,10 @@ export default defineConfig({ // a second project would hold and clear that state underneath the // first — and underneath any other spec that reads AI availability. "onboarding-ai-offer.spec.ts", + // The delegated-writes journey invites, accepts and revokes between + // its own two accounts. A second project would send a second invitation + // between the same pair while the first is live, and meet the 409. + "delegated-writes.spec.ts", // The invitation journey mints its token and registers from a // cookie-less window. Two projects registering from one address // share the five-per-fifteen-minutes budget on `auth:register`, so diff --git a/scripts/__tests__/check-e2e-skipped-specs.test.ts b/scripts/__tests__/check-e2e-skipped-specs.test.ts new file mode 100644 index 000000000..8f248574c --- /dev/null +++ b/scripts/__tests__/check-e2e-skipped-specs.test.ts @@ -0,0 +1,192 @@ +/** + * `scripts/check-e2e-skipped-specs.mjs`: the CI half of "a spec file that + * skips everything must say so". The logic is exercised on hand-built reports + * in Playwright's JSON shape, the CLI is run once end to end, and the workflow + * wiring is pinned so the check cannot be dropped from the gate job silently. + */ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, describe, expect, it } from "vitest"; +import { parse } from "yaml"; + +import { + ALLOWED_ALL_SKIPPED, + findProblems, + tallySpecFiles, +} from "../check-e2e-skipped-specs.mjs"; + +const ROOT = join(__dirname, "../.."); +const SCRIPT = join(ROOT, "scripts/check-e2e-skipped-specs.mjs"); + +type Status = "expected" | "unexpected" | "flaky" | "skipped"; + +/** One report in Playwright's JSON shape: file suite → describe → spec → tests. */ +function report( + entries: Array<{ file: string; tests: Array<[string, Status]> }>, +) { + return { + suites: entries.map(({ file, tests }) => ({ + title: file, + file, + specs: [], + suites: [ + { + title: "describe", + file, + specs: tests.map(([projectName, status], i) => ({ + title: `t${i}`, + file, + tests: [{ projectName, status }], + })), + }, + ], + })), + }; +} + +describe("check-e2e-skipped-specs", () => { + it("flags a file whose every test skipped", () => { + const problems = findProblems( + [ + report([ + { file: "a.spec.ts", tests: [["chromium-desktop", "expected"]] }, + { + file: "b.spec.ts", + tests: [ + ["chromium-desktop", "skipped"], + ["chromium-mobile", "skipped"], + ], + }, + ]), + ], + {}, + ); + expect(problems).toEqual([ + "b.spec.ts: all 2 test(s) skipped in every project and shard", + ]); + }); + + it("aggregates across projects and shards before judging a file", () => { + // The desktop half skipped in shard 1, the mobile half ran in shard 2. + const reports = [ + report([{ file: "m.spec.ts", tests: [["chromium-desktop", "skipped"]] }]), + report([{ file: "m.spec.ts", tests: [["chromium-mobile", "expected"]] }]), + ]; + expect(tallySpecFiles(reports).get("m.spec.ts")).toEqual({ + total: 2, + ran: 1, + }); + expect(findProblems(reports, {})).toEqual([]); + }); + + it("counts a failed or flaky test as having run", () => { + const reports = [ + report([ + { file: "f.spec.ts", tests: [["chromium-desktop", "unexpected"]] }, + { file: "g.spec.ts", tests: [["chromium-desktop", "flaky"]] }, + ]), + ]; + expect(findProblems(reports, {})).toEqual([]); + }); + + it("accepts an allowlisted file and rejects a stale allowlist entry", () => { + const reports = [ + report([ + { file: "a.spec.ts", tests: [["chromium-desktop", "skipped"]] }, + { file: "b.spec.ts", tests: [["chromium-desktop", "expected"]] }, + ]), + ]; + expect(findProblems(reports, { "a.spec.ts": "reason" })).toEqual([]); + expect(findProblems(reports, { "b.spec.ts": "reason" })).toEqual([ + "a.spec.ts: all 1 test(s) skipped in every project and shard", + "b.spec.ts: allowlisted but 1 test(s) ran", + ]); + expect( + findProblems(reports, { + "a.spec.ts": "reason", + "gone.spec.ts": "reason", + }), + ).toEqual(["gone.spec.ts: allowlisted but absent from the report"]); + }); + + it("refuses an empty report rather than passing over nothing", () => { + expect(findProblems([{ suites: [] }], {})).toEqual([ + "the report(s) hold no tests at all", + ]); + }); + + it("gives every allowlist entry a written reason", () => { + for (const [file, reason] of Object.entries(ALLOWED_ALL_SKIPPED)) { + expect(file).toMatch(/\.spec\.ts$/); + expect(reason.trim().length, file).toBeGreaterThan(10); + } + }); + + describe("the command line", () => { + const dir = mkdtempSync(join(tmpdir(), "e2e-skip-check-")); + afterAll(() => rmSync(dir, { recursive: true, force: true })); + + function run(payload: unknown): number { + const path = join(dir, `r${Math.random()}.json`); + writeFileSync(path, JSON.stringify(payload)); + try { + execFileSync("node", [SCRIPT, path], { stdio: "pipe" }); + return 0; + } catch (err) { + return (err as { status: number }).status; + } + } + + it("exits 0 on a clean report and 1 on an all-skipped file", () => { + expect( + run( + report([ + { file: "a.spec.ts", tests: [["chromium-desktop", "expected"]] }, + ]), + ), + ).toBe(0); + expect( + run( + report([ + { file: "z.spec.ts", tests: [["chromium-desktop", "skipped"]] }, + ]), + ), + ).toBe(1); + }); + }); +}); + +describe("the e2e workflow runs the check", () => { + type Step = { uses?: string; run?: string; with?: Record }; + const workflow = parse( + readFileSync(join(ROOT, ".github/workflows/e2e.yml"), "utf8"), + ) as { jobs: Record }; + + it("each shard uploads its JSON report", () => { + const upload = workflow.jobs.shard?.steps?.find( + (step) => + step.uses?.startsWith("actions/upload-artifact@") && + step.with?.name === "playwright-json-shard-${{ matrix.shard }}", + ); + expect(upload?.with).toMatchObject({ + path: "playwright-json/results.json", + "if-no-files-found": "error", + }); + }); + + it("the gate job downloads every shard's report and runs the script", () => { + const steps = workflow.jobs.e2e?.steps ?? []; + const download = steps.find((step) => + step.uses?.startsWith("actions/download-artifact@"), + ); + expect(download?.with).toMatchObject({ + pattern: "playwright-json-shard-*", + path: "playwright-json", + }); + expect(steps.map((step) => step.run ?? "")).toContain( + "node scripts/check-e2e-skipped-specs.mjs playwright-json/*/results.json", + ); + }); +}); diff --git a/scripts/__tests__/ci-determinism.test.ts b/scripts/__tests__/ci-determinism.test.ts index c9708f90a..f9254aaa0 100644 --- a/scripts/__tests__/ci-determinism.test.ts +++ b/scripts/__tests__/ci-determinism.test.ts @@ -44,7 +44,11 @@ describe("Playwright CI determinism", () => { expect(config.retries).toBe(2); expect(config.failOnFlakyTests).toBe(true); - expect(config.reporter).toEqual([["github"], ["html", { open: "never" }]]); + expect(config.reporter).toEqual([ + ["github"], + ["html", { open: "never" }], + ["json", { outputFile: "playwright-json/results.json" }], + ]); // Three servers, and which is which matters. The suite's own server runs // with the dashboard's RSC prefetch OFF so `page.route` fixtures govern // what the dashboard paints; the second runs it ON, which is the default diff --git a/scripts/check-e2e-skipped-specs.mjs b/scripts/check-e2e-skipped-specs.mjs new file mode 100644 index 000000000..57aa49ec9 --- /dev/null +++ b/scripts/check-e2e-skipped-specs.mjs @@ -0,0 +1,119 @@ +#!/usr/bin/env node +/** + * Fail CI when a whole e2e spec file ran nothing. + * + * A spec that skips all of its tests reports green, and in a summary a quiet + * skip looks exactly like a pass. `delegated-writes.spec.ts` skipped every + * test on every run for months that way: its skip condition was evaluated + * before the form it looked for had painted. + * + * This reads Playwright's JSON report (one per shard; the e2e workflow's gate + * job passes all of them) and counts, per spec FILE, the tests that ran across + * every project and shard together. A file is only reported when none of its + * tests ran anywhere, so a mobile-only spec that skips on the desktop project + * is fine as long as the mobile project ran it. Aggregating across shards is + * the reason this runs in the gate job and not inside each shard: a shard can + * receive only the skipped half of a file. + * + * A file that is meant to skip entirely goes in `ALLOWED_ALL_SKIPPED` below + * with the reason written next to it. The per-call half of the rule, that + * every conditional skip carries a reason, is the unit guard + * `src/__tests__/e2e-conditional-skip-guard.test.ts`. + * + * Usage: node scripts/check-e2e-skipped-specs.mjs [...] + */ +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +/** + * Spec files allowed to report every test skipped, keyed by the path the JSON + * report prints (relative to `testDir`), each with the reason it is allowed. + * + * @type {Record} + */ +export const ALLOWED_ALL_SKIPPED = {}; + +/** + * Walk a report's suites and tally each spec file's tests. + * + * @param {Array<{ suites?: unknown[] }>} reports parsed Playwright JSON reports + * @returns {Map} + */ +export function tallySpecFiles(reports) { + /** @type {Map} */ + const files = new Map(); + const visit = (suite) => { + for (const spec of suite.specs ?? []) { + const entry = files.get(spec.file) ?? { total: 0, ran: 0 }; + for (const test of spec.tests ?? []) { + entry.total += 1; + if (test.status !== "skipped") entry.ran += 1; + } + files.set(spec.file, entry); + } + for (const child of suite.suites ?? []) visit(child); + }; + for (const report of reports) { + for (const suite of report.suites ?? []) visit(suite); + } + return files; +} + +/** + * The problems to report: files whose every test skipped and that are not + * allowlisted, allowlist entries that no longer apply, and an empty report. + * + * @param {Array<{ suites?: unknown[] }>} reports + * @param {Record} [allowed] + * @returns {string[]} + */ +export function findProblems(reports, allowed = ALLOWED_ALL_SKIPPED) { + const files = tallySpecFiles(reports); + const problems = []; + let total = 0; + for (const entry of files.values()) total += entry.total; + if (total === 0) { + // A check over nothing passes for the wrong reason; say so instead. + problems.push("the report(s) hold no tests at all"); + return problems; + } + for (const [file, { total: count, ran }] of [...files].sort()) { + if (ran > 0) continue; + if (Object.hasOwn(allowed, file)) continue; + problems.push( + `${file}: all ${count} test(s) skipped in every project and shard`, + ); + } + for (const file of Object.keys(allowed)) { + const entry = files.get(file); + if (!entry) { + problems.push(`${file}: allowlisted but absent from the report`); + } else if (entry.ran > 0) { + problems.push(`${file}: allowlisted but ${entry.ran} test(s) ran`); + } + } + return problems; +} + +function main(paths) { + if (paths.length === 0) { + console.error("usage: check-e2e-skipped-specs.mjs [...]"); + return 2; + } + const reports = paths.map((p) => JSON.parse(readFileSync(p, "utf8"))); + const problems = findProblems(reports); + const files = tallySpecFiles(reports); + if (problems.length > 0) { + console.error("e2e spec files that ran nothing:"); + for (const problem of problems) console.error(` - ${problem}`); + return 1; + } + console.log( + `check-e2e-skipped-specs: ${files.size} spec file(s) across ${paths.length} report(s), each ran at least one test`, + ); + return 0; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + process.exit(main(process.argv.slice(2))); +} diff --git a/src/__tests__/e2e-conditional-skip-guard.test.ts b/src/__tests__/e2e-conditional-skip-guard.test.ts new file mode 100644 index 000000000..46541e6e9 --- /dev/null +++ b/src/__tests__/e2e-conditional-skip-guard.test.ts @@ -0,0 +1,150 @@ +/** + * Structural guard: every runtime skip in the e2e suite says why. + * + * A runtime `test.skip(condition, reason)` or `test.fixme(condition, reason)` + * turns a test off without failing anything, and a skipped test reads like a + * passing one in a CI summary. `delegated-writes.spec.ts` skipped every test on + * every run that way: its condition was evaluated before the form it checked + * for had painted. The reason string is what makes a skip reviewable, so every + * conditional call must carry one, and a bare `test.skip()` (which cannot) is + * refused outright. + * + * This is the per-call half. The per-file half, a spec whose every test + * skipped in CI, is `scripts/check-e2e-skipped-specs.mjs`, run by the e2e + * workflow's gate job over the Playwright JSON reports. + * + * The matcher reads the call's arguments with a small bracket-aware scanner + * rather than a regex, because conditions span lines and reasons are + * concatenated. Its limit: a skip reached through another name (an alias of + * `test`, `testInfo.skip`) is not seen. The non-empty assertion below keeps the + * matcher from going quietly green by matching nothing. + */ +import { globSync, readFileSync } from "node:fs"; +import { join, sep } from "node:path"; +import { describe, expect, it } from "vitest"; + +const E2E = join(process.cwd(), "e2e"); + +type SkipCall = { file: string; line: number; args: string[] }; + +function specFiles(): string[] { + return globSync("**/*.spec.ts", { cwd: E2E }) + .map((p) => p.split(sep).join("/")) + .sort(); +} + +/** Blank out comments, keeping offsets and newlines so line numbers hold. */ +function stripComments(src: string): string { + return src + .replace(/\/\*[\s\S]*?\*\//g, (m) => m.replace(/[^\n]/g, " ")) + .replace( + /(^|[^:"'`])\/\/.*$/gm, + (m, lead: string) => lead + " ".repeat(m.length - lead.length), + ); +} + +/** + * The top-level arguments of the call whose `(` sits at `open`, as trimmed + * source text. Strings and template literals are skipped over whole, so a + * comma or bracket inside a reason does not split it. + */ +function callArgs(src: string, open: number): string[] { + const args: string[] = []; + let depth = 0; + let start = open + 1; + for (let i = open; i < src.length; i++) { + const c = src[i]; + if (c === '"' || c === "'" || c === "`") { + for (i++; i < src.length && src[i] !== c; i++) { + if (src[i] === "\\") i++; + } + continue; + } + if (c === "(" || c === "[" || c === "{") depth++; + else if (c === ")" || c === "]" || c === "}") { + depth--; + if (depth === 0) { + const last = src.slice(start, i).trim(); + if (last) args.push(last); + return args; + } + } else if (c === "," && depth === 1) { + args.push(src.slice(start, i).trim()); + start = i + 1; + } + } + throw new Error("unbalanced call"); +} + +function skipCalls(): SkipCall[] { + const calls: SkipCall[] = []; + for (const file of specFiles()) { + const src = stripComments(readFileSync(join(E2E, file), "utf8")); + for (const m of src.matchAll(/\btest\.(?:skip|fixme)\s*\(/g)) { + const open = m.index + m[0].length - 1; + calls.push({ + file, + line: src.slice(0, m.index).split("\n").length, + args: callArgs(src, open), + }); + } + } + return calls; +} + +/** A string literal, template literal, or a concatenation starting with one. */ +const REASON = /^(["'`])(?!\1)[\s\S]+/; + +/** + * The declaration form, `test.skip("title", () => …)`, marks a test as + * skipped at collection time rather than on a runtime condition; the first + * argument is its title, not a condition. + */ +function isDeclaration(call: SkipCall): boolean { + return /^["'`]/.test(call.args[0] ?? "") && call.args.length >= 2; +} + +describe("e2e runtime skips carry a reason", () => { + const calls = skipCalls(); + const conditional = calls.filter( + (c) => c.args.length > 0 && !isDeclaration(c), + ); + + it("finds the conditional skips at all", () => { + // The suite has many project-specific skips. If the matcher ever finds + // none, it has stopped reading the specs, not the specs stopped skipping. + expect(conditional.length).toBeGreaterThan(10); + }); + + it("every conditional skip names its reason", () => { + const offenders = conditional + .filter((c) => c.args.length < 2 || !REASON.test(c.args[1]!)) + .map((c) => `${c.file}:${c.line}`); + expect(offenders).toEqual([]); + }); + + it("no spec skips unconditionally without a reason", () => { + const bare = calls + .filter((c) => c.args.length === 0) + .map((c) => `${c.file}:${c.line}`); + expect(bare).toEqual([]); + }); + + it("the delegated-writes journey no longer skips itself", () => { + // It skipped every test on every run for as long as its guard stood; it + // now fails loudly when the form it needs is missing. + expect(calls.filter((c) => c.file === "delegated-writes.spec.ts")).toEqual( + [], + ); + }); + + it("the scanner reads a multi-line call with a concatenated reason", () => { + // The matcher's own positive control, on the shape the suite actually uses. + const src = `test.skip(\n a(b, c) !== "x, y",\n "one, " +\n "two",\n);`; + expect(callArgs(src, src.indexOf("("))).toEqual([ + 'a(b, c) !== "x, y"', + '"one, " +\n "two"', + ]); + expect(REASON.test('""')).toBe(false); + }); +});