Review

Review Playwright Tests

Systematically review Playwright test files for anti-patterns, missed best practices, and coverage gaps.

Input

$ARGUMENTS can be:

• A file path: review that specific test file
• A directory: review all test files in the directory
• Empty: review all tests in the project's testDir

Steps

1. Gather Context

• Read playwright.config.ts for project settings
• List all *.spec.ts / *.spec.js files in scope
• If reviewing a single file, also check related page objects and fixtures

2. Check Each File Against Anti-Patterns

Load anti-patterns.md from this skill directory. Check for all 20 anti-patterns.

Critical (must fix): 1. waitForTimeout() usage 2. Non-web-first assertions (expect(await ...)) 3. Hardcoded URLs instead of baseURL 4. CSS/XPath selectors when role-based exists 5. Missing await on Playwright calls 6. Shared mutable state between tests 7. Test execution order dependencies

Warning (should fix): 8. Tests longer than 50 lines (consider splitting) 9. Magic strings without named constants 10. Missing error/edge case tests 11. page.evaluate() for things locators can do 12. Nested test.describe() more than 2 levels deep 13. Generic test names ("should work", "test 1")

Info (consider): 14. No page objects for pages with 5+ locators 15. Inline test data instead of factory/fixture 16. Missing accessibility assertions 17. No visual regression tests for UI-heavy pages 18. Console error assertions not checked 19. Network idle waits instead of specific assertions 20. Missing test.describe() grouping

3. Score Each File

Rate 1-10 based on:

• 9-10: Production-ready, follows all golden rules
• 7-8: Good, minor improvements possible
• 5-6: Functional but has anti-patterns
• 3-4: Significant issues, likely flaky
• 1-2: Needs rewrite

4. Generate Review Report

For each file:

## <filename> — Score: X/10

### Critical
- Line 15: `waitForTimeout(2000)` → use `expect(locator).toBeVisible()`
- Line 28: CSS selector `.btn-submit` → `getByRole('button', { name: "submit" })`

### Warning
- Line 42: Test name "test login" → "should redirect to dashboard after login"

### Suggestions
- Consider adding error case: what happens with invalid credentials?

5. For Project-Wide Review

If reviewing an entire test suite:

• Spawn sub-agents per file for parallel review (up to 5 concurrent)
• Or use /batch for very large suites
• Aggregate results into a summary table

6. Offer Fixes

For each critical issue, provide the corrected code. Ask user: "Apply these fixes? [Yes/No]"

If yes, apply all fixes using Edit tool.

Output

• File-by-file review with scores
• Summary: total files, average score, critical issue count
• Actionable fix list
• Coverage gaps identified (pages/features with no tests)

Playwright Anti-Patterns Reference

1. Using waitForTimeout()

Bad:

await page.click('.submit');
await page.waitForTimeout(3000);
await expect(page.locator('.result')).toBeVisible();

Good:

await page.getByRole('button', { name: 'Submit' }).click();
await expect(page.getByTestId('result')).toBeVisible();

Why: Arbitrary waits slow tests and cause flakiness. Web-first assertions auto-retry.

2. Non-Web-First Assertions

Bad:

const text = await page.textContent('.message');
expect(text).toBe('Success');

Good:

await expect(page.getByText('Success')).toBeVisible();

Why: expect(locator) auto-retries until timeout. expect(value) checks once and fails.

3. Hardcoded URLs

Bad:

await page.goto('http://localhost:3000/login');

Good:

await page.goto('/login');

Why: baseURL in config handles the host. Tests break across environments with hardcoded URLs.

4. CSS/XPath When Role-Based Exists

Bad:

await page.click('#submit-btn');
await page.locator('.nav-link.active').click();

Good:

await page.getByRole('button', { name: 'Submit' }).click();
await page.getByRole('link', { name: 'Dashboard' }).click();

Why: Role-based locators survive CSS renames, class refactors, and component library changes.

5. Missing await

Bad:

page.goto('/dashboard');
expect(page.getByText('Welcome')).toBeVisible();

Good:

await page.goto('/dashboard');
await expect(page.getByText('Welcome')).toBeVisible();

Why: Missing await causes race conditions. Tests pass sometimes, fail others.

6. Shared Mutable State

Bad:

let userId: string;

test('create user', async ({ page }) => {
  // ... creates user, sets userId
  userId = '123';
});

test('edit user', async ({ page }) => {
  await page.goto(`/users/${userId}`); // depends on previous test
});

Good:

test('edit user', async ({ page }) => {
  // Create user via API in this test's setup
  const userId = await createUserViaAPI();
  await page.goto(`/users/${userId}`);
});

Why: Tests must be independent. Shared state causes order-dependent failures.

7. Execution Order Dependencies

Bad:

test('step 1: fill form', async ({ page }) => { ... });
test('step 2: submit form', async ({ page }) => { ... });
test('step 3: verify result', async ({ page }) => { ... });

Good:

test('should fill and submit form successfully', async ({ page }) => {
  // All steps in one test
});

Why: Playwright runs tests in parallel by default. Order-dependent tests fail randomly.

8. Tests Over 50 Lines

Split into focused tests. Each test should verify one behavior.

9. Magic Strings

Bad:

await page.getByLabel('Email').fill('admin@test.com');

Good:

const TEST_USER = { email: 'admin@test.com', password: 'Test123!' };
await page.getByLabel('Email').fill(TEST_USER.email);

10. Missing Error Cases

If you test the happy path, also test:

• Invalid input
• Empty state
• Network error
• Permission denied
• Timeout/loading state

11. Using page.evaluate() Unnecessarily

Bad:

const text = await page.evaluate(() => document.querySelector('.title')?.textContent);

Good:

await expect(page.getByRole('heading')).toHaveText('Expected Title');

12. Deep Nesting

Keep test.describe() to max 2 levels. More makes tests hard to find and maintain.

13. Generic Test Names

Bad: test('test 1'), test('should work'), test('login test')

Good: test('should show error when email is invalid'), test('should redirect to dashboard after successful login')

14-20. Style Issues

• No page objects for complex pages → create them
• Inline data → use factories or fixtures
• Missing a11y assertions → add toHaveAttribute('role', ...)
• No visual regression → add toHaveScreenshot() for key pages
• Not checking console errors → add page.on('console', ...)
• Using networkidle → use specific assertions instead
• No test.describe() → group related tests