Fail an e2e test when the page threw or the browser refused our code

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
+A test fails when the app broke while it ran, whatever it asserted: an uncaught exception on any
-raised. `use.trace` and playwright's own reporting only cover contexts playwright made itself, and
+of its pages, or code of ours the browser refused under the content security policy. A refusal
-the harness makes its own, so without that attachment a failure carries the DOM snapshot and
+reaches the console and nothing else, which is how a policy that had rotted past the script it
-nothing the app said — which is unreadable when what failed is the app rendering its error page.
+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

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"),
+        )
+      }
+    },
+  }
+}

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<HarnessFixtures, HarnessWorkerFixtures>({
     await zooid.reset()
 
     const contexts: BrowserContext[] = []
-    const browserLog: string[] = []
+    const faults = watchFaults()
 
     let scenario: Maybe<Scenario>
 
@@ -223,17 +224,7 @@ export const test = base.extend<HarnessFixtures, HarnessWorkerFixtures>({
       // `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"
+      faults.observe(context, 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}`)
-      })
 
       // 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<HarnessFixtures, HarnessWorkerFixtures>({
       },
     })
 
-    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) {
+    faults.assertNone()
-      await context.close()
-    }
 
     for (const context of contexts) {
       assertNoLeaks(context)