diff --git a/.convoys/adopt-playwright-smoke.md b/.convoys/adopt-playwright-smoke.md new file mode 100644 index 0000000..ff09442 --- /dev/null +++ b/.convoys/adopt-playwright-smoke.md @@ -0,0 +1,276 @@ +--- +name: adopt-playwright-smoke +classification: convoy +success_metric: | + `Playwright smoke` on every PR reaches `npx playwright test` and either + passes (smoke green) or fails on real test assertions. The current + fast-fail at the test step ("playwright not installed" / "no config") + goes away. `Screenshot diff` workflow either passes (snapshots stable) + or fails on a real pixel diff with the standard upload + PR comment + artifacts. Both checks complete in < 5 minutes. +skip: + - role-design-system-auditor # spec authoring, not visual design + - role-a11y-auditor # tested-from-outside; a11y belongs in deeper specs + - role-ux-reviewer # no UX surface + - role-ia-architect # no IA surface +status: queued +created: 2026-05-24 +parent: ship-readiness +addresses: P1 #10 step 2 (launch sequence step 10) +depends_on: + - fix-vercel-deployment-protection-in-ci (shipped — PR #17, 9a3e077) + - bump-next-js (shipped — e57ea17; needed for working Vercel previews) +--- + +# Convoy: adopt-playwright-smoke + +Stand up Playwright end-to-end. The infrastructure scaffolding has already +landed in three earlier convoys; this one wires the actual `@playwright/test` +dependency, the `playwright.config.js` that points it at the Vercel preview +URL, and turns the existing `tests/smoke/app.smoke.spec.ts` from "drafted +but inert" into "executed by CI on every PR". + +## Why now + +PR #17 (`fix-vercel-deployment-protection-in-ci`, squash `9a3e077`) closed +the last infra gap blocking Playwright smoke. The wait-action now reaches a +2xx in ~200ms, both `BASE_URL` and `VERCEL_AUTOMATION_BYPASS_SECRET` are +already exported to the smoke / screenshot-capture step's `env:`, and +`tests/smoke/app.smoke.spec.ts` is already drafted (3 tests: home renders +without 5xx, sign-in page renders, `/api/health` responds 2xx — and +`pages/api/health.js` already exists, so the third test won't 404). + +What's missing is small and well-bounded: + +1. `@playwright/test` is NOT in `package.json` devDependencies (verified via + `grep`-of-package.json). +2. There is no `playwright.config.js` (or `.ts`) in the tree. +3. The smoke spec is `.ts` in a JS-only repo (no `tsconfig.json`, no other + `.ts` source files except `next-env.d.ts`). Decision to ratify: convert + to `.js` or accept `.ts` for `tests/` only? +4. The `Screenshot diff` workflow runs `npx playwright test --project=visual` + which also needs a `visual` project in the config. The visual workflow's + "Capture screenshots (PR)" step has `continue-on-error: true` and a + PR-comment step (after PR #16's `pull-requests: write` perm fix), so + we can ship without baselines on the first run — the comment will say + "no baselines yet, run `--update-snapshots` locally to seed them." + +This convoy is the next step in the launch sequence (step 10 of +`.convoys/ship-readiness.md`'s "Proposed launch sequence"). After it ships, +every PR gets real smoke regression signal — which materially de-risks +every subsequent convoy (`single-auth-provider`, `single-sql-client`, +`god-component-split`, etc.) because those will start touching live page +flows that nothing currently exercises automatically. + +## Scope + +**In scope:** + +- `package.json` — add `@playwright/test` to `devDependencies`. (Latest + stable as of 2026-05-24; architect picks the exact version pin.) +- `package-lock.json` — regenerated by `npm install`. +- `package.json` `scripts` — add `test:smoke` and `test:visual` (or + a single `test:e2e`; architect's call). Should the scripts run against + `localhost:3000` by default and require an explicit `BASE_URL` for + preview, or vice versa? Decision to ratify. +- `playwright.config.js` (new) — at minimum: `testDir: './tests'`, two + `projects:` blocks (`smoke` and `visual`) matching the workflow + invocations (`npx playwright test --project=smoke|visual`), + `use.baseURL` from `process.env.BASE_URL`, `use.extraHTTPHeaders` + forwarding `x-vercel-protection-bypass` from + `process.env.VERCEL_AUTOMATION_BYPASS_SECRET` (per AGENTS.md § 7), + and a reasonable `timeout` / `expect.timeout`. +- `tests/smoke/app.smoke.spec.ts` (existing) — keep as `.ts` OR rename + to `.js`, depending on Decision A. If kept as `.ts`, may need to + exclude `tests/**/*.ts` from ESLint (the JS-only repo's lint config + doesn't currently handle `.ts` and will likely error). +- `tests/visual/` (new directory + at least one trivial spec) — needs + a single `.spec.ts` (or `.js`) that takes a screenshot of the + homepage. Without a spec, `npx playwright test --project=visual` + exits 0 and the screenshot workflow has nothing to compare. +- `eslint.config.mjs` (possibly) — if Decision A keeps `.ts` specs, + add `tests/**/*.ts` to `globalIgnores` OR wire typescript-eslint + to parse them safely. +- `.gitignore` (possibly) — `test-results/`, `playwright-report/`, + `.playwright/` should be ignored (Playwright generates these on + every local run). +- `AGENTS.md` — section on running smoke tests locally (`npm run + test:smoke`) + the "expect baselines to drift on UI changes; run + `--update-snapshots`" guidance. The doc-writer pass at convoy + close handles this; the brief should NOT touch `AGENTS.md`. + +**Out of scope:** + +- **Writing deep E2E tests beyond the 3 existing smoke checks.** This + convoy makes smoke green; deeper coverage is per-feature work in + feature convoys (`add-rate-limiting` adds a rate-limit smoke check, + etc.). +- **Authoring real visual baselines.** First-run snapshots can be + trivial (homepage only). Real baseline curation across critical + pages is a separate convoy (`adopt-visual-baselines`?) once UX has + stabilized post-`pick-a-name`. +- **Re-enabling the `test:` job in `.github/workflows/ci.yml`.** Per + ship-readiness P1 #10 step 3, that re-enable is a separate task — + this convoy's job is the Playwright side only. +- **Replacing `wait-for-vercel-preview`.** Still queued as + `replace-wait-for-vercel-preview` if the action ages out further. +- **Adding `test:smoke:local` cron / pre-commit hooks.** Smoke specs + should be runnable locally on demand; automatic cron is a separate + scope. +- **Migrating any source files to TypeScript.** Decision A may keep + the spec as `.ts`, but that's a test-only file — no source code + migrates. See AGENTS.md Gotcha #9. + +## Operator action required + +**None.** All prerequisites are already in place: + +- `VERCEL_AUTOMATION_BYPASS_SECRET` is seeded in GitHub Actions repo + secrets (`gh secret list` shows it; seeded 2026-05-24T20:03:31Z). +- Both target workflows (`preview-smoke.yml`, `visual-diff.yml`) + already export the secret to the test step's `env:`. +- `BASE_URL` is already wired. +- `pages/api/health.js` already exists for the existing smoke spec. + +## Decisions to ratify with operator + +Queued; do not pre-decide. Architect picks recommended option per +decision and routes back at gate 1. + +1. **`.ts` vs `.js` for Playwright specs.** The existing + `tests/smoke/app.smoke.spec.ts` uses TypeScript-flavored imports + (`import { test, expect } from '@playwright/test'`). The codebase + is JS-only (no `tsconfig.json`, no other `.ts` source files except + `next-env.d.ts`). Three options: + - **(a)** Convert the spec to `.js` — matches codebase convention, + no eslint config change needed (the import syntax works fine in + ESM `.js`). + - **(b)** Keep `.ts` for `tests/` only — Playwright docs default + to `.ts`; tests are isolated from production code; need to add + `tests/**/*.ts` to `eslint.config.mjs`'s `globalIgnores` or + properly configure typescript-eslint for the tests directory. + - **(c)** Mixed — `.js` for smoke, `.ts` for new specs going + forward. Inconsistent; not recommended. +2. **Fail-loud vs warn-and-continue when + `VERCEL_AUTOMATION_BYPASS_SECRET` is unset.** Same pattern as + `lib/rate-limit.js` (per AGENTS.md Gotcha #12): in CI (where + `process.env.CI === 'true'`), fail loudly — throw at config load + time with a clear error pointing at `gh secret set ...`. In dev + (where the secret might be missing but you're hitting localhost), + warn-and-continue. Architect picks the exact predicate and + error-message wording. +3. **One project (`smoke`) or two (`smoke` + `visual`)?** Both + workflows already invoke `--project=smoke` and `--project=visual` + respectively (post PR #17). Splitting into two projects in the + config is required. The question is whether `visual` should reuse + the same specs as `smoke` (with screenshot assertions added) OR + live in its own `tests/visual/` directory with separate specs. + The convoy's success metric only requires the workflows to run to + completion — the actual visual-coverage scope is a follow-up. +4. **First-run visual baselines.** With no committed baseline images, + `npx playwright test --project=visual` will either (a) fail + (no baselines to diff against) — which the workflow's + `continue-on-error: true` swallows, then the upload + comment step + surfaces the missing-baseline state — OR (b) Playwright treats + no-baseline as "create on first run" (depends on config). + Decision: do we commit a trivial homepage baseline now, OR + document the "run `--update-snapshots` locally first" workflow, + OR auto-commit baselines via a separate PR? +5. **ESLint coverage for `tests/`.** Currently + `eslint.config.mjs` does NOT explicitly ignore `tests/`. If + Decision A keeps `.ts`, eslint will try to parse it. The two + choices: add `tests/**/*.ts` (or just `tests/**`) to + `globalIgnores`, OR wire typescript-eslint into the test + directory. Latter is more work for arguable test-side benefit; + recommend the former. +6. **Should we add a `test:smoke:local` script that boots + `next dev` and runs against `localhost:3000` automatically?** Or + leave it as "you boot dev manually, then `BASE_URL=http://localhost:3000 + npm run test:smoke`"? The latter is simpler; the former is + friendlier. Convention-match with the existing `test:run` script + shape. + +## Known constraints + +- **`tests/smoke/app.smoke.spec.ts` already exists** with 3 tests + using `@playwright/test`'s API. Don't rewrite it; just enable it. + The third test references `/api/health`, which exists at + `pages/api/health.js` — confirmed. +- **Both target workflows already export + `VERCEL_AUTOMATION_BYPASS_SECRET` and `BASE_URL` to the test + step's `env:`** — `playwright.config.js`'s job is to read them + and apply them via `use.baseURL` + `use.extraHTTPHeaders`. +- **The header form** of the Vercel bypass + (`x-vercel-protection-bypass: `) is the correct shape + for a browser cookie-jar context. Per AGENTS.md § 7 and PR #17's + Decision A reservation, this is what `playwright.config.js` + should use — NOT the query-param form (that's reserved for + curl/axios contexts without cookie jars). +- **Playwright versions** ship browsers as a separate install step + (`npx playwright install --with-deps chromium` — already in + both workflows). Picking a Playwright version pin should consider + the workflow's expectation that the binary exists. +- **Vercel preview URLs are auth-protected.** This is why the bypass + exists. Without `use.extraHTTPHeaders` correctly wired, every + `page.goto(BASE)` call will hit Vercel SSO and the test will fail + with a content-mismatch (not a 401, because Vercel returns an HTML + SSO challenge page with 401 status). +- **Test runtime budget:** the smoke spec's comment says "<60s + total". Three trivial smoke tests should run in well under 30s. + The convoy's success metric is < 5 min for the whole workflow + (which includes ~2 min of `npm ci` + browser install). Comfortable. + +## Acceptance criteria + +The convoy is shippable when ALL of the following hold: + +1. `Playwright smoke` workflow on a fresh PR reaches `npx playwright + test --project=smoke`, browsers are installed, and the 3 existing + smoke tests run to completion. Pass OR fail; just not "no config". +2. `Screenshot diff` workflow on a fresh PR touching `pages/**` or + `components/**` reaches its visual capture step and either passes + OR posts a meaningful "Visual Diff" comment to the PR (per the + existing `continue-on-error: true` + comment step pattern). +3. Both workflows complete in < 5 minutes total. +4. `npm run test:smoke` (or whatever Decision 6 chooses) works + locally against either localhost or a deployed preview URL, + given a `BASE_URL` env var. +5. `npm run lint` exit code matches baseline (still 128 problems + per the `fix-lint-baseline` convoy; do NOT regress). +6. `npm run test:run` (vitest) still passes 21/21 (no regression + from the existing test surface). +7. Bypass secret does NOT appear in any workflow run log. Verify + by downloading the raw log of a passing run and grepping for + the secret's first 8 chars. +8. `playwright.config.js` is well-commented (every non-obvious + choice has a one-line explanation), so the next agent doesn't + need to re-derive context from the convoy file. + +## Anything flagged but not acted on (in advance) + +- **`tests/smoke/app.smoke.spec.ts` is `.ts` in a JS-only repo.** + This is a real anomaly. Decision A resolves it one way or another. + If we keep `.ts`, document the special-case treatment in + AGENTS.md so future agents don't try to "normalize" by converting + to `.js` (or vice versa). +- **Visual baseline curation strategy.** First-run baselines will + inevitably need re-capturing as the UI evolves toward the + `pick-a-name` rebrand. Don't over-invest in baselines this + convoy; document the `--update-snapshots` workflow and move on. +- **CI workflow `paths:` filter for `visual-diff.yml`.** Currently + the filter is `pages/**`, `components/**`, `styles/**`, + `tailwind.config.js`, `postcss.config.js`. After this convoy + ships, the filter is still correct — visual tests should + re-run when any of those change. No change needed in this + convoy. +- **`@playwright/test` security advisories.** Pin a recent version + and document the rationale (avoid security CVEs, avoid known + buggy versions). Architect picks; no operator ratification + needed unless a specific advisory is relevant. +- **PR-comment template for `Screenshot diff`.** The existing + comment-on-PR step in `visual-diff.yml` hardcodes the comment + body ("Screenshots and diffs uploaded as artifacts: [view + run](...)"). After this convoy, the body should arguably + include a quick diff summary (pixel count, % difference per + page). That's an enhancement for a separate `polish-visual-diff-comment` + convoy; not in scope here.