/learn-review-implementation

name: Learn why to review implementation even with 100% test coverage
description: Interactive guidance on why human code review is essential for readability and maintainability.
user-invocable: false

Why Review Implementation Even with 100% Test Coverage?

Overview

This learning module helps junior developers understand why, as of today in 2026, human code review remains essential even when AI-generated code has 100% unit test coverage. Tests only verify code doesn't crash - humans must verify correctness, readability, and maintainability.

Instructions

Present the following content interactively. After each section, use `AskUserQuestion` to offer 3 options:

• "Continue to next section"
• "Show me an example"
• "I have a question"

---

Section 1: What 100% Test Coverage Actually Proves (and Doesn't)

Explain what test coverage measures and its limitations:

**What 100% test coverage proves:**

• Every line of code is executed during tests
• The code doesn't crash during execution

**That's it.** Coverage doesn't verify correctness - test assertions could be inadequate, wrong, or missing entirely.

**What 100% test coverage does NOT prove:**

• The code behaves correctly (assertions could be wrong)
• The code meets functional requirements (assertions could be inadequate)
• The code is easy to understand and intuitive
• The code will be easy to modify later
• The variable and function names are clear
• The solution is appropriately simple

**The key insight:**

Tests answer "Does it crash?" not necessarily "Does it work?" or "Is it good code?" All three questions matter.

---

Section 2: Why Test Cases Need Review

Since 100% coverage only proves code doesn't crash, the test assertions themselves are critical - and they need human review.

The Problem with Trusting Tests Blindly

// This test "passes" but proves nothing
test('should process order', () => {
  const result = processOrder(mockOrder);
  expect(result).toBeDefined();  // Weak assertion - just checks it exists
});

// This test passes but has wrong expectation
test('should calculate total', () => {
  const total = calculateTotal([10, 20, 30]);
  expect(total).toBe(50);  // Wrong! Should be 60
});

// This test is missing assertions entirely
test('should update user', async () => {
  await updateUser({ id: 1, name: 'John' });
  // No assertions - test passes if it doesn't crash
});

All three tests pass. None prove correctness.

What to Check in Every Test

**1. Are assertions actually verifying behavior?**

• `expect(result).toBeDefined()` - Too weak
• `expect(result).toBe(expectedValue)` - Better

**2. Are assertions checking the right values?**

• Review expected values manually - don't trust them
• Trace through the logic yourself

**3. Are critical assertions present?**

• Side effects verified (database, API calls)
• Edge cases covered
• Error conditions tested

**4. Do assertions match requirements?**

• Cross-reference with acceptance criteria
• Ensure all requirements have corresponding tests

Test Review Checklist

• [ ] Each test has meaningful assertions (not just `.toBeDefined()`)
• [ ] Expected values are verified to be correct
• [ ] All code paths have assertions, not just execution
• [ ] Edge cases have specific assertions
• [ ] Error scenarios verify the error, not just that one occurred

---

Section 3: Readability: Code That Passes Tests but Is Hard to Understand

Present examples of working code that has readability issues:

Example 1: Unclear Variable Names

// Passes tests but hard to understand
const d = new Date();
const t = d.getTime();
const x = t - (24 * 60 * 60 * 1000);
const r = items.filter(i => i.c > x);

// BETTER - Self-documenting
const now = new Date();
const currentTimestamp = now.getTime();
const oneDayAgo = currentTimestamp - (24 * 60 * 60 * 1000);
const recentItems = items.filter(item => item.createdAt > oneDayAgo);

Both pass the same tests. Only one is readable.

Example 2: Dense Logic

// Passes tests but requires mental parsing
return users.filter(u => u.a && u.r.includes('admin') && !u.d).map(u => ({...u, p: u.p.filter(p => p.e)}));

// BETTER - Step by step
const activeUsers = users.filter(user => user.isActive);
const adminUsers = activeUsers.filter(user => user.roles.includes('admin'));
const nonDeletedAdmins = adminUsers.filter(user => !user.isDeleted);

return nonDeletedAdmins.map(user => ({
  ...user,
  permissions: user.permissions.filter(permission => permission.enabled)
}));

Example 3: Magic Numbers and Strings

// Passes tests but meaning is unclear
if (status === 3 && retries < 5) {
  setTimeout(retry, 30000);
}

// BETTER - Named constants explain intent
const STATUS_FAILED = 3;
const MAX_RETRIES = 5;
const RETRY_DELAY_MS = 30000;

if (status === STATUS_FAILED && retries < MAX_RETRIES) {
  setTimeout(retry, RETRY_DELAY_MS);
}

---

Section 4: Maintainability: Code That Passes Tests but Is Hard to Change

Present examples of code that passes tests but will cause problems later:

Example 1: Over-Engineering

// Passes tests but over-engineered for a simple task
interface ConfigurationStrategy {
  getConfig(): Config;
}

class JsonConfigStrategy implements ConfigurationStrategy {
  getConfig() { return JSON.parse(fs.readFileSync('config.json')); }
}

class ConfigFactory {
  static create(type: string): ConfigurationStrategy {
    if (type === 'json') return new JsonConfigStrategy();
    throw new Error('Unknown type');
  }
}

const config = ConfigFactory.create('json').getConfig();

// BETTER - Simple solution for simple problem
const config = JSON.parse(fs.readFileSync('config.json'));

Example 2: Under-Engineering

// Passes tests but duplicates logic that should be shared
function validateUserEmail(email) {
  return email.includes('@') && email.includes('.') && email.length > 5;
}

function validateContactEmail(