test-code-conventions

Overview

This skill is a pure reference - no actions, no workflows. It catalogs the test-code conventions a test-code review enforces. When a review flags an issue, this reference gives the reviewer the underlying rule.

When to use

• A new team member is onboarding to the codebase and needs to understand "what we mean by a good test."
• A review flagged an issue and the reviewer needs the underlying rule's rationale.
• The team is authoring its own per-team test conventions document and wants a starting point.

How to use

1. Scope to test files. Apply these conventions to *.spec.* / *.test.* / tests/** only; production code is out of scope.
2. Read the Act lines first (§1). One scan of the single Act per test tells you what each test exercises before you read the body.
3. Check one logical assertion target (§2) and a self-documenting name (§3) - hide the body and see whether the name still explains it.
4. Judge the assertions and doubles (§4 assertion specificity, §5 mocking) - loose matchers and behaviour-verification are the common misses.
5. Trace fixture scope and magic literals (§6, §7): the smallest scope that holds, and named values across the cause-effect chain.
6. Flag slow setup (§10); for E2E tests, apply the selector-priority and web-first rules in §8 / §9.
7. Cite the section when flagging. Name the rule (§4) and spend the words on the specific edit, not on restating the rule.

§1 - AAA structure

The canonical test shape - Arrange, Act, Assert - splits each test into three phases:

test('addItem increases cart count', () => {
  // Arrange
  const cart = new Cart();

  // Act
  cart.addItem({ sku: 'BOOK-001', qty: 1 });

  // Assert
  expect(cart.itemCount).toBe(1);
});

The phases are visually separated (blank line; comment; or // arrange / act / assert headers). The benefits:

• Reviewer can scan a 50-test file by reading only the Act lines - knows what each test actually exercises.
• Failure debugging is faster: the failed assertion points to the specific Assert; the Arrange and Act are isolated.

Each test has exactly one Act. Two Acts = two tests. Splitting on multiple Acts is the canonical refactor when a single test grows too long.

§2 - Single-responsibility per test

A test that asserts cart.count === 1 AND cart.totalPrice === 10 AND cart.lastUpdated > now() is three tests in disguise. When one assertion fails, the test stops; the other two failures are masked.

The rule: one logical assertion per test. "Logical" means related to the same observable property - multiple expect calls that all verify "the cart has one item" (count = 1, items.length = 1, contents[0].sku = '...') are one logical assertion.

Splitting:

// Bad - three logical assertions
test('addItem updates cart', () => {
  cart.addItem(...);
  expect(cart.itemCount).toBe(1);          // assertion 1
  expect(cart.totalPrice).toBe(10);        // assertion 2 (different property)
  expect(cart.lastUpdated).toBeGreaterThan(t0);  // assertion 3 (different property)
});

// Good - three tests, each assertion isolated
test('addItem increments count', () => { /* ... */ });
test('addItem updates totalPrice', () => { /* ... */ });
test('addItem updates lastUpdated', () => { /* ... */ });

§3 - Naming patterns

Two well-established conventions:

A - <system_under_test>_<scenario>_<expected> (Roy Osherove)

test('addItem_validQty_incrementsCount', () => { /* ... */ });
test('addItem_zeroQty_throwsValidationError', () => { /* ... */ });
test('addItem_negativeQty_throwsValidationError', () => { /* ... */ });

The triple makes the test self-documenting; no need to read the body to understand what's verified.

B - Nested describe + it

describe('Cart', () => {
  describe('addItem', () => {
    describe('with valid qty', () => {
      it('increments count', () => { /* ... */ });
    });
    describe('with zero qty', () => {
      it('throws ValidationError', () => { /* ... */ });
    });
  });
});

Both are valid; the team picks one and stays consistent. Mixing the two in one suite is the smell.

Avoid: test('it works'), test('test 1'), test('addItem 1'), test('addItem 2'). Generic names are zero debugging help when the failure surfaces.

§4 - Assertion specificity

Assertions should narrow the verification window to exactly what's being asserted. Vague matchers hide regressions:

The general principle: the assertion should fail on any change to the SUT's behavior that isn't intentional. If the assertion passes for behaviors that shouldn't, it's too loose.

§5 - Mocking

The full taxonomy per mocks-stubs:

"Dummy objects: passed around but never actually used. Usually they are just used to fill parameter lists." (mocks-stubs)

