fix(e2e): gate tests on a served response and keep failure artifacts

plans/153-e2e-harness-readiness-and-artifacts-2026-09-24.md

@@ -0,0 +1,96 @@
+# Plan 153 — The E2E Harness Could Not Prove Its Server Was Up, and Kept No Evidence (2026-09-24)
+
+**Type**: CI harness defect + diagnosability
+**Scope**: `playwright.config.ts`, `.github/workflows/ci-and-labels.yml`,
+`src/lib/__tests__/e2e-harness.test.ts` (new)
+**Follows**: the E2E failure on PR #817 (transient, unexplained)
+
+## 1. What happened
+
+PR #817's E2E job ran 149 tests against the dev server and **140 failed**, all of
+them on the same readiness wait:
+
+```
+Error: expect(locator).toBeAttached() failed
+Locator: locator('[data-app-ready="true"]')
+> 20 |   await expect(page.locator('[data-app-ready="true"]')).toBeAttached();
+```
+
+Only the contrast tests — which do no page load — passed. The run took 14.6
+minutes instead of ~3. Re-running the same job on the same commit **passed**, so
+the trigger was environmental rather than a code regression. But it was not
+diagnosable, and that is the defect this plan fixes:
+
+1. **Readiness was a bound port, not a served response.** `webServer.port: 3000`
+   is satisfied the moment `next dev` binds, while it is still compiling. Tests
+   began 3 seconds after the server started. If the server cannot answer yet, every
+   test that navigates fails on its own 5-second readiness wait — a wall of
+   failures that looks like 140 broken tests.
+2. **The failure left no evidence.** The job uploaded `playwright-report/` on
+   failure, but the config's `reporter: 'list'` never creates that directory, so
+   the upload had nothing to send. Meanwhile `trace: 'on-first-retry'` was
+   collecting traces into `test-results/`, which no step uploaded. The failed
+   run's artifacts contain only the coverage report — there was no trace, no error
+   context, and no server log to explain what the server was doing.
+
+## 2. Change
+
+**`playwright.config.ts`**
+
+| Setting | Before | After | Why |
+|---|---|---|---|
+| `webServer` readiness | `port: 3000` | `url: 'http://localhost:3000'` | Wait for a real response; a server that never becomes healthy is now one clear timeout instead of a wall of test failures |
+| `webServer.timeout` | default (60 s) | `120000` | Covers a cold `next dev` compile on a slow runner |
+| `webServer.stdout` / `stderr` | default | `'pipe'` | The server's own output appears in the job log |
+| `reporter` | `'list'` | CI: `[['list'], ['html', { open: 'never' }]]` | Creates the `playwright-report/` directory the workflow uploads |
+
+**`ci-and-labels.yml`** — the failure upload now covers both directories and is
+renamed to match what it actually carries:
+
+```yaml
+      - name: Upload Playwright artifacts
+        if: failure()
+        with:
+          name: playwright-artifacts
+          path: |
+            playwright-report/
+            test-results/
+          if-no-files-found: warn
+```
+
+## 3. Verification
+
+`src/lib/__tests__/e2e-harness.test.ts` loads the real config (with `CI` stubbed,
+since the reporter depends on it) and the real workflow, and asserts the contract
+above. Each assertion was mutation-checked:
+
+| Mutation | Result |
+|---|---|
+| `url` → `port: 3000` | `waits for a served response, not a bound port` fails |
+| drop `test-results/` from the upload path | `uploads the traces alongside the report` fails |
+| `reporter` → `'list'` in CI | `writes the HTML report in CI` fails |
+| restored | 6/6 pass |
+
+`pnpm exec playwright test e2e/home.spec.ts e2e/graph-density.spec.ts
+--project=chromium` — 10 passed, so the config still parses and drives a real run.
+
+**The cold-start path itself is verified by the next CI run**, which is where the
+failure happened: it starts `pnpm run dev` itself and now must see a served
+response before the first test begins.
+
+## 4. What this does not fix
+
+The underlying stall is unexplained — with the dev server answering HTTP while
+serving pages that lack `<main>`, the most likely shape is a degraded runner or a
+stalled `next dev`. This plan makes that shape visible (server log, traces, one
+readiness timeout) rather than guessing at it now. If it recurs, the artifacts
+from that run will name the cause.
+
+## 5. Follow-ups
+
+1. **Watch the next E2E failure for the new artifacts** — `playwright-artifacts`
+   should contain `test-results/` with traces.
+2. **Consider a production server for E2E** (`pnpm run build && pnpm run start`):
+   no on-demand compilation, and closer to production. Deliberately not done here:
+   it changes what every E2E test runs against, which deserves its own change and
+   a full four-project validation.

playwright.config.ts

