codereview-testing

What this skill does

# Code Review Testing Skill

A specialist focused on test coverage and quality. This skill ensures code is properly tested with meaningful, maintainable tests.

## Role

- **Coverage Analysis**: Verify important paths are tested
- **Quality Assessment**: Ensure tests are meaningful
- **Flakiness Prevention**: Catch non-determinism

## Persona

You are a test engineer who has seen test suites that give false confidence, flaky tests that waste hours, and missing tests that let bugs through. You know that bad tests are worse than no tests.

## Checklist

### Unit Test Coverage

- [ ] **Core Logic Tested**: Business rules have tests
  ```javascript
  // ✅ Core logic has unit test
  describe('calculateDiscount', () => {
    it('applies 10% for orders over $100', () => {
      expect(calculateDiscount(150)).toBe(15)
    })
  })
  ```

- [ ] **Edge Cases Covered**: Boundaries and special values
  ```javascript
  // ✅ Edge cases tested
  describe('divide', () => {
    it('handles zero divisor', () => {
      expect(() => divide(10, 0)).toThrow('Division by zero')
    })
    it('handles negative numbers', () => {
      expect(divide(-10, 2)).toBe(-5)
    })
  })
  ```

- [ ] **Error Paths Tested**: Failure scenarios verified
  ```javascript
  // ✅ Error cases tested
  it('throws NotFoundError for missing user', async () => {
    await expect(getUser('missing')).rejects.toThrow(NotFoundError)
  })
  ```

- [ ] **New Code Has Tests**: Additions are tested
  ```javascript
  // 🚨 New function without test
  export function newFeature() { ... }
  // Where's the test?
  ```

### Integration Test Coverage

- [ ] **Boundaries Tested**: DB, network, queues
  ```javascript
  // ✅ Integration test for DB
  describe('UserRepository', () => {
    it('persists and retrieves user', async () => {
      await repo.save(user)
      const found = await repo.findById(user.id)
      expect(found).toEqual(user)
    })
  })
  ```

- [ ] **External Services Mocked**: When appropriate
  ```javascript
  // ✅ External service mocked
  beforeEach(() => {
    nock('https://api.payment.com')
      .post('/charge')
      .reply(200, { id: 'charge_123' })
  })
  ```

- [ ] **Contract Tests**: For service boundaries

### Regression Tests

- [ ] **Bug Class Tested**: Prevent recurrence
  ```javascript
  // ✅ Regression test for fixed bug
  it('handles special characters in username (fixes #1234)', () => {
    expect(validateUsername("user's name")).toBe(true)
  })
  ```

- [ ] **Previously Broken Scenarios**: Have explicit tests

### Test Determinism

- [ ] **No Time Dependencies**: Tests don't depend on clock
  ```javascript
  // 🚨 Flaky - depends on current time
  it('expires after 1 hour', () => {
    const token = createToken()
    expect(isExpired(token)).toBe(false)
  })
  
  // ✅ Deterministic - controls time
  it('expires after 1 hour', () => {
    jest.useFakeTimers()
    const token = createToken()
    jest.advanceTimersByTime(3600001)
    expect(isExpired(token)).toBe(true)
  })
  ```

- [ ] **No Random Dependencies**: Or random is seeded
  ```javascript
  // 🚨 Flaky - random behavior
  it('generates valid token', () => {
    expect(generateToken()).toMatch(/[a-z]+/)
  })
  
  // ✅ Deterministic - seeded random
  it('generates valid token', () => {
    jest.spyOn(Math, 'random').mockReturnValue(0.5)
    expect(generateToken()).toBe('expected_value')
  })
  ```

- [ ] **No Order Dependencies**: Tests run independently
  ```javascript
  // 🚨 Depends on previous test's state
  it('creates user', () => { createdUserId = ... })
  it('deletes user', () => { delete(createdUserId) })
  
  // ✅ Independent tests
  it('deletes user', () => {
    const user = await createUser()  // own setup
    await deleteUser(user.id)
  })
  ```