"Fake objects: actually have working implementations, but usually take some shortcut which makes them not suitable for production." (mocks-stubs)

"Stubs: provide canned answers to calls made during the test, usually not responding at all to anything outside what's programmed in." (mocks-stubs)

"Spies: stubs that also record some information based on how they were called." (mocks-stubs)

"Mocks: objects pre-programmed with expectations which form a specification of the calls they are expected to receive." (mocks-stubs)

Per mocks-stubs: "Only mocks insist upon behavior verification. The other doubles can, and usually do, use state verification."

Convention rules:

1. Prefer state verification over behavior verification. Assert on the SUT's resulting state, not on which methods were called on a collaborator. Behavior verification couples the test to the implementation; refactors break tests that should pass.
2. Don't mock what you don't own. Mocking third-party libraries makes tests pass against a fictional API; when the library updates, the mock drifts undetected. Prefer adapter patterns + contract tests against the real boundary.
3. Prefer fakes over mocks for state-bearing collaborators. A fake DB / fake clock / fake feature-flag service is reusable across tests; mocks are bespoke per test.

§6 - Fixture coupling

Tests that share fixtures (parameters, builders, factory functions) should be coupled at the smallest scope:

Anti-pattern: a giant globalFixtures.ts that every test imports. Tests now break in unrelated ways when the global is touched; the test no longer "owns" what it verifies.

§7 - Magic numbers and strings

Test code is more tolerant of magic numbers than production code - the test's job is often to assert against specific values. But meaningful magic matters:

// Bad - unexplained value
expect(cart.totalPrice).toBe(43.21);   // why 43.21?

// Also bad - expectation recomputes the formula under test, so it agrees
// with the implementation even when the implementation is wrong
const EXPECTED_TOTAL = PRICE_PER_BOOK * QTY * (1 + EXPECTED_TAX_RATE);
expect(cart.totalPrice).toBeCloseTo(EXPECTED_TOTAL, 2);

// Better - inputs named, expected value stated independently
const PRICE_PER_BOOK = 10.99;
const QTY = 4;
const TAX_RATE = 0.0825;
expect(cart.totalPrice).toBeCloseTo(47.59, 2);   // 4 x 10.99, +8.25% tax

Name the inputs; never derive the expected value from the expression under test. The named inputs document where the number came from, and the literal expectation is what makes the assertion independent of the code it is checking.

§8 - E2E selectors

Prefer user-facing, accessibility-first locators over DOM-structure selectors. Per tl-queries the query priority is getByRole → getByLabelText → text / placeholder → getByTestId (last resort), and per pw-best-practices CSS-class and XPath selectors are brittle because the DOM changes freely. The full priority table, the per-framework mappings (Playwright / Cypress / Selenium), and the CSS/XPath rationale are in references/e2e-selector-and-assertion-conventions.md.

§9 - Web-first assertions (E2E)

Prefer auto-waiting web-first assertions (await expect(locator).toBeVisible()) over synchronous .isVisible() checks that race the render, per pw-best-practices. The before / after example is in references/e2e-selector-and-assertion-conventions.md.

§10 - Slow setup is a smell

A test that takes >1s in setup (creating fixtures, seeding DB, warming caches) has a coupling problem. The remedies:

• Move fixture creation to a per-suite beforeAll if shared.
• Use a template-DB snapshot/restore pattern for DB tests instead of db:reset.
• Move the unit test to an integration layer if it really needs the full stack - don't run integration tests under the unit-test budget.

The whole-suite cost of slow setup compounds: 10 tests × 2s = +20s per CI run × 50 PRs/day = 1000s/day burned.

§11 - Step design (architecture tier)

Within an "Act" phase, what is one step? The architecture-tier rules for step granularity, layering, and phrasing:

FIRST principles

