Test Automation Review

Test Automation Review

When to Use

• Reviewing pull requests containing test automation code
• Auditing existing test suites for quality and maintainability
• Evaluating page object model implementations
• Identifying and fixing flaky tests
• Improving test data management strategies
• Assessing assertion quality and coverage
• Establishing test automation best practices for a team
• Refactoring legacy test code

When NOT to Use

• Reviewing unit tests for business logic (use code-review skill)
• Designing test cases from scratch (use test-case-design skill)
• Performance or load testing setup
• Manual test case review

Review Checklist

Quick Reference

Page Object Patterns

Page objects encapsulate UI structure and behavior, separating what the test does from how it interacts with the UI.

Page Object Review Checklist

• Each page/component has its own page object class
• Selectors are defined as private/protected properties
• Methods represent user actions, not implementation details
• Page objects return other page objects for navigation
• No assertions inside page objects (with rare exceptions)
• Selectors use stable attributes (data-testid, aria-label) over CSS classes
• Reusable components have their own component objects

Page Object Patterns

Good Pattern: Action-Oriented Methods

// ✅ GOOD: Methods describe user intent
class LoginPage {
  private usernameInput = '[data-testid="username"]';
  private passwordInput = '[data-testid="password"]';
  private submitButton = '[data-testid="login-submit"]';

  async login(username: string, password: string): Promise<DashboardPage> {
    await this.enterUsername(username);
    await this.enterPassword(password);
    await this.clickSubmit();
    return new DashboardPage();
  }

  private async enterUsername(username: string): Promise<void> {
    await this.page.fill(this.usernameInput, username);
  }
}

Anti-Pattern: Exposed Implementation

// ❌ BAD: Test knows about implementation details
class LoginPage {
  public usernameInput = '#user-name-field';  // Exposed selector
  
  async typeInField(selector: string, text: string) {  // Generic method
    await this.page.type(selector, text);
  }
}

// Test using anti-pattern
test('login', async () => {
  await loginPage.typeInField(loginPage.usernameInput, 'user');  // Coupled to selectors
});

Selector Strategy Review

Page Object Issues to Flag

Test Data Management

Effective test data management ensures tests are reliable, independent, and maintainable.

Test Data Review Checklist

• Tests create their own data (no shared state between tests)
• Test data is cleaned up after test completion
• Realistic data used (not just "test123" everywhere)
• Sensitive data is not hardcoded (use environment variables or fixtures)
• Data factories or builders are used for complex objects
• Database state is reset between test runs
• Test data files are version controlled
• No dependency on production data or external systems

Data Management Patterns

Good Pattern: Factory/Builder for Test Data

// ✅ GOOD: Factory creates consistent, customizable data
class UserFactory {
  static create(overrides: Partial<User> = {}): User {
    return {
      id: faker.datatype.uuid(),
      email: faker.internet.email(),
      name: faker.name.fullName(),
      role: 'user',
      createdAt: new Date(),
      ...overrides
    };
  }

  static createAdmin(overrides: Partial<User> = {}): User {
    return this.create({ role: 'admin', ...overrides });
  }
}

// Usage in tests
test('admin can delete user', async () => {
  const admin = UserFactory.createAdmin();
  const targetUser = UserFactory.create();
  // ...
});

Anti-Pattern: Hardcoded Test Data

// ❌ BAD: Hardcoded, fragile test data
test('user registration', async () => {
  await registerUser({
    email: 'test@test.com',    // Will conflict with parallel tests
    password: 'password123',   // Hardcoded, may violate password rules
    name: 'Test User'          // Not representative of real data
  });
});

Test Data Strategy Matrix

Test Data Issues to Flag

Assertion Quality

Good assertions are specific, meaningful, and provide clear failure messages.

Assertion Review Checklist

• Each test has at least one assertion
• Assertions verify the right thing (not just "something happened")
• Custom error messages explain what failed and why
• Assertions are specific (exact values, not just truthiness)
• Multiple related assertions are grouped logically
• Soft assertions used when checking multiple independent conditions
• No assertions in loops without clear purpose
• Negative test cases assert expected failures

Assertion Patterns

Good Pattern: Specific, Meaningful Assertions

