660 lines
32 KiB
Markdown
660 lines
32 KiB
Markdown
|
|
---
|
|||
|
|
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: in-progress
|
|||
|
|
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: <secret>`) 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.
|
|||
|
|
|
|||
|
|
## Decisions (post-IA round)
|
|||
|
|
|
|||
|
|
Six decisions queued in the convoy file. Architect-investigated;
|
|||
|
|
recommendations + ratification routing below. **3 of 6** are
|
|||
|
|
architect-self-ratifiable (structural facts or convention mirrors).
|
|||
|
|
**3 of 6** need operator ratification at human gate 1.
|
|||
|
|
|
|||
|
|
### Decision 1 — `.ts` vs `.js` for Playwright specs → **(b) keep `.ts`** (operator ratifies)
|
|||
|
|
|
|||
|
|
**Recommendation:** keep `tests/smoke/app.smoke.spec.ts` as `.ts`;
|
|||
|
|
new visual spec ships as `tests/visual/homepage.spec.ts`.
|
|||
|
|
|
|||
|
|
**Investigation:**
|
|||
|
|
|
|||
|
|
- `npx eslint tests/smoke/app.smoke.spec.ts` — exit 0, zero output
|
|||
|
|
(verified 2026-05-24 against the post-PR-17 tree). The
|
|||
|
|
`eslint-config-next/core-web-vitals` chain bundled by
|
|||
|
|
`eslint-config-next@16` already parses `.ts` files via its
|
|||
|
|
transitive `typescript-eslint` dependency. **This is the same
|
|||
|
|
mechanism that makes `typescript@^5.9.3` a hard devDep even
|
|||
|
|
though no source file is TS** (AGENTS.md Gotcha #9).
|
|||
|
|
- `npm run lint` baseline stays at 128 problems with the `.ts`
|
|||
|
|
spec present — confirmed empirically. Decision 5 (no eslint
|
|||
|
|
config change) follows from this.
|
|||
|
|
|
|||
|
|
**Rationale:**
|
|||
|
|
|
|||
|
|
1. Playwright's official docs and `create-playwright` scaffolding
|
|||
|
|
default to `.ts`. Fighting that for every future spec is
|
|||
|
|
friction.
|
|||
|
|
2. Test files are isolated from production source — AGENTS.md
|
|||
|
|
Gotcha #9's "no `.ts` files except `next-env.d.ts`" rule was
|
|||
|
|
scoped at production code, not test infrastructure.
|
|||
|
|
3. Lint already accepts it; no `eslint.config.mjs` change needed.
|
|||
|
|
4. Converting to `.js` saves one anomaly in `rg --type=ts` output
|
|||
|
|
but creates a new precedent ("the repo prefers `.js` even when
|
|||
|
|
the framework defaults to `.ts`") that future Playwright work
|
|||
|
|
would have to re-litigate.
|
|||
|
|
|
|||
|
|
**Routing:** operator ratifies. Convention preference, not a
|
|||
|
|
structural requirement.
|
|||
|
|
|
|||
|
|
### Decision 2 — fail-loud vs warn-and-continue → **CI fail-loud, dev warn-and-no-op** (architect-self-ratifiable)
|
|||
|
|
|
|||
|
|
**Decision:** in `playwright.config.js`, `throw` at config load
|
|||
|
|
when `process.env.CI === 'true'` AND
|
|||
|
|
`process.env.VERCEL_AUTOMATION_BYPASS_SECRET` is missing /
|
|||
|
|
empty. In non-CI (`process.env.CI` unset), `console.warn` once
|
|||
|
|
and continue with `extraHTTPHeaders` undefined (caller hits
|
|||
|
|
localhost or a public URL).
|
|||
|
|
|
|||
|
|
**Rationale:** mirrors the established `lib/rate-limit.js`
|
|||
|
|
convention documented in AGENTS.md Gotcha #12 ("In prod, the
|
|||
|
|
rate-limit module fails closed if either of the two REST vars is
|
|||
|
|
missing... In dev / test, it warn-and-continues as a no-op").
|
|||
|
|
Predicate is `process.env.CI === 'true'` (not `NODE_ENV ===
|
|||
|
|
'production'`) because Playwright config has no Next.js context
|
|||
|
|
and `CI` is the canonical CI-runner signal. Error message names
|
|||
|
|
the env var, the rotation command (`gh secret set
|
|||
|
|
VERCEL_AUTOMATION_BYPASS_SECRET --body "<value>"`), and points
|
|||
|
|
at AGENTS.md § 7 for the full context.
|
|||
|
|
|
|||
|
|
**Routing:** architect-self-ratifiable. Mirrors existing repo
|
|||
|
|
convention; not a fresh policy.
|
|||
|
|
|
|||
|
|
### Decision 3 — one project (`smoke`) or two (`smoke` + `visual`) → **two projects, two directories** (architect-self-ratifiable)
|
|||
|
|
|
|||
|
|
**Decision:** `playwright.config.js` declares two projects:
|
|||
|
|
|
|||
|
|
- `smoke` — `testMatch: 'smoke/**/*.spec.@(ts|js)'`
|
|||
|
|
- `visual` — `testMatch: 'visual/**/*.spec.@(ts|js)'`
|
|||
|
|
|
|||
|
|
Both projects share the same `use:` block (`baseURL`,
|
|||
|
|
`extraHTTPHeaders`, `trace: 'off'`). The `visual` project alone
|
|||
|
|
hits `expect(page).toHaveScreenshot()`; `smoke` does not.
|
|||
|
|
|
|||
|
|
**Rationale:** the post-PR-17 workflows already invoke
|
|||
|
|
`--project=smoke` (in `preview-smoke.yml`) and `--project=visual`
|
|||
|
|
(in `visual-diff.yml`). Two projects is a structural
|
|||
|
|
requirement, not a preference. Separate directories cleanly
|
|||
|
|
partition `testMatch` and avoid the boot-the-brief failure
|
|||
|
|
mode where `testDir: './tests'` mixes both project's specs
|
|||
|
|
into both projects.
|
|||
|
|
|
|||
|
|
**Routing:** architect-self-ratifiable. The workflows already
|
|||
|
|
made this call; the architect's job is to honor it.
|
|||
|
|
|
|||
|
|
### Decision 4 — first-run visual baselines → **(b) do NOT commit baselines this convoy; document seed-on-Linux workflow + queue follow-up** (operator ratifies)
|
|||
|
|
|
|||
|
|
**Recommendation:** ship the visual spec without a committed
|
|||
|
|
baseline. First CI run of `Screenshot diff` will fail at the
|
|||
|
|
test step (per Playwright's `--update-snapshots=none` semantics,
|
|||
|
|
"missing snapshots cause test failure"), but the workflow's
|
|||
|
|
existing `continue-on-error: true` swallows that and the
|
|||
|
|
comment-on-PR step posts "Visual Diff — view run" with empty
|
|||
|
|
artifacts. Operator then runs `npm run test:visual:update`
|
|||
|
|
in a Linux environment (Docker `mcr.microsoft.com/playwright`
|
|||
|
|
image, or a manually-dispatched workflow with
|
|||
|
|
`--update-snapshots`) and commits the baselines in a separate
|
|||
|
|
follow-up PR (`seed-visual-baselines-on-linux`, queued).
|
|||
|
|
|
|||
|
|
**Investigation:**
|
|||
|
|
|
|||
|
|
- Playwright snapshot file names include the platform suffix:
|
|||
|
|
`<arg>-<browserName>-<platform>.png` (default template). With
|
|||
|
|
a custom `snapshotPathTemplate` we can drop the platform, but
|
|||
|
|
cross-platform mismatches then become silent overwrites — bad
|
|||
|
|
for a multi-OS team.
|
|||
|
|
- Verified via [Playwright docs](https://playwright.dev/docs/test-snapshots)
|
|||
|
|
and `--update-snapshots` flag semantics: default mode is
|
|||
|
|
`missing` (auto-create on first run, fail the test); explicit
|
|||
|
|
`none` (what the workflow uses) NEVER creates and ALWAYS fails
|
|||
|
|
when missing.
|
|||
|
|
- The convoy file's own out-of-scope section says: "Authoring
|
|||
|
|
real visual baselines... is a separate convoy
|
|||
|
|
(`adopt-visual-baselines`?) once UX has stabilized." This
|
|||
|
|
decision matches that intent.
|
|||
|
|
|
|||
|
|
**Rationale:** committing a Mac-generated baseline now would
|
|||
|
|
either (i) fail CI on the very first run because the platform
|
|||
|
|
suffix won't match, or (ii) drop the platform suffix entirely
|
|||
|
|
via `snapshotPathTemplate`, which silently hides platform
|
|||
|
|
drift for any future contributor running on a different OS.
|
|||
|
|
Neither is worth saving the operator one follow-up PR.
|
|||
|
|
Bundling Linux-Docker baseline generation into this convoy
|
|||
|
|
adds a Docker dependency, a `--network=host` workaround on
|
|||
|
|
Mac, and a `BASE_URL` reachability question — all out of
|
|||
|
|
scope per the convoy file.
|
|||
|
|
|
|||
|
|
**Routing:** operator ratifies. Process preference about how
|
|||
|
|
much baseline curation belongs in this convoy vs the
|
|||
|
|
follow-up.
|
|||
|
|
|
|||
|
|
### Decision 5 — ESLint coverage for `tests/` → **no change to `eslint.config.mjs`** (architect-self-ratifiable)
|
|||
|
|
|
|||
|
|
**Decision:** `eslint.config.mjs` is NOT touched in this convoy.
|
|||
|
|
The brief verifies post-implementation by re-running `npm run
|
|||
|
|
lint` and confirming the baseline stays at 128 problems.
|
|||
|
|
|
|||
|
|
**Investigation:** `npx eslint tests/smoke/app.smoke.spec.ts`
|
|||
|
|
exits 0 with zero output against the current config (verified
|
|||
|
|
2026-05-24 on the convoy branch HEAD). The
|
|||
|
|
`eslint-config-next/core-web-vitals` chain in `eslint-config-next@16`
|
|||
|
|
bundles the `typescript-eslint` parser; `.ts` files in the
|
|||
|
|
repo (today: just `next-env.d.ts` plus the smoke spec) are
|
|||
|
|
parsed cleanly without any explicit `tests/**/*.ts`
|
|||
|
|
`globalIgnores` entry. The convoy file flagged this as
|
|||
|
|
"likely to need an ignore" — investigation found it doesn't.
|
|||
|
|
|
|||
|
|
**Routing:** architect-self-ratifiable. Empirical evidence;
|
|||
|
|
no convention question.
|
|||
|
|
|
|||
|
|
### Decision 6 — `test:smoke:local` boot-`next dev` script → **no; ship simple `test:smoke` + `test:visual` + `test:visual:update`** (operator ratifies)
|
|||
|
|
|
|||
|
|
**Recommendation:** add three scripts to `package.json`:
|
|||
|
|
|
|||
|
|
- `"test:smoke": "playwright test --project=smoke"`
|
|||
|
|
- `"test:visual": "playwright test --project=visual"`
|
|||
|
|
- `"test:visual:update": "playwright test --project=visual --update-snapshots"`
|
|||
|
|
|
|||
|
|
Operator's local workflow:
|
|||
|
|
|
|||
|
|
```bash
|
|||
|
|
# Terminal 1
|
|||
|
|
npm run dev
|
|||
|
|
# Terminal 2
|
|||
|
|
BASE_URL=http://localhost:3000 npm run test:smoke
|
|||
|
|
# OR against a deployed preview:
|
|||
|
|
BASE_URL=https://<preview>.vercel.app \
|
|||
|
|
VERCEL_AUTOMATION_BYPASS_SECRET=<value> \
|
|||
|
|
npm run test:smoke
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
**Rationale:**
|
|||
|
|
|
|||
|
|
1. Matches the existing `test` / `test:run` shape — each
|
|||
|
|
script does one thing.
|
|||
|
|
2. Auto-booting `next dev` from a test runner introduces
|
|||
|
|
wait-for-ready / cleanup / port-conflict edge cases
|
|||
|
|
that are fragile across OS.
|
|||
|
|
3. `test:visual:update` is the exact command the visual
|
|||
|
|
workflow's PR-comment text instructs operators to run
|
|||
|
|
when seeding new baselines, so it's a one-line DX win
|
|||
|
|
without adding any logic.
|
|||
|
|
|
|||
|
|
**Routing:** operator ratifies. DX preference about how
|
|||
|
|
much wrapper logic belongs in the test scripts.
|
|||
|
|
|
|||
|
|
## Architecture
|
|||
|
|
|
|||
|
|
### File plan
|
|||
|
|
|
|||
|
|
| File | Action | Purpose |
|
|||
|
|
|---|---|---|
|
|||
|
|
| `package.json` | modified | Add `@playwright/test@^1.60.0` to devDependencies. Add `test:smoke`, `test:visual`, `test:visual:update` scripts. |
|
|||
|
|
| `package-lock.json` | modified | Regenerated by `npm install` (committed in the same commit; required for `npm ci` parity in CI per Risk R7). |
|
|||
|
|
| `playwright.config.js` | new | Root-level ESM config. `testDir: './tests'`, two `projects:` blocks (`smoke` + `visual`) per Decision 3, `use.baseURL` from `process.env.BASE_URL`, `use.extraHTTPHeaders` with CI-gated `x-vercel-protection-bypass` per Decision 2, `snapshotPathTemplate` for the visual project (Risk R2), `timeout: 30_000` + `expect.timeout: 10_000`. Well-commented per AC #8. |
|
|||
|
|
| `tests/smoke/app.smoke.spec.ts` | unchanged | Existing 3-test spec stays as `.ts` per Decision 1. NOT renamed; NOT edited. |
|
|||
|
|
| `tests/visual/homepage.spec.ts` | new | One screenshot spec: `expect(page).toHaveScreenshot('home.png')`. Inline comment documents the seed-on-Linux first-run workflow per Decision 4. |
|
|||
|
|
| `.gitignore` | modified | Add `/playwright-report/`, `/test-results/`, `/.playwright/`. **Do NOT add `tests/visual/__screenshots__/`** — baselines MUST be committed when they exist (boot-the-brief finding). |
|
|||
|
|
|
|||
|
|
### API surface
|
|||
|
|
|
|||
|
|
N/A. No new API routes. Smoke tests hit existing
|
|||
|
|
`pages/api/health.js` which already returns 200 anonymously
|
|||
|
|
with no auth gate and no rate-limit wrapper (verified by
|
|||
|
|
reading the source — 7-line handler, no `getUserFromRequest`,
|
|||
|
|
no `checkAuthRateLimit`). The Vercel bypass header passes
|
|||
|
|
through to API routes because Vercel's preview protection
|
|||
|
|
runs at the platform edge, not inside the function — the
|
|||
|
|
function receives the request as if anonymous.
|
|||
|
|
|
|||
|
|
### Schema diff
|
|||
|
|
|
|||
|
|
N/A. No DB changes.
|
|||
|
|
|
|||
|
|
### Test plan
|
|||
|
|
|
|||
|
|
This convoy IS the test infrastructure. Net new tests:
|
|||
|
|
|
|||
|
|
- `tests/smoke/app.smoke.spec.ts` (already exists, 3 tests):
|
|||
|
|
home renders without 5xx, sign-in page renders, `/api/health`
|
|||
|
|
2xx. Becomes executed-by-CI in this convoy.
|
|||
|
|
- `tests/visual/homepage.spec.ts` (new, 1 test): screenshot of
|
|||
|
|
the homepage at default viewport (1280×720 — Playwright
|
|||
|
|
default). First run fails because no baseline exists; that's
|
|||
|
|
the documented end state of this convoy per Decision 4.
|
|||
|
|
|
|||
|
|
Regression coverage held in place by:
|
|||
|
|
|
|||
|
|
- `npm run test:run` (vitest) still passes 21/21 — the new
|
|||
|
|
`@playwright/test` install does not touch any vitest config or
|
|||
|
|
setup file. Verified by re-running `npm run test:run` after
|
|||
|
|
the implementer's `npm install`.
|
|||
|
|
- `npm run lint` exit code matches the established 128-problem
|
|||
|
|
baseline (see Decision 5).
|
|||
|
|
- `npm run build` still succeeds — Playwright is a devDep, not
|
|||
|
|
bundled into the Next.js build graph.
|
|||
|
|
|
|||
|
|
### Risk list
|
|||
|
|
|
|||
|
|
- **R1 — `request` fixture vs `use.extraHTTPHeaders` propagation.**
|
|||
|
|
The existing smoke spec uses `await request.get(/api/health)`.
|
|||
|
|
Per [Playwright fixtures docs](https://playwright.dev/docs/api/class-fixtures#fixtures-request),
|
|||
|
|
the test-level `request` fixture is an "Isolated APIRequestContext
|
|||
|
|
instance for each test." Per `testOptions.extraHTTPHeaders` docs,
|
|||
|
|
the option applies to **every request** including those issued
|
|||
|
|
by the APIRequestContext (the testOptions surface is shared
|
|||
|
|
between browser context and request fixture creation).
|
|||
|
|
**Mitigation:** the brief's manual-verification step confirms
|
|||
|
|
the health-endpoint test passes against a Vercel preview. If it
|
|||
|
|
401s with the SSO HTML body despite the config, the implementer
|
|||
|
|
files a hotfix to pass headers explicitly in the spec body OR
|
|||
|
|
to add a `request` fixture override in `playwright.config.js`.
|
|||
|
|
Not expected based on the doc evidence, but worth a manual
|
|||
|
|
check.
|
|||
|
|
|
|||
|
|
- **R2 — Snapshot path template misalign with workflow artifact path.**
|
|||
|
|
Playwright's default snapshot location is
|
|||
|
|
`<spec>.spec.ts-snapshots/<arg>-<browser>-<platform>.png`
|
|||
|
|
alongside the spec. The visual workflow uploads
|
|||
|
|
`tests/visual/__screenshots__/` recursively. Without an
|
|||
|
|
explicit `snapshotPathTemplate`, the artifact upload picks up
|
|||
|
|
zero baselines. **Mitigation:** set
|
|||
|
|
`snapshotPathTemplate: 'tests/visual/__screenshots__/{arg}{ext}'`
|
|||
|
|
in `playwright.config.js`. Architect verified the supported
|
|||
|
|
tokens via Playwright `testProject.snapshotPathTemplate`
|
|||
|
|
reference (`{arg}`, `{ext}`, `{projectName}`, `{snapshotDir}`,
|
|||
|
|
`{testDir}`, `{testFileDir}`, `{testFileName}`,
|
|||
|
|
`{testFilePath}` — no `{platform}` token).
|
|||
|
|
|
|||
|
|
- **R3 — Cross-platform snapshot mismatch (Mac dev vs Linux CI).**
|
|||
|
|
With the flat `snapshotPathTemplate` from R2, all baselines
|
|||
|
|
collapse into a single file per snapshot name regardless of
|
|||
|
|
platform. A Mac contributor running `npm run test:visual:update`
|
|||
|
|
locally would overwrite the Linux CI baseline. **Mitigation:**
|
|||
|
|
per Decision 4, do not commit baselines this convoy. Document
|
|||
|
|
the "seed via Docker `mcr.microsoft.com/playwright:v1.60.0-noble`
|
|||
|
|
on Linux" workflow in the visual spec inline comment +
|
|||
|
|
AGENTS.md (doc-writer pass). Queue `seed-visual-baselines-on-linux`
|
|||
|
|
as the follow-up convoy.
|
|||
|
|
|
|||
|
|
- **R4 — `--update-snapshots=none` behavior with missing baselines.**
|
|||
|
|
Verified via Playwright CLI docs: "Possible values are 'all',
|
|||
|
|
'changed', 'missing', and 'none'. Running tests without the
|
|||
|
|
flag defaults to 'missing'; running tests with the flag but
|
|||
|
|
without a value defaults to 'changed'." Mode `none` never
|
|||
|
|
creates snapshots; missing snapshots fail the test. The
|
|||
|
|
visual workflow's `continue-on-error: true` + comment-on-PR
|
|||
|
|
step is the safety net. **Mitigation:** documented behavior;
|
|||
|
|
no config change needed.
|
|||
|
|
|
|||
|
|
- **R5 — `process.env.CI` predicate false negative.** GitHub
|
|||
|
|
Actions sets `CI=true`; Vercel build runtime sets `CI=1`.
|
|||
|
|
Playwright is invoked only from GitHub Actions in this convoy
|
|||
|
|
(the Vercel build does not run Playwright). Strict equality
|
|||
|
|
`process.env.CI === 'true'` is correct for the GitHub Actions
|
|||
|
|
case. If a future workflow invokes Playwright from a different
|
|||
|
|
CI provider, revisit. **Mitigation:** documented in the
|
|||
|
|
config's inline comment.
|
|||
|
|
|
|||
|
|
- **R6 — Bypass secret leakage via Playwright trace HAR.** If a
|
|||
|
|
future change enables `trace: 'on'` (or `retain-on-failure`),
|
|||
|
|
the bypass header lands in the trace.zip HAR payload. The
|
|||
|
|
upload-artifact step then preserves it for 7 days.
|
|||
|
|
**Mitigation:** keep `trace: 'off'` in this convoy. If/when
|
|||
|
|
traces are enabled in a future polish convoy, that convoy MUST
|
|||
|
|
decide on HAR sanitization (e.g. a custom reporter that strips
|
|||
|
|
the `x-vercel-protection-bypass` header from saved traces, or
|
|||
|
|
rotating the bypass token more aggressively).
|
|||
|
|
|
|||
|
|
- **R7 — `npm ci` in CI vs `npm install` locally.** Both
|
|||
|
|
workflows run `npm ci`, which requires
|
|||
|
|
`package-lock.json` to be in sync with `package.json`. The
|
|||
|
|
implementer MUST run `npm install` locally and commit the
|
|||
|
|
regenerated lockfile in the SAME commit (or `npm ci` in CI
|
|||
|
|
will fail with "lockfile out of sync"). **Mitigation:** AC
|
|||
|
|
in the brief calls this out explicitly; the architect's
|
|||
|
|
Boot-the-brief did not run `npm install` itself (read-only
|
|||
|
|
pass) but the requirement is mechanical.
|
|||
|
|
|
|||
|
|
- **R8 — `eslint-config-next` typescript-eslint future drift.**
|
|||
|
|
Verified clean today (`npx eslint tests/smoke/app.smoke.spec.ts`
|
|||
|
|
exit 0). Risk is future drift if `eslint-config-next` ever
|
|||
|
|
drops or restructures the typescript-eslint bundle.
|
|||
|
|
**Mitigation:** the brief verifies post-implementation that
|
|||
|
|
`npm run lint` still hits the 128-problem baseline. If it
|
|||
|
|
grows, investigate before merge — the new errors are most
|
|||
|
|
likely from the `.ts` spec parsing, which would need a
|
|||
|
|
`globalIgnores` entry as a hotfix.
|
|||
|
|
|
|||
|
|
- **R9 — Playwright 1.60.0 freshness (released 2026-05-11, 13
|
|||
|
|
days old at time of this convoy).** No reported critical
|
|||
|
|
regressions in changelog scan. Previous stable 1.59.1 (2026-04-01,
|
|||
|
|
~7 weeks old) is the safer pin. Architect picks `^1.60.0` to
|
|||
|
|
align with the workflow's `npx playwright install --with-deps
|
|||
|
|
chromium` step (downloads the bundled binary matching the
|
|||
|
|
installed package version) and to inherit any 1.60.x patches.
|
|||
|
|
**Mitigation:** if 1.60.0 surfaces regressions in the
|
|||
|
|
implementer's smoke run, downgrade to `^1.59.1` in a hotfix
|
|||
|
|
before merge.
|
|||
|
|
|
|||
|
|
## Decomposition
|
|||
|
|
|
|||
|
|
| Brief # | Title | Files | Depends on | Estimated PR size |
|
|||
|
|
|---|---|---|---|---|
|
|||
|
|
| 1 | Install `@playwright/test`, ship `playwright.config.js`, add visual homepage spec | `package.json`, `package-lock.json`, `playwright.config.js`, `tests/visual/homepage.spec.ts`, `.gitignore` | none | ~120 LOC source diff + lockfile churn |
|
|||
|
|
|
|||
|
|
**Brief count: 1.** Justification:
|
|||
|
|
|
|||
|
|
1. **Total source-diff LOC < 200.** `playwright.config.js` ~70
|
|||
|
|
LOC, visual spec ~20 LOC, `package.json` ~5 LOC,
|
|||
|
|
`.gitignore` ~5 LOC. Lockfile churn is mechanical, not
|
|||
|
|
reviewable.
|
|||
|
|
2. **All files are semantically coupled.** The visual spec
|
|||
|
|
cannot be discovered without `playwright.config.js`
|
|||
|
|
declaring the `visual` project. The config cannot be
|
|||
|
|
loaded without `@playwright/test` in `node_modules`.
|
|||
|
|
`.gitignore` covers artifacts produced by both. Splitting
|
|||
|
|
into separate briefs would force interim states that
|
|||
|
|
either don't compile or don't run.
|
|||
|
|
3. **No parallelizability benefit.** Two implementers cannot
|
|||
|
|
meaningfully work on disjoint subsets of this change set.
|
|||
|
|
4. **One human review is enough.** All changes fit in a
|
|||
|
|
single PR's diff comfortably.
|
|||
|
|
|
|||
|
|
### Slice dependencies (multitask-ready)
|
|||
|
|
|
|||
|
|
```yaml
|
|||
|
|
slice_dependencies:
|
|||
|
|
- brief: 1
|
|||
|
|
depends_on: []
|
|||
|
|
files:
|
|||
|
|
- package.json
|
|||
|
|
- package-lock.json
|
|||
|
|
- playwright.config.js
|
|||
|
|
- tests/visual/homepage.spec.ts
|
|||
|
|
- .gitignore
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
Single brief; no `/multitask` fan-out. Conductor dispatches
|
|||
|
|
serially.
|