Playwright Code Review

Purpose

Audit Playwright E2E tests for violations of best practices. Detects anti-patterns that cause flaky tests, hide bugs, or reduce maintainability.

When NOT to Use

• Writing new tests (use

  playwright-writing

  skill instead)
• Unit test reviews (different patterns)
• API-only test reviews
• Non-Playwright E2E frameworks (Cypress, etc.)

Review Workflow

Step 1: Identify Playwright Test Files

# Find all Playwright test files
find . -name "*.spec.ts" -o -name "*.test.ts" | grep -v node_modules

# Or check specific directory
ls -la tests/e2e/ playwright/

Step 2: Run Automated Violation Checks

Execute these checks in parallel for efficiency:

# === CRITICAL VIOLATIONS ===

# Find mocked application APIs (FORBIDDEN)
grep -rn "page.route('/api\|page.route(\"/api" --include="*.spec.ts" --include="*.test.ts"

# Find skipped tests (FORBIDDEN)
grep -rn "test.skip\|\.skip(\|test\.fixme" --include="*.spec.ts" --include="*.test.ts"

# Find commented-out tests
grep -rn "// test(\|// it(\|/\* test" --include="*.spec.ts" --include="*.test.ts"

# === HIGH PRIORITY VIOLATIONS ===

# Find waitForTimeout (FORBIDDEN)
grep -rn "waitForTimeout\|waitForTime" --include="*.spec.ts" --include="*.test.ts"

# Find CSS class selectors (FORBIDDEN)
grep -rn "\.locator('\.\|\.locator(\"\." --include="*.spec.ts" --include="*.test.ts"

# Find manual assertions without web-first (FORBIDDEN)
grep -rn "\.isVisible()).toBe\|\.isHidden()).toBe\|\.textContent()).toBe\|\.innerText()).toBe" --include="*.spec.ts" --include="*.test.ts"

# Find expect(await pattern (usually wrong)
grep -rn "expect(await.*\.isVisible\|expect(await.*\.isEnabled\|expect(await.*\.isChecked" --include="*.spec.ts" --include="*.test.ts"

# === MEDIUM PRIORITY CHECKS ===

# Find selectOption (may not work with Mantine)
grep -rn "\.selectOption(" --include="*.spec.ts" --include="*.test.ts"

# Find potential setTimeout/delay patterns
grep -rn "setTimeout\|new Promise.*resolve.*setTimeout" --include="*.spec.ts" --include="*.test.ts"

Step 3: Manual Review

For each file, check:

1. Locator Quality

   • Are

     getByRole

     ,

     getByText

     ,

     getByLabel

     used?
   • Are CSS selectors avoided?
   • Is

     data-testid

     only used when necessary?

2. Assertion Quality

   • Are all assertions web-first (

     await expect(locator).toBeVisible()

     )?
   • No manual

     isVisible()

     /

     textContent()

     checks?

3. Test Structure

   • Is

     beforeEach

     used for common setup?
   • Are tests independent (no shared state)?
   • Is navigation handled properly?

4. Mantine Components

   • Are Mantine Selects handled with click pattern?
   • No

     selectOption()

     on Mantine components?

Step 4: Report Findings

Use the output template below to report issues.

Severity Classification

CRITICAL (Must Fix Before Merge)