// ✅ GOOD: Specific assertions with clear failure messages
test('user profile displays correct information', async () => {
  const user = UserFactory.create({ name: 'Jane Doe', email: 'jane@example.com' });
  await profilePage.load(user.id);

  expect(await profilePage.getName()).toBe(user.name);
  expect(await profilePage.getEmail()).toBe(user.email);
  expect(await profilePage.isVerifiedBadgeVisible()).toBe(true);
});

// With custom error messages
expect(actualBalance).toBe(expectedBalance, 
  `Expected balance ${expectedBalance} after withdrawal, got ${actualBalance}`);

Anti-Pattern: Weak Assertions

// ❌ BAD: Vague, unhelpful assertions
test('login works', async () => {
  await loginPage.login('user', 'pass');
  
  expect(page.url()).toBeTruthy();         // Passes even on error page
  expect(await page.content()).toBeDefined(); // Always true
  expect(result).not.toBeNull();           // Doesn't verify correctness
});

Assertion Quality Matrix

Assertion Anti-Patterns to Flag

Soft Assertions

Use soft assertions to check multiple independent conditions:

// ✅ GOOD: Soft assertions collect all failures
test('form validation shows all errors', async ({ expect }) => {
  await formPage.submitEmpty();

  await expect.soft(formPage.getError('name')).toBe('Name is required');
  await expect.soft(formPage.getError('email')).toBe('Email is required');
  await expect.soft(formPage.getError('password')).toBe('Password is required');
  // All errors reported, even if first fails
});

Flaky Test Detection

Flaky tests pass and fail intermittently without code changes, eroding confidence in the test suite.

Flaky Test Review Checklist

• No hardcoded waits (sleep(5000))
• Explicit waits for specific conditions
• No race conditions between test steps
• Tests don't depend on execution order
• Proper handling of animations and transitions
• Network requests are waited for or mocked
• No reliance on external services without mocking
• Date/time is controlled, not using new Date()
• Random data doesn't affect assertions
• Retry logic only for known flaky external dependencies

Common Flakiness Patterns

Timing Issues

// ❌ BAD: Hardcoded wait
await page.click('#submit');
await page.waitForTimeout(3000);  // Arbitrary wait
expect(await page.getText('#result')).toBe('Success');

// ✅ GOOD: Wait for specific condition
await page.click('#submit');
await page.waitForSelector('#result:has-text("Success")', { state: 'visible' });
expect(await page.getText('#result')).toBe('Success');

Race Conditions

// ❌ BAD: Race condition with async operation
await page.click('#load-data');
const text = await page.getText('#data');  // May get old/empty text
expect(text).toBe('Loaded');

// ✅ GOOD: Wait for state change
await page.click('#load-data');
await page.waitForFunction(() => 
  document.querySelector('#data')?.textContent === 'Loaded'
);
expect(await page.getText('#data')).toBe('Loaded');

External Dependencies

// ❌ BAD: Depends on external API
test('shows weather', async () => {
  await page.goto('/weather');
  expect(await page.getText('#temp')).toMatch(/\d+°/);  // External API may fail
});

// ✅ GOOD: Mock external dependencies
test('shows weather', async () => {
  await page.route('**/api/weather', route => 
    route.fulfill({ body: JSON.stringify({ temp: 72 }) })
  );
  await page.goto('/weather');
  expect(await page.getText('#temp')).toBe('72°');
});

Flakiness Root Causes

Flakiness Detection Strategies

## Flakiness Detection Checklist

1. **Static Analysis**
   - [ ] Search for `sleep`, `waitForTimeout`, hardcoded delays
   - [ ] Find tests without explicit waits after actions
   - [ ] Identify global state modifications
   - [ ] Check for `new Date()` without mocking

2. **Runtime Analysis**
   - [ ] Run tests multiple times (10+ iterations)
   - [ ] Run tests in different orders (randomize)
   - [ ] Run tests in parallel
   - [ ] Run in CI environment
   
3. **Historical Analysis**
   - [ ] Track tests that frequently need retries
   - [ ] Monitor tests with inconsistent duration
   - [ ] Review tests that fail only in specific conditions

Maintainability Best Practices

Maintainable test code is readable, well-organized, and easy to update.

Maintainability Review Checklist

