Maintaining Test Suites — Avoiding Test Rot

1// SYMPTOM 1: Slow tests (> 1 second per unit test)
2// Cause: real I/O leaked into unit tests
3test('validates user email format', async () => {  // async! why?
4  const user = await createUser({ email: 'bad' });  // real DB call!
5  expect(user.valid).toBe(false);
6});
7// Fix: validate pure logic without DB
8test('validateEmail returns false for invalid format', () => {
9  expect(validateEmail('bad').valid).toBe(false);
10});
11
12// SYMPTOM 2: Brittle tests (break on unrelated changes)
13test('renders UserCard', () => {
14  const { container } = render(<UserCard user={user} />);
15  expect(container).toMatchSnapshot(); // 200-line snapshot
16});
17// When you add a className, the snapshot fails even though no behaviour changed
18// Fix: test specific behaviours, not full snapshots
19test('UserCard shows user name', () => {
20  render(<UserCard user={user} />);
21  expect(screen.getByRole('heading')).toHaveTextContent('Alice');
22});
23
24// SYMPTOM 3: Tests that always pass (vacuous assertions)
25test('processes the order', async () => {
26  await processOrder(testOrder);
27  // No assertion — always passes regardless of what processOrder does
28});
29// Fix: assert on the observable outcome
30test('processOrder returns the confirmed order id', async () => {
31  const result = await processOrder(testOrder);
32  expect(result.orderId).toMatch(/^ord-[0-9a-f]+$/);
33  expect(result.status).toBe('confirmed');
34});
35
36// SYMPTOM 4: Commented-out tests (dead code)
37test.skip('returns 429 after 10 failed logins', async () => {
38  // TODO: re-enable when rate limiting is fixed
39  // Skipped 6 months ago, never fixed
40});
41// Fix: either fix the rate limiting bug, or delete this test and create a GitHub issue
42
43// SYMPTOM 5: Inter-dependent tests (order matters)
44describe('user flow', () => {
45  let userId: string;
46  test('creates a user', async () => {
47    userId = (await createUser('[email protected]')).id;  // sets shared state
48  });
49  test('deletes the user', async () => {
50    await deleteUser(userId);  // depends on test 1 having run
51    expect(await findUser(userId)).toBeNull();
52  });
53});
54// Fix: each test manages its own state

1// BEFORE: duplicated setup across tests
2describe('OrderService', () => {
3  test('creates order for premium user', async () => {
4    const user = await db.user.create({ data: { email: '[email protected]', role: 'PREMIUM', passwordHash: 'x', name: 'Alice', createdAt: new Date() } });
5    const product = await db.product.create({ data: { title: 'Widget', price: 49.99, published: true, instructorId: user.id } });
6    const service = new OrderService(new OrderRepository(db));
7    const result = await service.create(user.id, product.id);
8    expect(result.status).toBe('pending');
9  });
10
11  test('calculates correct total for premium user', async () => {
12    const user = await db.user.create({ data: { email: '[email protected]', role: 'PREMIUM', passwordHash: 'x', name: 'Bob', createdAt: new Date() } });
13    const product = await db.product.create({ data: { title: 'Gadget', price: 99.99, published: true, instructorId: user.id } });
14    const service = new OrderService(new OrderRepository(db));
15    const result = await service.create(user.id, product.id);
16    expect(result.total).toBe(89.99); // 10% premium discount
17  });
18
19  // 8 more tests with the same setup duplicated...
20});
21
22// AFTER: extract factory functions (same pattern as production factories)
23async function createPremiumUser(overrides = {}) {
24  return db.user.create({
25    data: { email: `user-${Date.now()}@test.com`, role: 'PREMIUM', passwordHash: 'x', name: 'Test', createdAt: new Date(), ...overrides },
26  });
27}
28
29async function createPublishedProduct(instructorId: string, overrides = {}) {
30  return db.product.create({
31    data: { title: 'Test Product', price: 49.99, published: true, instructorId, ...overrides },
32  });
33}
34
35describe('OrderService (refactored)', () => {
36  let service: OrderService;
37  beforeEach(() => { service = new OrderService(new OrderRepository(db)); });
38
39  test('creates order with pending status', async () => {
40    const user = await createPremiumUser();
41    const product = await createPublishedProduct(user.id);
42    const result = await service.create(user.id, product.id);
43    expect(result.status).toBe('pending');
44  });
45
46  test('applies 10% discount for premium users', async () => {
47    const user = await createPremiumUser();
48    const product = await createPublishedProduct(user.id, { price: 100 });
49    const result = await service.create(user.id, product.id);
50    expect(result.total).toBe(90);
51  });
52  // Each test is 3 lines — setup is declarative
53});