page.route('/api

test.skip

.skip()

// test(

HIGH (Should Fix Before Merge)

waitForTimeout()

waitForTimeout

.locator('.'

isVisible()).toBe(true)

expect(await

expect(await.*isVisible

MEDIUM (Should Fix)

selectOption

beforeEach

locator('#id')

LOW (Suggestions)

getByRole

getByTestId

describe

Critical Violation Details

Mocked Application APIs

FORBIDDEN: Mocking YOUR application's API endpoints.

// ❌ CRITICAL VIOLATION
await page.route('/api/users', route => route.fulfill({
  body: JSON.stringify([{ id: 1, name: 'Mock' }])
}));

// ❌ CRITICAL VIOLATION
await page.route('/api/products/**', route => route.fulfill({
  body: JSON.stringify({ price: 99.99 })
}));

ALLOWED: Mocking external third-party APIs only:

// ✅ ACCEPTABLE - external service
await page.route('https://api.stripe.com/**', route => route.fulfill({ ... }));

Review action: Check if the mocked URL is YOUR API or an external service.

Skipped Tests

FORBIDDEN: Using

.skip()

// ❌ CRITICAL VIOLATION
test.skip('checkout flow', async ({ page }) => { ... });

// ❌ CRITICAL VIOLATION
test('checkout flow', async ({ page }) => {
  test.skip(true, 'TODO: fix later');
});

// ❌ CRITICAL VIOLATION
test.fixme('broken test', async ({ page }) => { ... });

Review action: Require fix or removal. No exceptions without documented ticket.

High Violation Details

Explicit Timeouts

// ❌ HIGH VIOLATION
await page.waitForTimeout(2000);
await page.waitForTimeout(500);

// ❌ HIGH VIOLATION
await new Promise(resolve => setTimeout(resolve, 1000));

// ✅ CORRECT - web-first assertion
await expect(page.getByText('Loaded')).toBeVisible();

CSS Class Selectors

// ❌ HIGH VIOLATION
page.locator('.btn-primary')
page.locator('.MuiButton-root')
page.locator('div.container > .item')

// ✅ CORRECT
page.getByRole('button', { name: 'Submit' })
page.getByTestId('submit-button')  // if disambiguation needed

Manual Assertions

// ❌ HIGH VIOLATION - no auto-wait/retry
expect(await page.locator('#status').isVisible()).toBe(true);
expect(await page.getByText('Hello').textContent()).toBe('Hello');

// ✅ CORRECT - web-first, auto-waits
await expect(page.getByTestId('status')).toBeVisible();
await expect(page.getByText('Hello')).toHaveText('Hello');

Mantine Component Checks

Select Components

// ❌ HIGH VIOLATION - selectOption doesn't work with Mantine
await page.getByRole('combobox').selectOption('value');

// ✅ CORRECT pattern for Mantine Select
await page.locator('[data-testid="StatusSelect"]').click();
await page.locator('div[value="active"]').click();

Review action: If you see

selectOption

Review Output Template

## Playwright Test Review: [File/PR Name]

### Summary
[1-2 sentence overview of findings]

### Critical Issues (Must Fix)
- [ ] **Mocked app API** - `tests/checkout.spec.ts:45` - Mocking `/api/cart`
- [ ] **Skipped test** - `tests/auth.spec.ts:23` - `test.skip('login flow')`

### High Priority (Should Fix)
- [ ] **waitForTimeout** - `tests/dashboard.spec.ts:67` - `waitForTimeout(2000)`
- [ ] **CSS selector** - `tests/form.spec.ts:34` - `.locator('.submit-btn')`
- [ ] **Manual assertion** - `tests/profile.spec.ts:89` - `isVisible()).toBe(true)`

### Medium Priority
- [ ] **selectOption on Mantine** - `tests/filters.spec.ts:56` - Review if Mantine component

### Passed Checks
- [x] No skipped tests
- [x] Web-first assertions used
- [x] User-facing locators preferred
- [x] Test isolation with beforeEach

### Recommendations
- [Optional improvements and suggestions]

Quick Commands Reference

# Full audit - run all checks
grep -rn "waitForTimeout\|test\.skip\|\.skip(\|page\.route('/api\|\.locator('\.\|isVisible()).toBe" --include="*.spec.ts" --include="*.test.ts"

# Count violations by type
echo "=== waitForTimeout ===" && grep -c "waitForTimeout" **/*.spec.ts 2>/dev/null || echo "0"
echo "=== test.skip ===" && grep -c "test.skip\|\.skip(" **/*.spec.ts 2>/dev/null || echo "0"
echo "=== CSS selectors ===" && grep -c "\.locator('\." **/*.spec.ts 2>/dev/null || echo "0"

Integration

Related skills:

• playwright-writing

  - Writing new tests (use for guidance)

• run-tests

  - Executing test suite

• quality-check

  - General code quality

Cross-reference: All rules in this review skill match those in

playwright-writing

Related Agent

For comprehensive E2E testing guidance that coordinates this and other Playwright skills, use the

playwright-e2e-expert