Playwright E2E Review — gc.platform

Audit Playwright test code against the suite's four hard rules (from tests/playwright/CLAUDE.md) and its strict-TypeScript bar. This skill is read-only: produce findings the author can act on, don't patch. If the user wants the fixes applied, hand the findings to the playwright-e2e-authoring skill or make the edits in a separate, explicit step.

Path convention: All paths in this document are relative to tests/playwright/ (the test suite root).

When to activate

• Reviewing new or changed .spec.ts, *.page.ts, or *.fixture.ts files.
• A pre-merge / pre-commit check of E2E test changes.
• "Does this follow the Playwright conventions?" / "audit these tests".

Scope the review

Default to the changed files when there's a diff, otherwise the path the user names:

cd tests/playwright
git diff --name-only main...HEAD -- tests support   # changed test files

Read each file fully before judging — context matters (e.g. a page.locator('tbody tr') inside a POM is allowed; the same inside a spec is not).

The checklist

Rule 1 — No mocks (Critical)

The suite's entire value is that it hits the real stack. Any simulation is a Critical finding.

grep -rnE "page\.route|context\.route|waitForTimeout|mockServiceWorker|msw|sinon|nock|page\.fulfill" tests support --include='*.ts'

(Or use the harness Grep tool.)

• page.route() / context.route() / route.fulfill() → network interception. Forbidden.
• page.waitForTimeout(...) or any arbitrary sleep → flaky and banned; replace with a web-first assertion on the awaited state.
• MSW / sinon / nock / stub libraries, hardcoded response fixtures, fake data files → forbidden.

Admitted exception: A single page.route() with times: 1 is allowed only to force a failure shape the real stack cannot produce deterministically (e.g. a 401, or .abort()), scoped to one URL pattern, carrying a JSDoc justification. Link to ../playwright-e2e-authoring/references/spec.md#admitted-exceptions for examples and constraints. If the route stubs business data, simulates success, or widens beyond one request — Critical.

Rule 2 — Page Object Model (High)

grep -rnE "(page|authenticatedPage)\.(locator|getBy|click|fill|goto)\(" tests --include='*.spec.ts'

(Or use the harness Grep tool.)

• A spec calling page.locator(...) / page.getByRole(...) / page.click(...) (or the same on authenticatedPage) directly → DOM access leaked out of a POM. Move it into a POM method.
• A spec that destructures authenticatedPage to touch the DOM directly (rather than calling POM methods) is the same violation.
• Every screen referenced by a spec should have a POM under tests/<module>/pages/ that extends BasePage (support/pages/base.page.ts).
• Inside POMs: flag fragile selectors (raw CSS/XPath, index-based nth(...)) where a getByRole/getByLabel/getByTestId would work. page.locator('tbody tr') filtered by text is acceptable for structural collections.

False negatives (grep won't catch): Locators obtained via a destructured page stored in a variable (e.g. const { getByRole } = page; getByRole(...)), or obtained via context.newPage() inline in the test. Read the file to verify. False positive: page.goto in a public-route beforeEach is allowed (part of test setup, not implementation detail).

Rule 3 — Work by fixture (High)

grep -rnE "from '@playwright/test'" tests --include='*.spec.ts'   # specs must NOT import from here
grep -rnE "new \w+Page\(" tests --include='*.spec.ts'             # POMs must come from fixtures

(Or use the harness Grep tool.)

• A spec importing test/expect from @playwright/test instead of its module fixture (./fixtures/<module>.fixture) → loses fixtures. Fix the import.
• new SomePage(...) inside a spec → POM instantiated by hand instead of injected. Provide it as a fixture key.
• A module fixture should extend base (public routes) or auth (authenticated routes) and re-export expect.
• Authenticated-route POMs should be bound to authenticatedPage, not a fresh page.
• Admitted exceptions: Importing BrowserContext types from @playwright/test is allowed for multi-context tests (Medium "verify justification" instead of High). Hand-instantiating a POM with test.use({ storageState: undefined }) is allowed for alternate-tenant tests. Link to ../playwright-e2e-authoring/references/spec.md#admitted-exceptions for constraints.
• Orphan POMs: Each POM class under tests/<module>/pages/ must be exposed by a key in the module fixture. Flag any POMs that are imported nowhere (Medium).

Rule 4 — Per application module (Medium)

• Test folder tests/<module>/ must mirror a real frontend/src/app/modules/<module>. Flag mismatched or invented module names.
• POM, fixtures, and specs for a module stay colocated under that folder; cross-module helpers belong in support/, not copied between modules.
• Naming: <screen>.page.ts/<Screen>Page, <module>.fixture.ts, <feature>.spec.ts.

TypeScript & hygiene

# Detect `any` type (not English word "any" in comments):
grep -rnE ":\s*any\b|as any|<any[,>]" tests support --include='*.ts'

# Detect console.log in test code:
grep -rnE "console\.log" tests support --include='*.ts'

# Detect leftover test markers:
grep -rnE "test\.(only|skip|fixme)\(" tests --include='*.spec.ts'

(Or use the harness Grep tool.)

• any type annotation (: any, as any, <any>) → repo-wide ban (High). Locators are Locator, ctor params Page.
• console.log in test code → remove.
• Mutated shared state instead of fresh objects → flag (immutability is a repo rule).
• Leftover test markers: test.only, test.skip, test.fixme without a reason string → High for .only (blocks CI), Medium for .skip/.fixme (explain why). Note: forbidOnly is CI-enforced, not locally.
• Warm-up check (Medium): Every authenticated route navigated by a POM's goto() must appear in the route warm-up loop of support/auth.setup.ts. Cross-check the two lists — missing routes cause cold-compile flakiness.
• Comment audit (High): If the repo provides scripts/check-added-comments.sh, run it against the base branch; otherwise git diff -U0 | grep -nE '^\+.*(//|/\*|#|<!--)', to verify all added/changed comments are truthful and belong to allowed classes (~/.claude/rules/common/code-comments.md, or the repo's copy). False or forbidden comments block approval.

Independent verification

Don't trust a green self-report — compile it yourself:

cd tests/playwright
npx tsc --noEmit --pretty false

Report the result. If it errors, those are Critical findings regardless of the checklist.

Output format

Lead with a one-line verdict, then findings grouped by severity, each with file:line, the rule it breaks, and a concrete fix. Close with what's clean so the author knows it was actually checked.

## Playwright E2E review — <scope>

Verdict: <BLOCK / approve with fixes / clean>

### Critical
- tests/foo/foo.spec.ts:23 — Rule 1 (no mocks): `page.route('**/api/x')` intercepts the backend.
  Fix: delete the route; assert against the real response, seed data via the `request` fixture.

### High
- tests/foo/foo.spec.ts:1 — Rule 3 (fixtures): imports `test` from `@playwright/test`.
  Fix: `import { test, expect } from './fixtures/foo.fixture';`

### Medium
- ...

### Clean
- POMs extend BasePage and use getByRole/getByLabel throughout.
- `npx tsc --noEmit` passes.

Keep it terse and specific — a reviewer should be able to jump to each file:line and act without re-reading the rules.

For the fix wording: Reuse the templates in ../playwright-e2e-authoring/references/pom.md, fixtures.md, spec.md, and data-setup.md so fixes feel native to the suite's conventions.

On duplication: The four core rules are already stated in tests/playwright/CLAUDE.md (canonical). This skill's checklist adds procedure and the review's own verification steps; do not restate the full rules, only reference them.