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)

HIGH (Should Fix Before Merge)

MEDIUM (Should Fix)

LOW (Suggestions)

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() to hide failing tests.

// ❌ 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, check if it's a Mantine component.

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