1// THE RULE:
2// A skipped test must be fixed or deleted within one sprint.
3// A test that cannot be fixed is a known bug — create a GitHub issue, then delete the test.
4// Never commit test.skip or test.todo without an associated issue number.
5
6// ACCEPTABLE: skip with issue reference and expiry
7test.skip('returns 429 after rate limit — fix in #456', async () => {
8  // Skipped: rate limiter has a known bug in test environment
9  // Issue: https://github.com/org/repo/issues/456
10  // Expires: Sprint 24 (one sprint from now)
11});
12
13// UNACCEPTABLE: skip without reason or deadline
14test.skip('some test', async () => { ... }); // what broke? when was this skipped? why?
15
16// PRE-COMMIT HOOK: fail if test.skip appears without issue reference
17// .husky/pre-commit:
18if git diff --cached | grep -E '^\+.*test\.skip\(' | grep -v 'issue\|#[0-9]'; then
19  echo 'ERROR: test.skip without issue reference detected'
20  echo 'Add an issue link: test.skip("description — fix in #NNN", ...)'
21  exit 1
22fi
23
24// ESLINT RULE: enforce the pattern
25// .eslintrc.ts:
26// 'jest/no-disabled-tests': 'error',  // ban test.skip() entirely
27// OR
28// custom rule that requires issue reference in skip message

1// F.I.R.S.T Audit Script — paste into a team meeting and work through
2
3// F — FAST:
4// npx jest --verbose 2>&1 | grep -E '^  .* [0-9]+ms' | sort -t' ' -k2 -rn | head -20
5// Any unit test > 100ms is suspect
6// Any unit test > 500ms has leaked I/O — fix immediately
7
8// I — ISOLATED:
9// npx jest --randomize  (run 3 times — all should produce same pass/fail)
10// npx jest --randomize --runInBand
11// Failures that only appear with --randomize have shared state
12
13// R — REPEATABLE:
14// npx jest --passWithNoTests --count=10 (run suite 10 times)
15// Any test that fails < 100% of runs is non-repeatable — prioritise for fix
16
17// S — SELF-VALIDATING:
18// grep -r 'console.log' src/**/*.test.ts  — manual validation leak
19// Tests that print output require a human to interpret → not self-validating
20
21// T — TIMELY:
22// git log --follow -p -- src/modules/auth/auth.service.ts
23// Check: were tests added in the same commit as the code?
24// Code committed without tests violates T — schedule test-writing session
25
26// Additional checks:
27// Branch coverage gate:
28npx jest --coverage 2>&1 | grep 'Branches' | awk '{print $2}'
29// If < 80%, branches are untested
30
31// Dead test detection:
32npx jest --verbose --testPathPattern='.*' 2>&1 | grep 'PASS\|FAIL' | wc -l
33// Compare to total test files — if many fewer, some files are not being picked up

1// CONVENTION 1: Tests in the same commit as the code they test
2// CI check: every commit that touches src/**/*.ts must also touch *.test.ts
3// (Enforced via pre-commit hook or CI step)
4
5// CONVENTION 2: Test names follow the behaviour specification format
6// All test names must match: /returns .+ when .+|throws .+ when .+|calls .+ with .+/
7// Enforced with ESLint custom rule or code review checklist
8
9// CONVENTION 3: 80% branch coverage is non-negotiable
10// jest.config.ts coverageThreshold is set and cannot be reduced without PR approval
11// Coverage ratchet: never allow coverage to drop below current level
12
13// CONVENTION 4: Code review checklist for test quality
14const testReviewChecklist = [
15  'Every new file has a corresponding .test.ts file',
16  'Test names read as specifications (not "test1" or "works")',
17  'No page.waitForTimeout() or arbitrary sleeps',
18  'No console.log statements in test files',
19  'No test.skip without issue reference',
20  'beforeEach resets all shared state',
21  'Mocks are cleaned up (jest.clearAllMocks or clearMocks: true in config)',
22];
23
24// CONVENTION 5: Pair on failing tests
25// When a developer gets stuck on a TDD session (> 30 minutes on one cycle),
26// they request pairing. The pair's job is not to solve it but to ask:
27// 'Is the test too big? Is the implementation trying to do too much at once?'
28
29// CONVENTION 6: Test-review sessions
30// Monthly: randomly pick 5 test files, review them as a team
31// Questions: Are these testing behaviour or implementation?
32//            Are the test names still accurate after refactoring?
33//            Are there obvious missing edge cases?