| name | code-review |
| description | Reviews Playwright test code for POM compliance, best practices, security, and coverage completeness. Auto-loaded when the user asks to review, audit, or inspect test files. |
Playwright Code Review Skill
When reviewing Playwright test code in this repository, apply the following structured checklist and produce a severity-ranked report.
Artifact Locations
- Spec files:
tests/**/*.spec.ts
- Page Objects:
tests/pages/*.ts
- Test data:
tests/data/urls.ts, tests/data/users.ts
- Configuration:
playwright.config.ts
- Results:
test-results/results.json, junit.xml, playwright-report/
1. POM Compliance
| Rule | Pass Condition |
|---|
No raw selectors in *.spec.ts | All CSS/XPath/text selectors live in tests/pages/*.ts only |
| Single-purpose page methods | Each Page Object method performs exactly one action |
| Centralised test data | URLs and users imported from tests/data/ — not hardcoded in specs or page objects |
| Constructor pattern | Page Objects accept Page via constructor and store as readonly page: Page |
How to fix selector leakage:
- Move the selector string to the relevant
tests/pages/*.ts as a private readonly locator property
- Add a method that uses it
- Replace the inline selector in the spec with the page object method call
2. Playwright Best Practices
| Anti-pattern | Replacement |
|---|
page.waitForTimeout(n) | await expect(locator).toBeVisible() or waitForResponse |
page.waitForNavigation() without waitUntil | page.waitForLoadState('domcontentloaded') |
Positional XPath (//div[3]/span) | page.getByRole(), page.getByLabel(), or data-test attribute |
networkidle in flaky contexts | waitForResponse or waitForLoadState('load') |
| Shared state between tests | Use test.beforeEach with fresh page context; avoid module-level state |
No test.describe grouping | Wrap related tests in test.describe('Feature', () => { ... }) |
3. Security (OWASP Top 10 aligned)
- A02 Cryptographic Failures: No passwords, tokens, or API keys hardcoded in any file — use
process.env.VAR or placeholder values in tests/data/users.ts
- A03 Injection: XSS test payloads must use clearly non-functional strings (e.g.
<script>alert(1)</script> for detection tests only — never working exploits)
- A07 Auth Failures: Auth tests must not log or persist sensitive session data to stdout or test artifacts
- A09 Logging Failures: No
console.log(password) or similar in test helpers or page objects
4. Coverage Completeness
- Every positive (happy-path) test should have a paired negative (error/edge) scenario
- UI-critical pages should have a corresponding entry in
tests/accessibility/a11y.spec.ts
- Performance-sensitive pages should appear in
tests/performance-tests/load-time.spec.ts
- Network-dependent flows should have a mock counterpart in
tests/mock-tests/
5. Report Output Format
Produce findings in this table, sorted by severity:
| Severity | File | Issue | Recommendation |
|----------|------|-------|----------------|
| 🔴 Critical | tests/foo.spec.ts:L12 | Selector hardcoded in spec | Move to FooPage.ts |
| 🟡 Warning | tests/pages/FooPage.ts:L8 | waitForTimeout(3000) | Replace with expect(locator).toBeVisible() |
| 🔵 Suggestion | tests/foo.spec.ts | No negative test | Add error-path scenario |
Severity guide:
- 🔴 Critical — blocks merge; violates POM contract or security rules
- 🟡 Warning — degrades reliability or maintainability; should be fixed
- 🔵 Suggestion — improves coverage or readability; nice to have
PowerShell commands for local validation
# Run only the reviewed file to confirm no regressions
npx playwright test tests/<file>.spec.ts --project=chromium
# Run with trace on failure to inspect selector issues
npx playwright test tests/<file>.spec.ts --project=chromium --trace on
# Open the HTML report
npx playwright show-report
