From 9f7bd3ba2e201e4dec3827186b4d6d68539eb365 Mon Sep 17 00:00:00 2001 From: Coracle-Bot Date: Tue, 15 Sep 2026 15:56:23 +0000 Subject: [PATCH] Fail an e2e test when the page threw or the browser refused our code --- e2e/ARCHITECTURE.md | 15 ++++++--- e2e/harness/faults.ts | 72 +++++++++++++++++++++++++++++++++++++++++++ e2e/harness/index.ts | 29 ++++++++--------- 3 files changed, 95 insertions(+), 21 deletions(-) create mode 100644 e2e/harness/faults.ts diff --git a/e2e/ARCHITECTURE.md b/e2e/ARCHITECTURE.md index 8b1163b3..218f6d04 100644 --- a/e2e/ARCHITECTURE.md +++ b/e2e/ARCHITECTURE.md @@ -414,10 +414,17 @@ pnpm test # starts and stops the con Every test skips when docker is unavailable, rather than failing. -A failing test attaches `browser-console`, the console errors and uncaught exceptions its pages -raised. `use.trace` and playwright's own reporting only cover contexts playwright made itself, and -the harness makes its own, so without that attachment a failure carries the DOM snapshot and -nothing the app said — which is unreadable when what failed is the app rendering its error page. +A test fails when the app broke while it ran, whatever it asserted: an uncaught exception on any +of its pages, or code of ours the browser refused under the content security policy. A refusal +reaches the console and nothing else, which is how a policy that had rotted past the script it +names stayed invisible to every spec for a week (#535). A failed request or a warning is neither, +and a dev server is full of both, so those are logged and left alone — as is the route chunk +sveltekit loses when the per-test container churns the network out from under it (#529). + +Either way a failing test attaches `browser-console`, everything both sides said. `use.trace` and +playwright's own reporting only cover contexts playwright made itself, and the harness makes its +own, so without that attachment a failure carries the DOM snapshot and nothing the app said — +which is unreadable when what failed is the app rendering its error page. One engine per run. These specs exercise sockets, auth and sync, so running them under three engines adds little coverage. `E2E_BROWSER=webkit pnpm test` runs the whole diff --git a/e2e/harness/faults.ts b/e2e/harness/faults.ts new file mode 100644 index 00000000..a4b29df0 --- /dev/null +++ b/e2e/harness/faults.ts @@ -0,0 +1,72 @@ +import {inspect} from "node:util" +import type {BrowserContext} from "@playwright/test" + +// A CSP refusal reaches the console and nothing else, so a policy that has rotted past the script +// it names is invisible to every spec: app.html's requestIdleCallback shim was refused on every +// platform for a week with the suite green (#535). +const isRefusal = (text: string) => text.includes("Content Security Policy") + +// Every test recreates the zooid container, chromium aborts what the page had in flight when the +// interfaces churn, and sveltekit reports a route chunk lost that way as an uncaught TypeError. It +// reaches nearly every spec — 256 of the 258 faults a full survey run raised — so it stays in the +// log and out of the fault set until #529 stops the churn. +const isChunkLoss = (text: string) => text.includes("Failed to fetch dynamically imported module") + +// Playwright builds this from the page's exception details, and a page that throws something other +// than an Error leaves it with neither message nor stack — which is how a fault used to reach the +// console log as a bare "uncaught:". +const describe = (error: Error) => error.stack || error.message || inspect(error) + +export type FaultWatch = { + // Everything either side said, for the account attached to a failing test. + log: string[] + // The subset of it that means the app broke rather than the box being noisy. + found: string[] + observe(context: BrowserContext, who: string): void + // Throws when the app itself broke while the test ran: an uncaught exception, or code of ours the + // browser refused to run. A failed request or a noisy warning is neither — a dev server is full + // of both — so those stay in the log. + assertNone(): void +} + +export const watchFaults = (): FaultWatch => { + const log: string[] = [] + const found: string[] = [] + + const record = (line: string, fault: boolean) => { + log.push(line) + + if (fault) found.push(line) + } + + return { + log, + found, + observe(context, who) { + context.on("console", message => { + const type = message.type() + const text = message.text() + + if (["error", "warning"].includes(type)) { + record(`[${who}] ${type}: ${text}`, type === "error" && isRefusal(text)) + } + }) + + context.on("weberror", error => { + const text = describe(error.error()) + + record(`[${who}] uncaught: ${text}`, !isChunkLoss(text)) + }) + }, + assertNone() { + if (found.length > 0) { + throw new Error( + [ + "The app broke while the test ran, so nothing it asserted means anything:", + ...found.map(fault => ` ${fault}`), + ].join("\n"), + ) + } + }, + } +} diff --git a/e2e/harness/index.ts b/e2e/harness/index.ts index 11d43e35..2f820363 100644 --- a/e2e/harness/index.ts +++ b/e2e/harness/index.ts @@ -17,6 +17,7 @@ import { } from "./net/http" import type {BlossomOptions, HostingFixtures, RelayInfoOverrides} from "./net/http" import {assertNoLeaks, installWebSocketRoutes} from "./net/websocket" +import {watchFaults} from "./faults" import {boot} from "./app/boot" import {injectNip07} from "./app/nip07" import {injectWebLn} from "./app/webln" @@ -194,7 +195,7 @@ export const test = base.extend({ await zooid.reset() const contexts: BrowserContext[] = [] - const browserLog: string[] = [] + const faults = watchFaults() let scenario: Maybe @@ -223,17 +224,7 @@ export const test = base.extend({ // `use.trace` and the built-in reporting only cover contexts playwright made itself, so a // failure in a context opened here arrives with the app's own account of it thrown away — // which is how a spec that caught the app on its 500 page had nothing to say about why. - const who = user?.name ?? "anonymous" - - context.on("console", message => { - if (["error", "warning"].includes(message.type())) { - browserLog.push(`[${who}] ${message.type()}: ${message.text()}`) - } - }) - - context.on("weberror", error => { - browserLog.push(`[${who}] uncaught: ${error.error().stack ?? error.error().message}`) - }) + faults.observe(context, user?.name ?? "anonymous") // Playwright matches the most recently registered route first and every mock falls through // what it doesn't recognize, so the block-all goes in before the mocks, and all of it before @@ -294,18 +285,22 @@ export const test = base.extend({ }, }) - if (browserLog.length > 0 && testInfo.status !== testInfo.expectedStatus) { + // Closed before anything is read off it: an $effect teardown reading a binding svelte has + // already cleared throws on unmount, and that is the fault the suite was blindest to. + for (const context of contexts) { + await context.close() + } + + if (faults.found.length > 0 || testInfo.status !== testInfo.expectedStatus) { // A path rather than a body: the list reporter truncates an inline attachment, and the // nightly run on the box keeps test-results and nothing else. const path = testInfo.outputPath("browser-console.log") - await writeFile(path, browserLog.join("\n")) + await writeFile(path, faults.log.join("\n")) await testInfo.attach("browser-console", {path, contentType: "text/plain"}) } - for (const context of contexts) { - await context.close() - } + faults.assertNone() for (const context of contexts) { assertNoLeaks(context)