Playwright E2E Review

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).