The Art of Test Code Review: Design Quality Revealed
When a PR includes test code, many reviewers glance at it quickly and move on. "There's a test, good job." But this is a missed opportunity for insight. Test code reveals design problems that production code can hide. And good tests often indicate good design, while difficult-to-test code usually indicates design problems.
What Tests Reveal About Design
Signal 1: Too Many Mocks (Tight Coupling)
If you're writing a test and need to mock five or more dependencies:
@Test
void processOrder() {
when(userRepository.findById(any())).thenReturn(testUser);
when(inventoryService.checkStock(any())).thenReturn(true);
when(paymentGateway.charge(any())).thenReturn(paymentResult);
when(emailService.send(any(), any())).thenReturn(true);
when(pointService.accrue(any())).thenReturn(100);
OrderResult result = orderService.processOrder(testRequest);
verify(userRepository).findById(testRequest.getUserId());
verify(inventoryService).checkStock(testRequest.getItems());
verify(paymentGateway).charge(testOrder.getTotalAmount());
verify(emailService).send(testUser.getEmail(), testOrder);
verify(pointService).accrue(testUser.getId(), testOrder.getTotalAmount());
assertThat(result.isSuccess()).isTrue();
}
This isn't a sign the test is complicated. It's a sign the code being tested is too tightly coupled to too many external systems. It depends on five different things.
The fix isn't better mocking techniques. It's better design — separate responsibilities using events and event handlers. Code that's easy to test is code with clear boundaries.
Signal 2: Difficulty Naming Tests
If you write a test and can't think of a good name, the test isn't clear enough about what it's testing.
// Bad - what does this test actually verify?
@Test void testProcessOrder() { }
@Test void test1() { }
@Test void processOrderTest2() { }
// Good - describes the scenario, condition, and expectation
@Test void processOrder_whenStockIsInsufficient_shouldThrowOutOfStockException() { }
@Test void processOrder_whenPaymentFails_shouldCancelOrderAndNotify() { }
@Test void processOrder_whenInputInvalid_shouldThrowValidationException() { }
A good test name tells you the scenario, the condition being tested, and the expected outcome. It becomes documentation.
Signal 3: Setup Code Longer Than Test Body
If creating test data requires 30 lines and the actual test is 2 lines, something is wrong:
@Test
void calculateTotalDiscount_shouldApplyBulkDiscount() {
// Setup — way too long
User user = new User();
user.setId("user-1");
user.setTier(UserTier.GOLD);
user.setJoinedAt(LocalDate.of(2020, 1, 1));
Product product = new Product();
product.setId("product-1");
product.setPrice(new BigDecimal("10000"));
product.setCategory(ProductCategory.ELECTRONICS);
Order order = new Order();
order.setUser(user);
order.setStatus(OrderStatus.PENDING);
order.setItems(List.of(new OrderItem(product, 150)));
DiscountPolicy policy = new BulkDiscountPolicy(100, new BigDecimal("0.1"));
// Act — one line
BigDecimal discount = discountService.calculate(order, policy);
// Assert — one line
assertThat(discount).isEqualByComparingTo(new BigDecimal("150000"));
}
Either the function takes too many parameters, the objects are too complex to construct, or test setup can be extracted into a builder or factory. Well-designed functions are easy to test.
The AAA Pattern: Structure for Clarity
Good tests follow AAA: Arrange → Act → Assert
@Test
void generateToken_shouldCreateAndSaveResetToken() {
// Arrange - prepare test data
User user = userRepository.save(new User("test@test.com", "hashedPw"));
// Act - execute the function being tested
String token = passwordResetService.generateToken(user.getId());
// Assert - verify the results
assertThat(token).isNotNull();
assertThat(tokenRepository.findByToken(token)).isPresent();
}
This structure makes the test readable at a glance. You immediately see what's being prepared, what's being tested, and what should happen.
The Coverage Myth
100% code coverage doesn't mean the code is correct. You can have perfect coverage with meaningless tests:
@Test
void divide_functionRuns() {
divide(10, 2); // no assertion at all
}
// Coverage: 100%. Verification: 0%
Coverage measures execution, not verification. The goal is meaningful assertions, not high numbers.
What matters is whether each test verifies actual values:
@Test
void divide_withValidInputs_returnsCorrectResult() {
expect(divide(10, 2)).toBe(5);
expect(divide(9, 3)).toBe(3);
expect(divide(20, 4)).toBe(5);
}
@Test
void divide_byZero_throwsError() {
expect(() => divide(10, 0)).toThrow('Cannot divide by zero.');
}
@Test
void divide_withNegativeNumbers_returnsNegativeResult() {
expect(divide(-10, 2)).toBe(-5);
}
Common AI Test Patterns to Fix
When AI generates tests, watch for these patterns:
- Only verifies existence: `expect(result).toBeDefined()` instead of `expect(result.value).toBe(expectedValue)`
- Only tests happy path: No failure cases, boundary values, or edge cases
- Tests implementation details: Directly manipulating internal state instead of testing behavior
- Shares state between tests: One test's setup affects another test's outcome
- Multiple assertions per test: Testing multiple scenarios in one test instead of one per test
As a reviewer, you add the missing cases and make assertions meaningful.
React: Test Behavior, Not Implementation
React tests should simulate what users do, not test internal mechanics:
// Bad - tests implementation details
it('LoginForm works', () => {
wrapper.setState({ email: 'test@test.com', password: 'pass' });
wrapper.instance().handleSubmit();
});
// This breaks if you refactor from class to hooks
// Good - tests user behavior
it('submits form when user enters email and clicks login', async () => {
render(<LoginForm onSubmit={mockSubmit} />);
await userEvent.type(screen.getByLabelText('Email'), 'test@test.com');
await userEvent.type(screen.getByLabelText('Password'), 'password123');
await userEvent.click(screen.getByRole('button', { name: 'Log in' }));
expect(mockSubmit).toHaveBeenCalledWith('test@test.com', 'password123');
});
// This still works if you refactor internals, as long as behavior stays same
Test Review as Design Review
Here's the insight that changes how reviewers view test code: test review IS design review.
Code that's easy to test is code with clear responsibilities. Code that requires five mocks has too many dependencies. Code with simple tests is code that's simple to use and maintain.
When reviewing tests, you're reviewing the design. Make assertions meaningful. Add failure cases and boundary values. Separate concerns. The code will follow.
The Boundary Value Test: Revealing Hidden Bugs
One of the most neglected test categories is boundary value testing. These are the values at the edges of what a function can handle:
// Function under test
function calculateAge(birthYear: number): number {
return new Date().getFullYear() - birthYear;
}
// Incomplete tests - only happy path
test('calculates age correctly', () => {
expect(calculateAge(2000)).toBe(24); // current year 2024
});
// Better tests - includes boundary values
test('calculateAge with current year', () => {
expect(calculateAge(2024)).toBe(0); // person born today
});
test('calculateAge with very old year', () => {
expect(calculateAge(1900)).toBe(124); // very old age
});
test('calculateAge with future year', () => {
expect(() => calculateAge(2025)).toThrow(); // not yet born
});
test('calculateAge with invalid input', () => {
expect(() => calculateAge(-1)).toThrow();
expect(() => calculateAge(Infinity)).toThrow();
expect(() => calculateAge(NaN)).toThrow();
});
Boundary values (0, very large, negative, infinity, NaN) often reveal bugs that the happy path never sees.
Snapshot Tests: Hidden Maintenance Burden
Snapshot tests are tempting because they're quick to write. But they often hide problems:
// Tempting but problematic
it('renders UserCard correctly', () => {
expect(render(<UserCard user={user} />).html()).toMatchSnapshot();
});
// Better - explicit assertions
it('displays user name and email', () => {
const { screen } = render(<UserCard user={user} />);
expect(screen.getByText('John Doe')).toBeInTheDocument();
expect(screen.getByText('john@example.com')).toBeInTheDocument();
});
it('shows admin badge when user is admin', () => {
const { screen } = render(<UserCard user={{...user, isAdmin: true}} />);
expect(screen.getByText('Admin')).toBeInTheDocument();
});
Snapshots are convenient but brittle. Any change to output requires manually updating the snapshot, and developers often approve changes without thinking about whether they're correct.
Mutation Testing: When Coverage Lies
Code coverage measures line execution, not code correctness. Mutation testing goes further. It runs your code with small changes and checks if your tests catch the mutations:
function multiply(a, b) {
return a * b;
}
// Naive test with low value
test('multiply works', () => {
expect(multiply(2, 3)).toBe(6);
});
// Coverage: 100%. But if implementation mutates to:
// - return a + b (adds instead of multiplies)
// - return a * 1 (ignores b)
// - return 0
// The test still passes because tests don't verify behavior enough.
// Better test catching mutations
test('multiply returns correct product', () => {
expect(multiply(2, 3)).toBe(6);
expect(multiply(5, 4)).toBe(20);
expect(multiply(0, 100)).toBe(0);
expect(multiply(1, 7)).toBe(7);
expect(multiply(-2, 3)).toBe(-6);
});
Multiple test cases with boundary values catch mutations better than single comprehensive assertions.
Test Isolation: The Shared State Problem
Tests must pass independently, regardless of execution order. This is critical:
// Before: tests depend on shared state
describe('UserService', () => {
let userId: string;
it('creates a user', async () => {
const user = await userService.create('test@test.com');
userId = user.id; // set by this test
expect(user.id).toBeDefined();
});
it('finds user by id', async () => {
// assumes userId was set by previous test
const user = await userService.findById(userId);
expect(user).toBeDefined();
});
});
// After: each test is independent
describe('UserService', () => {
let testUserId: string;
beforeEach(async () => {
const user = await userService.create('test@test.com');
testUserId = user.id;
});
afterEach(async () => {
await userService.delete(testUserId);
});
it('creates a user correctly', async () => {
expect(testUserId).toBeDefined();
});
it('finds existing user by id', async () => {
const user = await userService.findById(testUserId);
expect(user.email).toBe('test@test.com');
});
it('throws when finding non-existent user', async () => {
expect(() => userService.findById('nonexistent'))
.toThrow(UserNotFoundException);
});
});
The second approach is more robust. Each test sets up its own preconditions. They can run in any order. They're more maintainable and clearer about dependencies.
Integration Tests vs Unit Tests: Knowing the Difference
Both are important, but they serve different purposes:
Unit Tests: Test a single function in isolation. Dependencies are mocked. Fast to run. Should be the majority of tests (70%+).
Integration Tests: Test multiple components working together. Real database, real APIs. Slower but more realistic. Should be 20-30% of tests.
A PR with only unit tests might pass but fail in integration. A PR with only integration tests is slow and brittle. Aim for the pyramid: many unit tests, fewer integration tests, fewer end-to-end tests.
Assertion Specificity
When reviewing test assertions, check they're testing what you think:
// Bad - too vague
expect(result).toBeDefined();
expect(result).toBeTruthy();
// Better - specific values
expect(result.status).toBe('success');
expect(result.userId).toBe('user-123');
expect(result.items).toHaveLength(3);
expect(result.items[0].price).toBe(29.99);
// Best - explicit description
expect(result).toEqual({
status: 'success',
userId: 'user-123',
items: [
{ productId: 'prod-1', price: 29.99 },
{ productId: 'prod-2', price: 49.99 },
{ productId: 'prod-3', price: 9.99 },
]
});
Comments
Post a Comment