From 6f13a09a08bdd69b8db2ac0ef5f11fdb6ab12915 Mon Sep 17 00:00:00 2001 From: d-oit Date: Thu, 24 Sep 2026 21:11:33 +0200 Subject: [PATCH] fix(e2e): gate tests on a served response and keep failure artifacts Amp-Thread-ID: https://ampcode.com/threads/T-01a0cf2b-5969-7627-bccd-702e8d30adda Co-authored-by: Amp --- .github/workflows/ci-and-labels.yml | 14 ++- ...ness-readiness-and-artifacts-2026-09-24.md | 96 +++++++++++++++ playwright.config.ts | 17 ++- src/lib/__tests__/e2e-harness.test.ts | 109 ++++++++++++++++++ 4 files changed, 231 insertions(+), 5 deletions(-) create mode 100644 plans/153-e2e-harness-readiness-and-artifacts-2026-09-24.md create mode 100644 src/lib/__tests__/e2e-harness.test.ts diff --git a/.github/workflows/ci-and-labels.yml b/.github/workflows/ci-and-labels.yml index c13c0456..cf4a3e23 100644 --- a/.github/workflows/ci-and-labels.yml +++ b/.github/workflows/ci-and-labels.yml @@ -201,12 +201,20 @@ jobs: else pnpm run test:e2e --project=chromium fi - - name: Upload Playwright report + - name: Upload Playwright artifacts if: failure() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: - name: playwright-report - path: playwright-report/ + name: playwright-artifacts + # Both directories matter. The HTML report is written by the `html` + # reporter (CI only), and `test-results/` holds the traces and error + # contexts (`trace: 'on-first-retry'`). Uploading `playwright-report/` + # alone kept nothing at all, because the list reporter never creates it — + # so a failed run left no evidence to diagnose (plans/153). + path: | + playwright-report/ + test-results/ + if-no-files-found: warn retention-days: 14 build: diff --git a/plans/153-e2e-harness-readiness-and-artifacts-2026-09-24.md b/plans/153-e2e-harness-readiness-and-artifacts-2026-09-24.md new file mode 100644 index 00000000..852375a4 --- /dev/null +++ b/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 `
`, 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. diff --git a/playwright.config.ts b/playwright.config.ts index 0a7db63d..e3e99e60 100644 --- a/playwright.config.ts +++ b/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', }, }); diff --git a/src/lib/__tests__/e2e-harness.test.ts b/src/lib/__tests__/e2e-harness.test.ts new file mode 100644 index 00000000..5e4f4148 --- /dev/null +++ b/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 => { + 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') + }) +})