Per Robert C. Martin - Clean Code (2008), ch. 9 "Unit Tests": Fast (slow tests don't get run), Independent (each test sets up its own world - per Fowler on non-determinism, the prerequisite to parallel execution), Repeatable (any environment), Self-validating (no human reads logs to determine the outcome), Timely (written close to the production code). A step that violates FIRST is the smell; the rules below are the fix.

Step granularity

A "step" is the minimal unit a reader can name in business terms - one meaningful operation, not one click. Each step does exactly one of Arrange / Act / Assert / Annotate; steps that mix two are split (await page.click('#submit'); expect(toast).toBeVisible(); is an Act and an Assert). Aim for 3-8 steps per test body; >15 signals the test does too much or sits at the wrong abstraction.

Abstraction layers

Three layers, named consistently: business (customer.signsIn(), checkout.placesOrder()) → page / component (Page Objects, Tasks: LoginPage.submit({...})) → mechanical (page.click(), page.fill()). The test body lives at the business layer - a body full of mechanical clicks reads as a script, not a specification. Exceptions that legitimately stay mechanical: a11y keyboard-order tests, visual regression, selector-resilience tests - tag them (@a11y, @visual) so reviewers don't refactor them.

Extraction: the rule of three

Per Fowler - Refactoring (2nd ed. 2018): inline at first occurrence, note at the second, extract at the third. Always extract a step that is 5+ mechanical lines, appears in 3+ tests, or needs an explanatory comment (the comment is the smell that the abstraction is missing). Don't pre-extract on the first test (YAGNI), and never hide Act + Arrange in one helper (setupAndDoThing()).

Phrasing and naming

Prefer declarative ("the customer signs in") over imperative ("enters email, clicks submit") - the test: would the wording change if the implementation changed? If yes, rewrite (per Cucumber - Better Gherkin, which applies beyond Gherkin). Step naming follows the same discipline as §3 test naming (per Osherove - The Art of Unit Testing): read the body aloud - it should sound like a specification, not "click, type, click, expect-truthy".

The AAA / Given-When-Then vocabulary mapping, the phase-separation rule, and the full declarative-vs-imperative tables with when-imperative-is-correct cases are in references/step-grouping-and-phrasing.md. Where extracted steps live (Page Object / Screenplay / App Actions) is object-model-patterns; fixture lifecycle is test-isolation-patterns.

Worked example - reviewing one test file

The file under review (checkout.spec.ts):

let cart;                             // file-level, reused across tests
beforeAll(() => { cart = buildCart(); });

test('checkout 1', async () => {
  cart.addItem({ sku: 'BOOK-001', qty: 2 });
  const res = await checkout(cart);
  expect(res).toBeTruthy();
  expect(cart.total).toBe(43.21);
});

Walking the conventions:

After the fixes:

const PRICE = 19.99, QTY = 2, TAX_RATE = 0.0825;
let cart;

beforeEach(() => { cart = buildCart(); });

test('checkout_validCart_confirmsOrder', async () => {
  // Arrange
  cart.addItem({ sku: 'BOOK-001', qty: QTY });

  // Act
  const res = await checkout(cart);

  // Assert
  expect(res.status).toBe('confirmed');
});

test('addItem_appliesTaxedTotal', () => {
  // Arrange
  cart.addItem({ sku: 'BOOK-001', qty: QTY });

  // Assert
  expect(cart.total).toBeCloseTo(PRICE * QTY * (1 + TAX_RATE), 2);
});

Each test now has one Act, one assertion target, a self-documenting name, a specific matcher, an inline-owned fixture, and a derived expected value. For the E2E selector and web-first conventions applied to a *.e2e.ts file, see references/e2e-selector-and-assertion-conventions.md.

References

• mocks-stubs - Martin Fowler on the test-double taxonomy: dummies, fakes, stubs, spies, mocks; state vs behavior verification; classical (Detroit) vs mockist (London) styles.
• tl-queries - Testing Library query priority: role-based → label → placeholder/text → testId. The "as users find them" principle; testid is the last resort.
• pw-best-practices - Playwright best practices: user-facing locators, web-first assertions, "automated tests should verify that the application code works for the end users."
• E2E selector priority + web-first assertions (§8 / §9), with these citations, in references/e2e-selector-and-assertion-conventions.md.
• Step grouping (AAA / Given-When-Then mapping) + declarative phrasing (§11) in references/step-grouping-and-phrasing.md.
• §11 canonical sources: Martin - Clean Code ch. 9 (FIRST, ISBN 978-0132350884); Fowler - Refactoring 2nd ed. (rule of three); Osherove - The Art of Unit Testing (naming); Meszaros - xUnit Test Patterns (ISBN 978-0131495050); Cucumber - Better Gherkin (declarative phrasing); ISTQB glossary - test step / test procedure.