Convoy file:
- status: queued -> in-progress
- New § Decisions (post-IA round): 6 decisions investigated.
Architect-self-ratifiable: D2 (CI fail-loud predicate mirrors
rate-limit.js convention), D3 (two projects is a structural
workflow requirement), D5 (no eslint change — verified `npx
eslint tests/smoke/app.smoke.spec.ts` exits 0). Operator-
ratifiable at gate 1: D1 (recommend keep .ts; convention call),
D4 (recommend defer baseline commit to Linux-Docker follow-up;
process call), D6 (recommend simple test:smoke/test:visual
scripts, no auto-boot wrapper; DX call).
- New § Architecture: file plan (5 files), API surface (n/a),
schema diff (n/a), test plan (3 smoke + 1 visual; vitest 21/21
+ lint baseline 128 held), risk list R1-R9 (request fixture
header propagation, snapshotPathTemplate alignment, cross-
platform mismatch, --update-snapshots=none semantics, CI
predicate variants, bypass trace leak, npm ci lockfile sync,
eslint future drift, Playwright 1.60.0 freshness).
- New § Decomposition: 1 brief, ~120 LOC, slice_dependencies
YAML block. Justified single brief because total LOC is
small and all files are semantically coupled (visual spec
depends on visual project in config which depends on the
installed dep).
Brief file (.convoys/adopt-playwright-smoke/brief-1-*.md):
- Goal/Files/Conventions/Acceptance/Manual-verification/
Boot-the-brief findings/Out-of-scope sections matching the
fix-vercel-deployment-protection-in-ci/brief-1 shape.
- 8 Boot-the-brief findings: dep resolution clean, .ts lints
clean, baselines dir MUST NOT be gitignored, extraHTTPHeaders
propagates to request fixture per docs, /api/health is
anonymous, ESM shape required, snapshotPathTemplate token set
has no {platform}, --update-snapshots=none never auto-creates.
- Verbatim playwright.config.js shape with CI fail-loud / dev
warn predicate, snapshotPathTemplate aligned with workflow's
artifact upload path, two projects with testMatch partitioning.
No code changes (read-only architect pass). No push.
Co-authored-by: Cursor <cursoragent@cursor.com>
495 lines
33 KiB
Markdown
495 lines
33 KiB
Markdown
---
|
||
convoy: adopt-playwright-smoke
|
||
brief_number: 1
|
||
depends_on: []
|
||
files:
|
||
- package.json
|
||
- package-lock.json
|
||
- playwright.config.js
|
||
- tests/visual/homepage.spec.ts
|
||
- .gitignore
|
||
---
|
||
|
||
# Brief 1: Install `@playwright/test`, ship `playwright.config.js`, add the homepage visual spec — so the post-PR-17 `Preview smoke` and `Screenshot diff` workflows reach `npx playwright test` and execute against the Vercel preview
|
||
|
||
## Goal (1 sentence)
|
||
|
||
Wire `@playwright/test@^1.60.0` into devDeps, create `playwright.config.js` with two projects (`smoke` + `visual`) and a CI-gated `x-vercel-protection-bypass` header per `playwright.config` Decision 2, add a single `tests/visual/homepage.spec.ts` baseline-bearing screenshot test per Decision 3, add three `npm` scripts per Decision 6, and ignore Playwright's local-run artifacts in `.gitignore` — without renaming or editing the existing `tests/smoke/app.smoke.spec.ts`, without touching `eslint.config.mjs`, and without committing any baseline images (operator seeds those in a follow-up Linux-Docker run per Decision 4).
|
||
|
||
## Files in scope (do not edit anything else)
|
||
|
||
- `package.json` — modified.
|
||
- `package-lock.json` — modified (regenerated by `npm install`; commit in the SAME commit as `package.json` to keep `npm ci` happy in CI per Risk R7).
|
||
- `playwright.config.js` — new (root level).
|
||
- `tests/visual/homepage.spec.ts` — new.
|
||
- `.gitignore` — modified.
|
||
|
||
**Files explicitly out of scope** (do not touch even if it seems related):
|
||
|
||
- `tests/smoke/app.smoke.spec.ts` — exists, stays as-is per Decision 1. NOT renamed to `.js`. NOT edited.
|
||
- `eslint.config.mjs` — empirically verified to parse the existing `.ts` spec cleanly (Decision 5 + Boot-the-brief finding #2 below). NOT touched.
|
||
- `vitest.config.js`, `test/setup.js`, any file under `test/` (vitest's home) — different runner; out of scope.
|
||
- `.github/workflows/preview-smoke.yml`, `.github/workflows/visual-diff.yml`, `.github/workflows/ci.yml` — all workflow YAML is owned by other convoys (PR #17 / PR #16 / `fix-lint-baseline`); zero touches here.
|
||
- `AGENTS.md` § 7 (the seed-on-Linux workflow + the local-run command table) — that's the doc-writer pass at convoy close, NOT this brief.
|
||
- `tests/visual/__screenshots__/` — do NOT create or commit baselines in this convoy per Decision 4. Operator runs `npm run test:visual:update` in a Linux Docker env (`mcr.microsoft.com/playwright:v1.60.0-noble`) and commits in a follow-up `seed-visual-baselines-on-linux` PR.
|
||
- `pages/api/health.js` — already exists and already returns 200 anonymously (verified by reading the 7-line source). Do NOT modify or move.
|
||
- `next.config.js`, `tailwind.config.js`, `postcss.config.js` — unrelated; zero touches.
|
||
|
||
## Conventions to follow
|
||
|
||
### Decisions from the convoy file (cite when implementing)
|
||
|
||
- **Decision 1 (`.convoys/adopt-playwright-smoke.md` § Decisions, post-IA round):** keep `tests/smoke/app.smoke.spec.ts` as `.ts`. New visual spec also `.ts` (`tests/visual/homepage.spec.ts`).
|
||
- **Decision 2:** in `playwright.config.js`, `throw` at config load when `process.env.CI === 'true'` AND `VERCEL_AUTOMATION_BYPASS_SECRET` is missing/empty. In non-CI, `console.warn` once and continue with `extraHTTPHeaders` undefined. Error message names the env var, the rotation command, and points at `AGENTS.md § 7`.
|
||
- **Decision 3:** two projects — `smoke` (`testMatch: 'smoke/**/*.spec.@(ts|js)'`) and `visual` (`testMatch: 'visual/**/*.spec.@(ts|js)'`). Both share the same `use:` block (no per-project `use:` overrides).
|
||
- **Decision 4:** do NOT commit baselines. First CI run of `Screenshot diff` will fail at the test step; the existing `continue-on-error: true` swallows the failure and the comment-on-PR step posts "Visual Diff — view run" with empty artifacts. That's the documented end state of this brief.
|
||
- **Decision 5:** no `eslint.config.mjs` change. Verified empirically that `npx eslint tests/smoke/app.smoke.spec.ts` exits 0 against the current config; the new `tests/visual/homepage.spec.ts` is structurally identical and will also pass.
|
||
- **Decision 6:** add three scripts — `test:smoke`, `test:visual`, `test:visual:update`. No auto-boot of `next dev`.
|
||
|
||
### Repo conventions (cite + match)
|
||
|
||
- **ESM module style.** `package.json` has `"type": "module"`. `vitest.config.js` and `next.config.js` are both ESM (`import { defineConfig } from '...'; export default defineConfig({...})`). `playwright.config.js` MUST match this shape:
|
||
```js
|
||
import { defineConfig } from '@playwright/test';
|
||
export default defineConfig({ ... });
|
||
```
|
||
- **No-go zones (`.cursor/rules/no-go-zones.mdc`).** None of the files in scope are listed. `playwright.config.js` does not exist yet. `tests/visual/` does not exist yet. `package.json` / `package-lock.json` / `.gitignore` are all editable per established convoy precedent (`bump-next-js`, `fix-auth-bypass`).
|
||
- **Secret-handling discipline (AGENTS.md § 7):**
|
||
- NEVER `console.log` / `echo` / write to a file any string containing `process.env.VERCEL_AUTOMATION_BYPASS_SECRET`.
|
||
- The `console.warn` branch (Decision 2 dev path) must say "VERCEL_AUTOMATION_BYPASS_SECRET unset" — NOT print the value.
|
||
- The fail-loud branch (Decision 2 CI path) `throw`s a string error message; the message names the env var by name but does NOT echo any value.
|
||
- **Style match.** Two close precedents for ESM config files: `vitest.config.js` (heavy inline comments explaining each non-default choice) and `next.config.js` (terse). `playwright.config.js` should follow `vitest.config.js`'s commented style because the convoy's AC #8 requires it ("`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").
|
||
|
||
## Acceptance criteria
|
||
|
||
### `package.json`
|
||
|
||
- [ ] Add `"@playwright/test": "^1.60.0"` to `devDependencies` (alphabetical position — between `@neondatabase/serverless` is in `dependencies`, so in `devDependencies` it lands between `@testing-library/react` and `autoprefixer`; verify alphabetization is preserved).
|
||
|
||
Pin rationale: `^1.60.0` accepts patch updates (1.60.x), matches the workflow's `npx playwright install --with-deps chromium` (which downloads the bundled browser matching the installed package version), and 1.60.0 is 13 days old at time of writing — fresh but not bleeding-edge. If `npm install` resolves to a newer 1.60.x patch, that's expected.
|
||
|
||
- [ ] Add three scripts to the `scripts` block, between `test:run` and the closing brace:
|
||
|
||
```json
|
||
"test:smoke": "playwright test --project=smoke",
|
||
"test:visual": "playwright test --project=visual",
|
||
"test:visual:update": "playwright test --project=visual --update-snapshots"
|
||
```
|
||
|
||
Notes:
|
||
- Use bare `playwright test`, NOT `npx playwright test`. With the dep installed locally, npm scripts resolve `playwright` from `node_modules/.bin` automatically. Matches the existing `test` / `test:run` shape (which uses bare `vitest`, not `npx vitest`).
|
||
- `test:visual:update` is the exact command the visual workflow's PR-comment text instructs operators to run — keep the verbatim string match so a future operator can copy-paste from the comment.
|
||
- Do NOT add `test:e2e`, `test:smoke:local`, or any auto-boot variant. Decision 6 explicitly rejected those.
|
||
|
||
- [ ] No other changes to `package.json`. `dependencies`, `name`, `version`, `private`, `type` all stay byte-identical.
|
||
|
||
### `package-lock.json`
|
||
|
||
- [ ] Regenerated by running `npm install` after the `package.json` edit. Commit the resulting lockfile in the SAME commit as the `package.json` change (Risk R7 in the convoy file). The diff will be large (Playwright pulls many transitive deps) but is mechanical; do NOT hand-edit it.
|
||
|
||
- [ ] Verify lockfile sync by running `npm ci` locally after commit: it should succeed with exit 0. If it errors with "Missing: ... from lock file" or "Invalid: lock file's ... does not satisfy package.json", the regeneration is incomplete — re-run `npm install` and re-commit.
|
||
|
||
### `playwright.config.js` (new file at repo root)
|
||
|
||
- [ ] Verbatim shape (commented for AC #8; the implementer is free to tighten wording but every non-obvious choice MUST have a one-line explanation):
|
||
|
||
```js
|
||
// Playwright config for the post-PR-17 `Preview smoke` and
|
||
// `Screenshot diff` workflows. ESM per the repo's
|
||
// `"type": "module"` setting in package.json. Companion docs:
|
||
// `.convoys/adopt-playwright-smoke.md` (Decisions 1-6),
|
||
// AGENTS.md § 7 (Vercel preview bypass conventions).
|
||
|
||
import { defineConfig } from '@playwright/test';
|
||
|
||
const BASE_URL = process.env.BASE_URL ?? 'http://localhost:3000';
|
||
const BYPASS_SECRET = process.env.VERCEL_AUTOMATION_BYPASS_SECRET;
|
||
|
||
// `CI === 'true'` is the canonical GitHub Actions signal (set by
|
||
// the runner). Playwright config has no Next.js context, so
|
||
// `NODE_ENV` is not reliable here. Mirrors the rate-limit.js
|
||
// fail-closed pattern documented in AGENTS.md Gotcha #12.
|
||
const IS_CI = process.env.CI === 'true';
|
||
|
||
if (IS_CI && !BYPASS_SECRET) {
|
||
// Fail loud in CI per Decision 2. The workflow's `env:` block
|
||
// (preview-smoke.yml line 101, visual-diff.yml line 85) maps
|
||
// `secrets.VERCEL_AUTOMATION_BYPASS_SECRET` into the process
|
||
// env; if it's empty here, the secret is unseeded or the
|
||
// workflow YAML drift broke the mapping.
|
||
throw new Error(
|
||
'VERCEL_AUTOMATION_BYPASS_SECRET is required in CI to reach ' +
|
||
'Vercel-Protection-protected preview deployments. ' +
|
||
'Reseed via: gh secret set VERCEL_AUTOMATION_BYPASS_SECRET --body "<value>". ' +
|
||
'See AGENTS.md § 7 for the full plumbing context.'
|
||
);
|
||
}
|
||
|
||
if (!BYPASS_SECRET && !IS_CI) {
|
||
// Dev fallback per Decision 2 — warn once at config load,
|
||
// proceed without the header. Local runs target localhost
|
||
// (no preview protection) or a non-protected URL.
|
||
// eslint-disable-next-line no-console -- intentional one-shot warning at config load
|
||
console.warn(
|
||
'[playwright.config] VERCEL_AUTOMATION_BYPASS_SECRET unset — ' +
|
||
'running without the Vercel bypass header. Targets a non-protected ' +
|
||
'URL (e.g. http://localhost:3000). Hitting a protected preview without ' +
|
||
'this header will return Vercel\'s SSO challenge page.'
|
||
);
|
||
}
|
||
|
||
export default defineConfig({
|
||
// Both `tests/smoke/` and `tests/visual/` live under `tests/`.
|
||
// Project-level `testMatch` (below) partitions them so the
|
||
// two workflows (`--project=smoke` and `--project=visual`)
|
||
// each see only the specs they should run.
|
||
testDir: './tests',
|
||
|
||
// Smoke + visual specs are independent; parallelism within a
|
||
// single spec adds no value here and would complicate the
|
||
// per-test screenshot baseline lifecycle.
|
||
fullyParallel: false,
|
||
workers: IS_CI ? 1 : undefined,
|
||
|
||
// One retry in CI handles transient Vercel preview flakes
|
||
// (cold-start, DNS propagation). Local: zero retries — fail
|
||
// fast so the dev sees the issue immediately.
|
||
retries: IS_CI ? 1 : 0,
|
||
|
||
// 30s per test is plenty for the 3 smoke checks + 1 visual
|
||
// screenshot. The convoy file's success metric is < 5min
|
||
// total workflow runtime; per-test 30s is well inside that.
|
||
timeout: 30_000,
|
||
expect: { timeout: 10_000 },
|
||
|
||
// List reporter in dev for human readability; add HTML in CI
|
||
// so the `Upload Playwright report on failure` step
|
||
// (preview-smoke.yml line 104) has a populated `playwright-report/`
|
||
// to upload. `open: 'never'` keeps the HTML from auto-launching
|
||
// a browser tab in headless CI.
|
||
reporter: IS_CI ? [['list'], ['html', { open: 'never' }]] : 'list',
|
||
|
||
// Visual baselines live at `tests/visual/__screenshots__/<arg>{ext}`.
|
||
// Workflow `visual-diff.yml` line 94 uploads this exact path as the
|
||
// artifact — keep them aligned. {arg} is the snapshot name from
|
||
// `toHaveScreenshot('home.png')` without the extension; {ext} is
|
||
// the extension with the leading dot.
|
||
//
|
||
// Cross-platform note (Risk R3): this template drops Playwright's
|
||
// default `-<browser>-<platform>` suffix. That means a Mac dev
|
||
// running `npm run test:visual:update` overwrites the Linux-CI
|
||
// baseline. Per Decision 4, we don't commit baselines this convoy;
|
||
// operator seeds via `mcr.microsoft.com/playwright:v1.60.0-noble`
|
||
// Docker on Linux. Multi-platform support is the
|
||
// `seed-visual-baselines-on-linux` follow-up convoy's job.
|
||
snapshotPathTemplate: 'tests/visual/__screenshots__/{arg}{ext}',
|
||
|
||
use: {
|
||
baseURL: BASE_URL,
|
||
// Headers apply to BOTH browser `page.goto(...)` calls AND the
|
||
// test-level `request` fixture's APIRequestContext (verified
|
||
// against Playwright docs: `testOptions.extraHTTPHeaders` is
|
||
// shared between browser context and APIRequestContext
|
||
// construction). This is why `tests/smoke/app.smoke.spec.ts`'s
|
||
// third test (`request.get('/api/health')`) reaches the
|
||
// protected preview without re-injecting the header in the
|
||
// spec body. If a future hotfix shows the header NOT
|
||
// propagating to APIRequestContext, see Risk R1 in the
|
||
// convoy file's Architecture section.
|
||
extraHTTPHeaders: BYPASS_SECRET
|
||
? { 'x-vercel-protection-bypass': BYPASS_SECRET }
|
||
: undefined,
|
||
// Trace OFF this convoy. Enabling it would land the bypass
|
||
// header in the HAR payload (Risk R6); a future polish convoy
|
||
// owns the trace-on + HAR-sanitization decision.
|
||
trace: 'off',
|
||
screenshot: 'off',
|
||
video: 'off',
|
||
},
|
||
|
||
projects: [
|
||
{
|
||
// `Preview smoke` workflow invokes `--project=smoke` (per
|
||
// preview-smoke.yml line 98). The testMatch keeps the
|
||
// visual specs out of this project.
|
||
name: 'smoke',
|
||
testMatch: 'smoke/**/*.spec.@(ts|js)',
|
||
},
|
||
{
|
||
// `Screenshot diff` workflow invokes `--project=visual`
|
||
// (per visual-diff.yml line 82). The testMatch keeps the
|
||
// smoke specs out of this project (so a `--project=visual`
|
||
// run doesn't redundantly execute the smoke tests).
|
||
name: 'visual',
|
||
testMatch: 'visual/**/*.spec.@(ts|js)',
|
||
},
|
||
],
|
||
});
|
||
```
|
||
|
||
Two things the implementer can vary without breaking AC:
|
||
|
||
1. Comment wording — the substantive choices (predicate, error message contents, path template) must stay; the explanatory prose can be tightened.
|
||
2. Property ordering inside `defineConfig({...})` — Playwright doesn't care; alphabetical or grouped-by-concern are both fine.
|
||
|
||
Things the implementer MUST NOT change:
|
||
|
||
- The `IS_CI` predicate (`=== 'true'`, not `=== 'true' || === '1'`).
|
||
- The `throw` vs `console.warn` branch logic.
|
||
- The `snapshotPathTemplate` value (the workflow's artifact upload path depends on it).
|
||
- The two project names (`smoke`, `visual`) — workflow YAML invokes them verbatim.
|
||
- The two `testMatch` patterns — partitioning is the whole reason for two projects.
|
||
- The `trace: 'off'` setting (Risk R6).
|
||
- `BYPASS_SECRET ? { ... } : undefined` ternary — passing `extraHTTPHeaders: { 'x-vercel-protection-bypass': undefined }` would still send the header with the literal string `'undefined'`, which 401s loudly. The conditional is load-bearing.
|
||
|
||
### `tests/visual/homepage.spec.ts` (new file)
|
||
|
||
- [ ] Verbatim shape:
|
||
|
||
```ts
|
||
import { test, expect } from '@playwright/test';
|
||
|
||
/**
|
||
* Visual baseline for the public homepage.
|
||
*
|
||
* FIRST RUN (no committed baseline yet):
|
||
* The Screenshot diff workflow runs `playwright test --project=visual
|
||
* --update-snapshots=none` (per .github/workflows/visual-diff.yml). With
|
||
* no baseline file at `tests/visual/__screenshots__/home.png` AND the
|
||
* `none` flag, this test FAILS — and that's the documented end state of
|
||
* the `adopt-playwright-smoke` convoy (Decision 4 in
|
||
* `.convoys/adopt-playwright-smoke.md`). The workflow's
|
||
* `continue-on-error: true` swallows the failure and the comment-on-PR
|
||
* step posts "Visual Diff — view run" with empty artifacts.
|
||
*
|
||
* SEEDING THE BASELINE (post-merge follow-up):
|
||
* Run `npm run test:visual:update` in a Linux environment so the
|
||
* generated PNG matches what CI will produce. The cleanest path is the
|
||
* Playwright Docker image:
|
||
*
|
||
* docker run --rm -v "$PWD":/work -w /work \
|
||
* mcr.microsoft.com/playwright:v1.60.0-noble \
|
||
* sh -c "npm ci && BASE_URL=https://<preview>.vercel.app \
|
||
* VERCEL_AUTOMATION_BYPASS_SECRET=<value> \
|
||
* npm run test:visual:update"
|
||
*
|
||
* Then commit `tests/visual/__screenshots__/home.png`. This is tracked
|
||
* as the `seed-visual-baselines-on-linux` follow-up convoy.
|
||
*/
|
||
const BASE = process.env.BASE_URL ?? 'http://localhost:3000';
|
||
|
||
test.describe('visual: public homepage', () => {
|
||
test('home renders consistently against baseline', async ({ page }) => {
|
||
await page.goto(BASE);
|
||
await expect(page).toHaveScreenshot('home.png');
|
||
});
|
||
});
|
||
```
|
||
|
||
Notes:
|
||
|
||
- `BASE` constant mirrors the existing `tests/smoke/app.smoke.spec.ts` shape (line 11). Don't read `process.env.BASE_URL` inside the test body.
|
||
- Snapshot name is the literal string `'home.png'`. The `snapshotPathTemplate` from `playwright.config.js` resolves this to `tests/visual/__screenshots__/home.png`.
|
||
- `test.describe` block name `'visual: public homepage'` matches the smoke spec's naming pattern (`'smoke: app boots and core pages render'`).
|
||
- Default viewport (1280×720) is fine for a first-pass baseline; do NOT set `viewport:` overrides this convoy.
|
||
- Do NOT add `await page.waitForLoadState('networkidle')` — networkidle is unreliable on a JS-heavy Next.js app and causes false flakes. `page.goto` already waits for `load` by default; that's enough for a homepage baseline.
|
||
- Do NOT add `{ fullPage: true }` to `toHaveScreenshot`. Default (viewport-only) keeps the baseline file small (~50KB) and easier to review on PRs.
|
||
|
||
### `.gitignore`
|
||
|
||
- [ ] Add the following block AFTER the existing `.code-review-graph/` line (current line 39):
|
||
|
||
```gitignore
|
||
|
||
# Playwright test runner artifacts (generated on every local run;
|
||
# never committed). Baselines under `tests/visual/__screenshots__/`
|
||
# are EXPLICITLY NOT ignored — they must be committed when they exist.
|
||
/playwright-report/
|
||
/test-results/
|
||
/.playwright/
|
||
```
|
||
|
||
Three rules:
|
||
|
||
1. `/playwright-report/` — the HTML reporter's output (config `reporter` block).
|
||
2. `/test-results/` — Playwright's default `--output` directory for trace/screenshot/video on failure.
|
||
3. `/.playwright/` — Playwright's local browser cache and other internal state.
|
||
|
||
Do NOT add `tests/visual/__screenshots__/` to `.gitignore`. Baselines are committed artifacts (per Decision 4 they don't exist yet, but when they do, they MUST be tracked). This is a boot-the-brief finding — see Finding 3 below.
|
||
|
||
### Cross-file checks
|
||
|
||
- [ ] **No secret leaks.** Grep before commit:
|
||
```bash
|
||
rg -i 'console\.log.*VERCEL_AUTOMATION_BYPASS_SECRET' playwright.config.js tests/
|
||
```
|
||
Expected: zero matches. The two intentional references (the `if (IS_CI && !BYPASS_SECRET)` `throw` and the `if (!BYPASS_SECRET && !IS_CI)` `console.warn`) name the env var but do NOT echo its value.
|
||
- [ ] **No baseline files committed.** Grep before commit:
|
||
```bash
|
||
ls tests/visual/__screenshots__/ 2>/dev/null && echo "FAIL: directory exists with files; do NOT commit" || echo "OK: no baselines"
|
||
```
|
||
Expected: `OK: no baselines`. If you ran `npm run test:visual:update` locally to verify the spec resolves, delete the generated PNG before commit.
|
||
- [ ] **`npm run test:run` exit 0** (vitest baseline unchanged):
|
||
```bash
|
||
npm run test:run
|
||
```
|
||
Expected: `Tests 21 passed (21)` per the existing vitest suite (16 auth + 5 Layout). If any vitest test fails, investigate before merge — this brief should be a pure additive change with no vitest impact.
|
||
- [ ] **`npm run lint` exit code unchanged** (Decision 5):
|
||
```bash
|
||
npm run lint 2>&1 | tail -3
|
||
```
|
||
Expected: `✖ 128 problems (81 errors, 47 warnings)` — the established baseline. If it grows by 1-2 new problems from the new visual spec, investigate (Risk R8); the architect's empirical check on the existing `.ts` smoke spec showed `exit 0` so the new spec should be silent too.
|
||
- [ ] **`npm run build` exit 0** — Playwright is a devDep and should not affect the Next.js build graph. Run as a smoke check:
|
||
```bash
|
||
npm run build 2>&1 | tail -5
|
||
```
|
||
Expected: build success (Turbopack compile ~1-2s, 23 static pages + 47 API routes per the post-`bump-next-js` baseline).
|
||
- [ ] **Diff hygiene.** `git diff main..HEAD --stat` should show only the 5 files listed in the brief frontmatter. No whitespace-only changes elsewhere.
|
||
|
||
### Acceptance criterion #1 — end-state behavior
|
||
|
||
After this brief lands on the convoy branch and a Vercel preview deployment is published for the PR:
|
||
|
||
- [ ] **`Preview smoke` workflow:** wait-action succeeds in ≤90s (already proven by PR #17 at 194ms); `npm ci` succeeds; `npx playwright install --with-deps chromium` succeeds (~30-60s); `Run smoke tests` step REACHES `npx playwright test --project=smoke`; **3 smoke tests execute against the Vercel preview**. Pass/fail outcome:
|
||
- **Most likely PASS**: home renders (existing pages don't 5xx), `/login` renders (existing page), `/api/health` returns 2xx (verified handler). If any of these fail, the failure is a real signal — investigate before declaring the brief incomplete.
|
||
- Total workflow runtime: < 5 minutes (convoy success metric).
|
||
- [ ] **`Screenshot diff` workflow:** wait-action succeeds; `npx playwright install` succeeds; `Capture screenshots (PR)` step REACHES `npx playwright test --project=visual --update-snapshots=none`. **The single visual test FAILS** because no baseline exists. `continue-on-error: true` swallows the failure. `Upload screenshots + diffs` step uploads `tests/visual/__screenshots__/` (empty) + `test-results/` (contains the failure detail). `Comment on PR with diff link` step posts "## Visual Diff" comment with the run URL. **This is the documented end state of this convoy** per Decision 4 — operator follows up with the seed-on-Linux PR.
|
||
- [ ] **The bypass secret does NOT appear in any line of either workflow's run log.** Same verification pattern as the `fix-vercel-deployment-protection-in-ci` brief (download logs, grep for first 8 chars of the secret value, expect zero hits). GitHub Actions auto-masks registered secrets; this brief's contribution (Decision 2's `throw` and `console.warn` branches) does not interpolate the secret value into any string.
|
||
|
||
## Manual verification (in addition to CI on push)
|
||
|
||
Run these in order. Paste relevant output (with secrets redacted) into the PR description.
|
||
|
||
- [ ] **Local install + lockfile parity.**
|
||
```bash
|
||
npm install
|
||
npm ci # second run, verify lockfile is in sync
|
||
```
|
||
Expected: both succeed exit 0. `npm ci` is the critical one — if it errors, the lockfile is out of sync and the implementer needs to re-run `npm install` and re-commit.
|
||
|
||
- [ ] **Config loads cleanly.**
|
||
```bash
|
||
npx playwright --version
|
||
npx playwright test --list --project=smoke
|
||
npx playwright test --list --project=visual
|
||
```
|
||
Expected:
|
||
- `Version 1.60.x` (or later 1.60 patch).
|
||
- `--list --project=smoke` enumerates 3 tests from `tests/smoke/app.smoke.spec.ts`.
|
||
- `--list --project=visual` enumerates 1 test from `tests/visual/homepage.spec.ts`.
|
||
If `--list` shows specs in the wrong project, the `testMatch` patterns are wrong — fix before commit.
|
||
|
||
- [ ] **Config Decision-2 fail-loud branch.** Simulate CI without the secret:
|
||
```bash
|
||
unset VERCEL_AUTOMATION_BYPASS_SECRET
|
||
CI=true npx playwright test --list --project=smoke
|
||
```
|
||
Expected: exit code 1, error includes "VERCEL_AUTOMATION_BYPASS_SECRET is required in CI" and the `gh secret set` rotation command. If it does NOT error, the Decision 2 logic is wrong.
|
||
|
||
- [ ] **Config Decision-2 warn-and-continue branch.** Simulate dev without the secret:
|
||
```bash
|
||
unset VERCEL_AUTOMATION_BYPASS_SECRET
|
||
unset CI
|
||
npx playwright test --list --project=smoke
|
||
```
|
||
Expected: stderr includes the `[playwright.config] VERCEL_AUTOMATION_BYPASS_SECRET unset — ...` warning, command proceeds and lists tests with exit 0.
|
||
|
||
- [ ] **Smoke spec runs against localhost.** In one terminal:
|
||
```bash
|
||
npm run dev
|
||
```
|
||
In another:
|
||
```bash
|
||
BASE_URL=http://localhost:3000 npm run test:smoke
|
||
```
|
||
Expected: 3 tests pass against the local Next.js dev server. If any fail locally, the spec body has a real issue OR a local config drift — investigate before pushing.
|
||
|
||
- [ ] **Visual spec fail-on-missing-baseline behavior.** Against localhost:
|
||
```bash
|
||
BASE_URL=http://localhost:3000 npx playwright test --project=visual --update-snapshots=none
|
||
```
|
||
Expected: 1 test FAILS with "A snapshot doesn't exist at tests/visual/__screenshots__/home.png". This is the desired Decision-4 behavior. If you accidentally generate a baseline locally with `--update-snapshots`, delete it before commit:
|
||
```bash
|
||
rm -rf tests/visual/__screenshots__/
|
||
```
|
||
|
||
- [ ] **`.gitignore` actually ignores the right things.** After a local Playwright run (which creates `playwright-report/` and `test-results/`):
|
||
```bash
|
||
git status --short
|
||
```
|
||
Expected: no `playwright-report/` or `test-results/` entries. If they appear, the `.gitignore` entries are wrong (missing leading `/` or wrong directory name).
|
||
|
||
- [ ] **No bypass-secret leak in spec output.** After running `npm run test:smoke` with the bypass var set:
|
||
```bash
|
||
VERCEL_AUTOMATION_BYPASS_SECRET=fake-secret-value BASE_URL=http://localhost:3000 \
|
||
npm run test:smoke 2>&1 | grep -i 'fake-secret-value' && echo "FAIL: secret leaked" || echo "OK: secret not in output"
|
||
```
|
||
Expected: `OK: secret not in output`. (`fake-secret-value` is just a probe string for the grep; the real secret never enters this command.)
|
||
|
||
- [ ] **Push and observe the first workflow run.** From `convoy/adopt-playwright-smoke`:
|
||
```bash
|
||
git push -u origin HEAD
|
||
```
|
||
Then watch both workflows. Expect the end-state described in Acceptance criterion #1 above: smoke passes (or fails on a real assertion), visual fails on missing baseline + posts the comment.
|
||
|
||
## Boot-the-brief findings (preempted by the architect; do not re-investigate)
|
||
|
||
### Finding 1 — `@playwright/test@^1.60.0` resolves cleanly against the current dep tree
|
||
|
||
`npm view @playwright/test version` → `1.60.0` (released 2026-05-11). No peer-dep conflicts with the existing `react@^18.3.1` / `next@^16.2.6` / `eslint@^9.39.4` graph (Playwright has no React or Next peers). The bundled Chromium binary downloaded by `npx playwright install --with-deps chromium` (already in both workflow steps — see `preview-smoke.yml` line 95 and `visual-diff.yml` line 79) matches the installed package version, so no version drift between the JS API and the browser binary.
|
||
|
||
### Finding 2 — ESLint already parses `.ts` test files cleanly
|
||
|
||
Verified 2026-05-24 on the convoy branch HEAD (`c8f1541`):
|
||
|
||
```bash
|
||
$ npx eslint tests/smoke/app.smoke.spec.ts
|
||
$ echo $?
|
||
0
|
||
```
|
||
|
||
Zero output, exit 0. The `eslint-config-next/core-web-vitals` chain bundled by `eslint-config-next@16` carries `typescript-eslint` as a hard dep (one of the reasons `typescript@^5.9.3` is in `devDependencies` per AGENTS.md Gotcha #9). The new `tests/visual/homepage.spec.ts` is structurally identical to the existing smoke spec (same imports, same patterns) so will also lint clean. **No `eslint.config.mjs` change is needed** (Decision 5).
|
||
|
||
### Finding 3 — `tests/visual/__screenshots__/` MUST NOT be in `.gitignore`
|
||
|
||
Default Playwright snapshot behavior: baselines are committed source-of-truth (visual regression depends on having a known-good reference). The convoy file's success metric requires the visual workflow's `Upload screenshots + diffs` step (visual-diff.yml line 88-96) to find files at `tests/visual/__screenshots__/` once baselines exist. Adding the path to `.gitignore` would silently break that step in any future PR that touches `pages/**` after baselines are committed in the follow-up convoy.
|
||
|
||
### Finding 4 — `extraHTTPHeaders` applies to the test-level `request` fixture
|
||
|
||
Per [Playwright fixtures docs](https://playwright.dev/docs/api/class-fixtures#fixtures-request), the test-level `request` fixture is "Isolated APIRequestContext instance for each test." Per [testOptions docs](https://playwright.dev/docs/api/class-testoptions), `extraHTTPHeaders` is "An object containing additional HTTP headers to be sent with **every request**." The testOptions surface is shared between browser context and APIRequestContext construction — the `request` fixture inherits the header. **This is why `tests/smoke/app.smoke.spec.ts`'s third test (`request.get('/api/health')`) reaches the protected preview without re-injecting the header in the spec body.** See Risk R1 in the convoy file's Architecture section for the hotfix path if this propagation breaks empirically.
|
||
|
||
### Finding 5 — `pages/api/health.js` returns 200 anonymously
|
||
|
||
The 7-line handler does NOT call `getUserFromRequest`, does NOT call `checkAuthRateLimit`, and does NOT gate on `req.method`. The Vercel preview's edge-protection layer terminates above the function: a request with the bypass header reaches the function as if anonymous, and the function returns `{ status: 'ok', ... }` with HTTP 200. The third smoke test (`request.get('/api/health')`) will pass cleanly against a Vercel preview, given the `extraHTTPHeaders` plumb (Finding 4).
|
||
|
||
### Finding 6 — Repo `"type": "module"` requires ESM-shape `playwright.config.js`
|
||
|
||
`package.json` line 5 declares `"type": "module"`. `vitest.config.js` and `next.config.js` both follow the ESM `import {...} from '...'; export default ...` shape. CommonJS-style `module.exports = { ... }` in `playwright.config.js` would throw at load time with `ReferenceError: module is not defined in ES module scope`. The spec'd `playwright.config.js` matches the ESM shape — do NOT regress to CommonJS.
|
||
|
||
### Finding 7 — `snapshotPathTemplate` token reference
|
||
|
||
Per [Playwright `testProject.snapshotPathTemplate` docs](https://playwright.dev/docs/api/class-testproject#test-project-snapshot-path-template), supported tokens are: `{arg}`, `{ext}`, `{projectName}`, `{snapshotDir}`, `{testDir}`, `{testFileDir}`, `{testFileName}`, `{testFilePath}`. **There is no `{platform}` or `{browserName}` token** — the default platform-suffix-in-filename behavior is built into the default template, and any custom template loses it. This is the structural reason for Risk R3 (cross-platform mismatch) and feeds Decision 4 (defer baseline commit to a Linux-Docker follow-up convoy).
|
||
|
||
### Finding 8 — `--update-snapshots=none` semantics
|
||
|
||
Per [Playwright CLI docs](https://playwright.dev/docs/test-cli): "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` (what `visual-diff.yml` line 82 uses) NEVER creates snapshots; missing snapshots fail the test. The workflow's `continue-on-error: true` + comment-on-PR step is the documented safety net. **First CI run of this brief on a touching-`pages/**` PR will produce a "Visual Diff — view run" comment with empty artifacts; that's the desired Decision-4 end state.**
|
||
|
||
## Out of scope (do not do these)
|
||
|
||
- [ ] Do not rename `tests/smoke/app.smoke.spec.ts` to `.js` (Decision 1).
|
||
- [ ] Do not commit any baseline image to `tests/visual/__screenshots__/` (Decision 4).
|
||
- [ ] Do not edit `eslint.config.mjs` (Decision 5 + Finding 2).
|
||
- [ ] Do not edit any `.github/workflows/*.yml` file (those are owned by other convoys; the brief's job is to MAKE the YAML's `npx playwright test` invocation work, not to modify the YAML).
|
||
- [ ] Do not edit `AGENTS.md` (doc-writer pass at convoy close owns § 7 updates and the seed-on-Linux instructions).
|
||
- [ ] Do not add a `test:smoke:local` or `test:e2e` wrapper script that boots `next dev` automatically (Decision 6).
|
||
- [ ] Do not add a `vitest` watch/setup change to support Playwright — the two runners stay independent (Test plan section of the convoy file).
|
||
- [ ] Do not enable `trace: 'on'` or `trace: 'retain-on-failure'` in `playwright.config.js`'s `use:` block (Risk R6).
|
||
- [ ] Do not add a `webServer:` block to `playwright.config.js` (auto-starts Next.js — explicitly rejected by Decision 6).
|
||
- [ ] Do not pin `@playwright/test` to an exact version (`1.60.0` without the `^`) — patch upgrades are desired (Risk R9 mitigation path).
|
||
- [ ] Do not add a `tsconfig.json` to the repo just because the new spec is `.ts`. Lint already accepts it; tsc-noEmit is not run in CI; the JS-only repo policy (AGENTS.md Gotcha #9) explicitly defers TypeScript adoption to a separate convoy.
|
||
- [ ] Do not bump `node-version: '20'` in any workflow (out of scope; Vercel default is 20).
|
||
- [ ] Do not run `npm audit fix` as part of this brief. If `npm install` surfaces audit warnings, note them in the PR description but do NOT take action — audit churn is a separate concern.
|
||
|
||
## Rationale (≤3 sentences)
|
||
|
||
The post-PR-17 workflows already invoke `npx playwright test --project={smoke,visual}` and already export `BASE_URL` + `VERCEL_AUTOMATION_BYPASS_SECRET` to the test step's `env:` — this brief is the small bridge that makes those invocations actually find a config, a dep, and a runnable visual spec. Bundling the visual spec with the config in one brief is the right call because they're semantically coupled (the visual spec depends on the `visual` project being declared in the config, which depends on `@playwright/test` being installed); splitting would force interim states that don't compile or run. Decision 4 (no baselines now) and Decision 6 (no auto-boot wrapper) explicitly keep the surface area small so the convoy ships in one PR; baseline curation and DX wrappers are queued as follow-up convoys (`seed-visual-baselines-on-linux`, possibly `adopt-test-smoke-local`) that operators can take or leave based on actual usage friction.
|