• Tests have clear, descriptive names
• Test files organized by feature/module
• Common setup extracted to before hooks or fixtures
• Helper functions are reusable and well-documented
• Magic numbers/strings replaced with named constants
• Tests follow single responsibility principle
• Consistent naming conventions throughout
• Test utilities are in separate files
• Comments explain "why" not "what"
• Tests are independent and can run in isolation

Test Organization Patterns

File Structure

tests/
├── e2e/
│   ├── auth/
│   │   ├── login.spec.ts
│   │   ├── logout.spec.ts
│   │   └── password-reset.spec.ts
│   └── checkout/
│       ├── cart.spec.ts
│       └── payment.spec.ts
├── fixtures/
│   ├── users.json
│   └── products.json
├── pages/
│   ├── LoginPage.ts
│   ├── DashboardPage.ts
│   └── components/
│       ├── HeaderComponent.ts
│       └── ModalComponent.ts
├── utils/
│   ├── factories/
│   │   └── UserFactory.ts
│   ├── helpers/
│   │   └── apiHelpers.ts
│   └── constants.ts
└── playwright.config.ts

Test Naming Conventions

// ✅ GOOD: Descriptive names following pattern
describe('User Authentication', () => {
  describe('Login', () => {
    it('should successfully login with valid credentials', async () => {});
    it('should display error message for invalid password', async () => {});
    it('should lock account after 5 failed attempts', async () => {});
  });
});

// ❌ BAD: Vague or implementation-focused names
describe('Login', () => {
  it('test1', async () => {});
  it('clicks submit button', async () => {});  // Describes action, not outcome
  it('works', async () => {});
});

DRY Principles in Tests

Good Pattern: Reusable Setup

// ✅ GOOD: Shared setup with customization
class TestContext {
  static async withAuthenticatedUser(
    options: { role?: string } = {}
  ): Promise<{ user: User; page: Page }> {
    const user = await UserFactory.create(options);
    const page = await browser.newPage();
    await loginAs(page, user);
    return { user, page };
  }
}

test('admin can view all users', async () => {
  const { page } = await TestContext.withAuthenticatedUser({ role: 'admin' });
  // ...
});

Anti-Pattern: Copy-Paste Tests

// ❌ BAD: Duplicated setup in every test
test('test 1', async () => {
  await page.goto('/login');
  await page.fill('#email', 'admin@test.com');
  await page.fill('#password', 'password');
  await page.click('#submit');
  await page.waitForNavigation();
  // actual test...
});

test('test 2', async () => {
  await page.goto('/login');
  await page.fill('#email', 'admin@test.com');  // Same setup copied
  await page.fill('#password', 'password');
  await page.click('#submit');
  await page.waitForNavigation();
  // actual test...
});

Maintainability Issues to Flag

Review Feedback Format

When reviewing test automation code, use this format:

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

### Summary
[Brief assessment of overall test quality]

### Scores

| Category | Score | Notes |
|----------|-------|-------|
| Page Objects | ⭐⭐⭐☆☆ | [Brief note] |
| Test Data | ⭐⭐⭐⭐☆ | [Brief note] |
| Assertions | ⭐⭐⭐⭐⭐ | [Brief note] |
| Flakiness Risk | ⭐⭐☆☆☆ | [Brief note] |
| Maintainability | ⭐⭐⭐☆☆ | [Brief note] |

### Critical Issues
- **[File:Line]** [Severity] - [Description and fix]

### Recommendations
1. [Prioritized improvement suggestion]
2. [Another suggestion]

### Strengths
- [What's done well]

Severity Levels

Best Practices

1. Prioritize stability - A reliable simple test beats a flaky comprehensive test
2. Review selectors first - Selector strategy is the foundation of UI test reliability
3. Check test isolation - Every test should pass when run alone or with others
4. Verify assertions are meaningful - Tests that can't fail provide no value
5. Look for hidden dependencies - Execution order, shared state, external services
6. Consider maintenance burden - Ask "who will update this when the UI changes?"
7. Balance coverage and speed - Fast tests that run often beat slow tests that don't
8. Encourage consistent patterns - Team alignment reduces cognitive load
9. Document non-obvious decisions - Explain workarounds for known issues
10. Review test code like production code - Apply same quality standards