Test Review

Test Review

Review tests for a feature to catch coverage gaps and quality issues before PR.

Instructions

1. Identify the feature being tested (from user or recent git changes)
2. Find related source files: validations, tRPC routers, components, pages
3. Find related test files: vitest and playwright
4. Check coverage gaps - what's missing?
5. Check test quality - are assertions meaningful?
6. Check for common mistakes
7. Report findings with file:line references

Coverage Gaps

Validation Schemas

• Happy path with valid input
• Each required field missing
• Invalid formats (email, date, etc.)
• Edge cases (empty string, zero, negative numbers)
• Type coercion if applicable

tRPC Procedures

• Success case returns expected shape
• Error case (not found, unauthorized)
• Input validation rejects bad data
• Side effects happen (database updated, email sent)

React Components

• Renders without crashing
• Displays data correctly
• Loading state
• Error state
• User interactions (click, submit, type)
• Accessibility (keyboard nav, screen reader)

E2E (Playwright)

• Happy path flow start to finish
• Form validation errors shown to user
• Error recovery (retry, go back)
• Empty states

Test Quality

Meaningful Assertions

// Bad - just checks truthy
expect(result).toBeTruthy()

// Good - checks specific value
expect(result.id).toBe('123')
expect(result.items).toHaveLength(3)

Independent Tests

• No shared mutable state between tests
• Each test sets up its own data
• Order of tests doesn't matter

Realistic Mocks

// Bad - mock returns nothing useful
const mockFetch = jest.fn()

// Good - mock returns realistic data
const mockFetch = jest.fn().mockResolvedValue({
  id: '123',
  name: 'Test User',
  createdAt: new Date(),
})

Error Testing

// Bad - just checks it throws
expect(() => fn()).toThrow()

// Good - checks specific error
expect(() => fn()).toThrow('User not found')
expect(() => fn()).toThrow(NotFoundError)

Common Mistakes

General

• No .only or .skip left in code
• No console.log in tests
• No hardcoded IDs that won't exist in CI
• No time-dependent tests without mocking Date.now()

Async Issues

// Bad - no await, test passes before async completes
it('creates user', () => {
  createUser({ name: 'Test' })
  expect(db.users).toHaveLength(1)
})

// Good - awaits async operation
it('creates user', async () => {
  await createUser({ name: 'Test' })
  expect(db.users).toHaveLength(1)
})

Playwright-Specific

• Uses semantic locators (getByRole, getByLabel) not CSS selectors
• No page.waitForTimeout() - use waitForResponse, waitForSelector, or expect().toBeVisible()
• Tests don't depend on previous test's state
• Assertions check visible outcomes, not implementation

// Bad - fragile selector, arbitrary timeout
await page.click('.btn-submit')
await page.waitForTimeout(1000)

// Good - semantic locator, waits for outcome
await page.getByRole('button', { name: 'Submit' }).click()
await expect(page.getByText('Saved successfully')).toBeVisible()

Vitest-Specific

• Mocks are reset between tests (vi.clearAllMocks() in beforeEach)
• No network calls without MSW or manual mocking
• describe blocks group related tests logically

Output Format

## Test Review: [feature]

### Coverage Gaps
- validations/booking.ts has no tests for `updateBookingSchema`
- No E2E test for delete flow
- Missing error state test for BookingForm component

### Quality Issues
- tests/vitest/booking.test.ts:23 - assertion just checks truthy, verify specific value
- tests/vitest/booking.test.ts:45 - mock returns empty object, use realistic data

### Common Mistakes
- tests/playwright/booking.spec.ts:12 - uses `waitForTimeout(2000)`, use `waitForResponse` or assertion
- tests/vitest/booking.test.ts:67 - `.only` left in code

### Passed
- Validation schema has happy path + required field tests
- tRPC procedures test success and not-found cases
- No hardcoded IDs
- Semantic locators used throughout

Notes

• Focus on changed files first, but check related test files too
• Not every gap needs fixing - use judgment on risk vs effort
• Flag .only and .skip as blockers - these break CI