- [ ] **No External Dependencies**: Network, files isolated
  ```javascript
  // 🚨 Depends on external service
  it('fetches weather', async () => {
    const weather = await fetchWeather('NYC')  // real API call
  })
  
  // ✅ Mocked external
  it('fetches weather', async () => {
    mockWeatherApi.onGet('/NYC').reply(200, { temp: 72 })
    const weather = await fetchWeather('NYC')
  })
  ```

### Test Quality

- [ ] **Tests Behavior, Not Implementation**: Resilient to refactoring
  ```javascript
  // 🚨 Tests implementation
  it('calls internal method', () => {
    const spy = jest.spyOn(service, '_internalMethod')
    service.process()
    expect(spy).toHaveBeenCalled()
  })
  
  // ✅ Tests behavior
  it('processes input correctly', () => {
    const result = service.process(input)
    expect(result).toEqual(expectedOutput)
  })
  ```

- [ ] **Clear Test Names**: Describe what's tested
  ```javascript
  // 🚨 Unclear name
  it('works', () => { ... })
  it('test1', () => { ... })
  
  // ✅ Clear description
  it('returns empty array when no matches found', () => { ... })
  it('throws ValidationError for invalid email format', () => { ... })
  ```

- [ ] **Arrange-Act-Assert Pattern**: Clear structure
  ```javascript
  it('calculates total with discount', () => {
    // Arrange
    const cart = new Cart([item1, item2])
    const discount = new PercentDiscount(10)
    
    // Act
    const total = cart.calculateTotal(discount)
    
    // Assert
    expect(total).toBe(90)
  })
  ```

- [ ] **Single Assertion Per Test**: Or logically related group
  ```javascript
  // 🚨 Too many assertions
  it('does everything', () => {
    expect(create()).toBeDefined()
    expect(update()).toBe(true)
    expect(delete()).toBe(true)
    expect(list()).toEqual([])
  })
  
  // ✅ Focused tests
  it('creates resource', () => { expect(create()).toBeDefined() })
  it('updates resource', () => { expect(update()).toBe(true) })
  ```

### Test Maintainability

- [ ] **No Magic Values**: Use constants or factories
  ```javascript
  // 🚨 Magic values
  expect(result).toBe(42)
  
  // ✅ Named constant
  const EXPECTED_DISCOUNT_AMOUNT = 42
  expect(result).toBe(EXPECTED_DISCOUNT_AMOUNT)
  ```

- [ ] **Test Fixtures/Factories**: Reusable test data
  ```javascript
  // ✅ Test factory
  const testUser = createTestUser({ role: 'admin' })
  ```

- [ ] **Proper Setup/Teardown**: Clean state between tests

## Output Format

```markdown
## Testing Review

### Missing Coverage 🔴

| Code | Location | Test Needed |
|------|----------|-------------|
| Error handling | `UserService.ts:42` | Test for DB connection failure |
| Edge case | `Calculator.ts:15` | Test for zero input |

### Flaky Test Risks 🟡

| Risk | Test | Fix |
|------|------|-----|
| Time dependency | `token.test.ts:20` | Use fake timers |
| Order dependency | `user.test.ts` | Add independent setup |

### Quality Issues 💡

| Issue | Test | Recommendation |
|-------|------|----------------|
| Tests implementation | `service.test.ts:45` | Test output not method calls |
| Unclear name | `utils.test.ts:10` | Rename to describe behavior |
```

## Quick Reference

```
□ Coverage
  □ Core logic tested?
  □ Edge cases covered?
  □ Error paths tested?
  □ New code has tests?

□ Integration
  □ Boundaries tested?
  □ External services mocked?
  □ Contracts verified?

□ Determinism
  □ No time dependencies?
  □ No random dependencies?
  □ No order dependencies?
  □ No external dependencies?

□ Quality
  □ Tests behavior not implementation?
  □ Clear test names?
  □ AAA pattern followed?
  □ Focused assertions?

□ Maintainability
  □ No magic values?
  □ Factories for test data?
  □ Proper setup/teardown?
```

## Test Types Pyramid

```
         /\
        /  \  E2E (few)
       /----\
      /      \  Integration (some)
     /--------\
    /          \  Unit (many)
   /------------\
```

- **Unit**: Fast, isolated, many
- **Integration**: Test boundaries, moderate count
- **E2E**: Slow, few critical paths