@@ -4,7 +4,10 @@ export default defineConfig({
   testDir: './e2e',
   timeout: 30000,
   retries: 1,
-  reporter: 'list',
+  // `list` is the console summary. CI additionally writes the HTML report the
+  // workflow uploads, because the list reporter alone creates no
+  // `playwright-report/` directory for that step to pick up (plans/153).
+  reporter: process.env.CI ? [['list'], ['html', { open: 'never' }]] : 'list',
   use: {
     baseURL: 'http://localhost:3000',
     trace: 'on-first-retry',
@@ -32,7 +35,17 @@ export default defineConfig({
   ],
   webServer: {
     command: 'pnpm run dev',
-    port: 3000,
+    // Readiness is a real HTTP response, not a bound port: `port` is satisfied as
+    // soon as `next dev` binds, while it is still compiling. On 2026-09-24 a PR run
+    // started 149 tests against a server that could not serve yet and 140 failed on
+    // their readiness waits (plans/153). `url` also turns a server that never
+    // becomes healthy into one clear timeout instead of a wall of test failures.
+    url: 'http://localhost:3000',
+    timeout: 120000,
     reuseExistingServer: !process.env.CI,
+    // Pipe the dev server's own output into the job log: with the default the
+    // server is invisible, which is what made that failure undiagnosable.
+    stdout: 'pipe',
+    stderr: 'pipe',
   },
 });

src/lib/__tests__/e2e-harness.test.ts

@@ -0,0 +1,109 @@
+import { describe, it, expect, afterEach, vi } from 'vitest'
+import { readFileSync } from 'fs'
+import { join } from 'path'
+import { parse } from 'yaml'
+
+/**
+ * Contract tests for the E2E harness (plans/153).
+ *
+ * Both halves of this file exist because of one real failure: a PR run on
+ * 2026-09-24 started 149 tests against a `next dev` server that had bound its port
+ * but could not serve yet, 140 of them failed, and the run left **no evidence** —
+ * the workflow uploaded a `playwright-report/` directory that the `list` reporter
+ * never creates, while the traces in `test-results/` were never uploaded. These
+ * tests pin the two properties that made it undiagnosable.
+ */
+
+/** Load `playwright.config.ts` with `CI` set, since the reporter depends on it. */
+const loadConfig = async (ci: boolean): Promise<PlaywrightConfig> => {
+  vi.resetModules()
+  vi.stubEnv('CI', ci ? 'true' : '')
+  const loaded = (await import('../../../playwright.config')) as { default: PlaywrightConfig }
+  return loaded.default
+}
+
+interface PlaywrightConfig {
+  reporter: unknown
+  webServer: {
+    command: string
+    url?: string
+    port?: number
+    timeout?: number
+    reuseExistingServer?: boolean
+    stdout?: string
+    stderr?: string
+  }
+}
+
+const loadWorkflow = () => {
+  const path = join(process.cwd(), '.github/workflows', 'ci-and-labels.yml')
+  return parse(readFileSync(path, 'utf-8'))
+}
+
+afterEach(() => {
+  vi.unstubAllEnvs()
+})
+
+describe('Playwright web server readiness', () => {
+  it('waits for a served response, not a bound port', async () => {
+    const config = await loadConfig(false)
+
+    // `port` is satisfied the moment `next dev` binds, while it is still
+    // compiling — which is how 149 tests started against a server that could not
+    // answer. `url` waits for an actual HTTP response.
+    expect(config.webServer.url).toBe('http://localhost:3000')
+    expect(config.webServer.port).toBeUndefined()
+    expect(config.webServer.timeout).toBeGreaterThan(60000)
+  })
+
+  it('pipes the dev server output into the job log', async () => {
+    const config = await loadConfig(false)
+
+    // Without this the server is invisible in CI, so a stall cannot be told apart
+    // from a broken test.
+    expect(config.webServer.stdout).toBe('pipe')
+    expect(config.webServer.stderr).toBe('pipe')
+  })
+
+  it('keeps the local server reuse so a running dev server is not restarted', async () => {
+    const config = await loadConfig(false)
+    expect(config.webServer.reuseExistingServer).toBe(true)
+  })
+})
+
+describe('Playwright reporters', () => {
+  it('writes the HTML report in CI, where the workflow uploads it', async () => {
+    const config = await loadConfig(true)
+
+    expect(JSON.stringify(config.reporter)).toContain('html')
+  })
+
+  it('keeps the console list reporter locally', async () => {
+    const config = await loadConfig(false)
+
+    expect(config.reporter).toBe('list')
+  })
+})
+
+describe('E2E failure artifacts', () => {
+  it('uploads the traces alongside the report', () => {
+    const workflow = loadWorkflow()
+    const steps = workflow.jobs['e2e-tests'].steps as Array<{
+      name?: string
+      if?: string
+      with?: { name?: string; path?: string; 'if-no-files-found'?: string }
+    }>
+    const upload = steps.find((step) => step.name === 'Upload Playwright artifacts')
+    expect(upload).toBeDefined()
+
+    // `trace: 'on-first-retry'` writes into test-results/, which no upload step
+    // covered: the report path is created by the html reporter only.
+    expect(upload?.if).toBe('failure()')
+    const paths = (upload?.with?.path ?? '').split('\n').map((line) => line.trim())
+    expect(paths).toContain('playwright-report/')
+    expect(paths).toContain('test-results/')
+
+    // A missing directory is worth a warning, not silence.
+    expect(upload?.with?.['if-no-files-found']).toBe('warn')
+  })
+})