Why Review Test Code?

Test code is production code. It runs in CI/CD pipelines, affects deployment decisions, and is maintained for years. Yet many teams treat test code as second-class — skipping reviews, tolerating poor naming, and ignoring duplication. The result: a brittle, unmaintainable test suite that nobody trusts.

Reviewing test code with the same rigor as application code prevents these problems. A good review catches false-positive tests (tests that always pass regardless of bugs), missing assertions, improper setup/teardown, and anti-patterns that lead to flakiness.

Test Code Review Checklist

1. Does the Test Test What It Claims?

This is the most important check. Read the test name, then verify the assertions match.

// BAD — name says "creation" but test only checks navigation
@Test
void testUserCreation() {
    loginPage.login("admin", "pass");
    dashboardPage.clickCreateUser();
    createUserPage.fillForm("Alice", "alice@test.com");
    createUserPage.submit();
    // No assertion that the user was actually created!
}

// GOOD — assertion matches the test name
@Test
void testUserCreation() {
    loginPage.login("admin", "pass");
    dashboardPage.clickCreateUser();
    createUserPage.fillForm("Alice", "alice@test.com");
    createUserPage.submit();
    assertTrue(userService.exists("alice@test.com")); // Verifies creation
    assertEquals("Alice", userService.findByEmail("alice@test.com").getName());
}

2. Test Naming

Test names should describe the scenario and expected outcome:

// BAD
@Test void test1() { ... }
@Test void testLogin() { ... }

// GOOD
@Test void should_showDashboard_when_loginWithValidCredentials() { ... }
@Test void should_showError_when_loginWithExpiredPassword() { ... }
@Test void should_lockAccount_when_threeFailedLoginAttempts() { ... }

3. Arrange-Act-Assert Structure

Each test should have clearly separated setup, action, and verification phases:

// GOOD — clear AAA structure
@Test
void should_applyDiscount_when_couponIsValid() {
    // Arrange
    Order order = OrderFactory.createWithTotal(100.00);
    Coupon coupon = CouponFactory.createValid("SAVE20", 20);

    // Act
    order.applyCoupon(coupon);

    // Assert
    assertEquals(80.00, order.getTotal(), 0.01);
    assertEquals("SAVE20", order.getAppliedCoupon().getCode());
}

4. One Concern per Test

Each test should verify one specific behavior. If a test has 10 assertions checking different behaviors, split it.

5. No Logic in Tests

Tests should have no if/else, loops, or try/catch. Logic in tests makes them harder to understand and can mask failures.

// BAD — logic in test
@Test
void testUserRoles() {
    for (String role : List.of("admin", "user", "moderator")) {
        User user = UserFactory.createWithRole(role);
        if (role.equals("admin")) {
            assertTrue(user.canDelete());
        } else {
            assertFalse(user.canDelete());
        }
    }
}

// GOOD — separate tests, no logic
@Test void admin_canDelete() { ... }
@Test void regularUser_cannotDelete() { ... }
@Test void moderator_cannotDelete() { ... }

Common Anti-Patterns

Mystery Guest

The test depends on external state not visible in the test:

// BAD — where does this user come from?
@Test
void testOrderCreation() {
    Order order = orderService.createOrder("user-123", items);
    assertNotNull(order); // Depends on user-123 existing in the database
}

// GOOD — test creates its own data
@Test
void testOrderCreation() {
    User user = UserFactory.createAndSave();
    Order order = orderService.createOrder(user.getId(), items);
    assertNotNull(order);
}

Eager Test

A test that verifies too many things at once, making it hard to diagnose which behavior broke.

The Giant

A test method with 100+ lines that is impossible to understand at a glance. Break it into smaller focused tests.

Sensitive Equality

Asserting on entire objects or strings when only a specific field matters:

// BAD — breaks if any field changes
assertEquals(expectedUser.toString(), actualUser.toString());

// GOOD — check only relevant fields
assertEquals("Alice", actualUser.getName());
assertEquals("admin", actualUser.getRole());

Hardcoded Waits 

// BAD
await page.waitForTimeout(5000);

// GOOD
await page.waitForSelector('.loaded');
await expect(page.locator('.result')).toBeVisible();

Review Guidelines for Teams

What Reviewers Should Check

  1. Assertion correctness — Does it verify the right behavior?
  2. Test isolation — Can this test fail because of another test?
  3. Naming clarity — Can you understand the test without reading the code?
  4. Data independence — Does the test create its own data?
  5. No flakiness risks — Are there hardcoded waits or timing dependencies?
  6. Proper cleanup — Does the test clean up after itself?
  7. Readability — Can a new team member understand this test?

What Reviewers Should NOT Nitpick

  • Minor formatting preferences (let linters handle this)
  • Assertion library choice (AssertJ vs Hamcrest)
  • Test data values that do not affect behavior

Exercises

Exercise 1: Anti-Pattern Detection

Review 5 provided test code samples and identify all anti-patterns. For each, explain why it is a problem and propose a fix.

Exercise 2: Naming Refactoring

Take 10 poorly named test methods and rename them following the should_outcome_when_condition convention. Verify that names alone describe what is being tested.

Exercise 3: Team Standards Document

Create a test code review standards document for your team covering naming conventions, structural rules, anti-patterns to avoid, and examples of good vs bad test code. Include a checklist that reviewers can use during code reviews.