modelcontextprotocol / test-quality

Test & code quality guide

What "best practice for new work" means in this repo. Each rule carries its recorded reasoning
where one exists; a rule with no stated why is a convention — follow it anyway.

Naming & shape

• Test names are behaviour sentences stating the observable outcome, not the feature being
  poked: test_elicit_form_decline_returns_no_content, never test_elicit_form_decline.

• Plain top-level test_* functions; no Test classes (legacy files have them — don't copy).

• Docstrings: 1–2 sentences of behaviour, honest about provenance — spec-mandated,
  SDK-defined, or pinning a known gap? Say which: provenance is the triage key when the test
  later fails. A pinned-gap assertion breaking usually means a change fixed the gap; a
  spec-mandated assertion breaking means a regression.

• Define things in dependency order; nothing forward-references. For client↔server tests:
  handlers → server construction → client setup → act → assert — the test reads in the order
  the conversation happens.

• Inline the server (or equivalent setup) in the test, so the whole observable behaviour fits
  on one screen. Lift to a file-level fixture only when several tests in that file genuinely
  share it; never share across files.

• A big multi-step test is fine when the property is irreducibly multi-step (e.g. resumability).
  Split when a failure wouldn't tell you which claim broke — not for shortness. Compensate with
  a numbered "Steps:" docstring so a reader sees the choreography before the body.

Level of abstraction

• Drive through the highest-level public API that can observe the property. Hand-built wire
  requests are brittle and don't prove the user-facing contract; tests that stay above the
  internals keep working when the internals change. Drop to raw HTTP only when the assertion is
  about something the high-level API cannot observe (status codes, headers, wire framing).

• Scripting a peer over raw streams is a last resort, reserved for behaviour the typed API
  cannot produce (malformed input, an impossible peer response). First ask what it would take
  for the public API to express it — often a small helper suffices. Every such test's docstring
  states why the public API couldn't do it.

• In-memory / in-process first. HTTP-, SSE-, and auth-shaped behaviour can all be driven
  through an in-process ASGI transport; threads only when necessary, subprocesses only when the
  process boundary is itself the thing under test. In-process isn't just faster — it surfaces
  bugs (a real stream leak was found this way) that subprocess indirection masks.

• Tests read like real user code: no aliasing shims in conftest, no walls of suppressions,
  no private imports unless that is genuinely the documented way to do the thing.

• The assertion must prove the round trip — no side-channel state. What the server saw comes
  back through the protocol, or via a closure-captured list asserted after the call. Handlers
  assert their dispatch identity first (assert params.name == "add"), proving the request
  that arrived is the request the test sent.

Assertions

• Transformations (input → output the SDK produced) → full-object snapshot(...) equality,
  so an added or dropped field fails. Regenerate with --inline-snapshot=fix so intentional
  changes arrive as a reviewable diff; never hand-edit snapshot literals.

• Pass-through values (opaque tokens, _meta, cursors) → identity against the same
  variable you sent. A snapshot of a pass-through value only "matches" because a human checked
  two literals correspond — it proves nothing.

• Errors → pytest.raises + .code against the named constant; snapshot SDK-authored
  messages; never match= on message text. Third-party text (pydantic, jsonschema) → stable
  prefix only — never pin text that changes with a dependency upgrade — with a comment saying so.

Determinism

• No sleeps, ever. A sleep guesses at timing instead of waiting on the condition, so it
  either flakes or pads the run — sleeps head the list of the older test code's failure modes.
  Coordinate with anyio.Event so the wait ends exactly when the condition holds (that
  discipline is why 529 e2e tests run in ~10 s). Sole exception: tests of time-based
  features, with a comment.

• Bound every indefinite wait with anyio.fail_after(5). 5 is the standard — widening it
  needs an articulable reason; "10 to be safe" is covering up a flake, not fixing one, and
  unexplained widenings propagate (one agent used 10 and every later one copied it).

• Never assert wall-clock time, even with huge margins. elapsed < 0.9 on a ~0.01 s
  operation — a 90× margin — was still rejected in review. fail_after bounding a hang is the
  only timing primitive allowed.

• Concurrency tests must prove genuine interleaving. Without barriers the scheduler is free
  to serialize — "a" can start, finish, and return before "b" even begins — so two start_soon
  calls prove nothing. Gate with events so all parties are mid-flight before any proceeds, emit
  interleaved, then assert the demux.

• Don't over-synchronize either. When delivery ordering is guaranteed (notifications
  emitted during a request you're awaiting, over a single ordered in-memory stream), a plain
  collected list asserted after the call is correct; events are for messages not tied to an
  awaited operation. Verify the ordering guarantee actually holds for your transport first.

• Async tests use anyio, not asyncio.

Behaviour philosophy

• Pin current behaviour; never xfail. A green suite asserting what actually happens is a
  regression bar for any refactor; an xfail proves nothing about it. Where current behaviour
  falls short of spec, pin the divergent output and record the gap as data — a tracking issue,
  with the docstring naming the known gap (suites with a requirements manifest record it as a
  divergence entry). Not hidden, not skipped.

• Hollow-proof check: before claiming a test covers a behaviour, re-read the claim and ask
  "which assertion proves this?" A passing test near a behaviour is not proof of it — a full
  review of the e2e suite found two such cases even under this discipline.

Hygiene

• No new # pragma, # type: ignore, # noqa by default — restructure first; a suppression
  usually means a test or a type is missing. The narrow sanctioned escape hatches (and the
  audit to run before pushing) are in AGENTS.md. In tests, narrow types with `assert
  isinstance; neverAny/object` when a real type exists.

• Warnings raised during tests are findings, not noise (the repo runs
  filterwarnings = error). Fix the cause; if the fix can't land in the same change, scope the
  suppression to the one fixture that needs it and track the real fix explicitly.

• Registered-but-never-invoked handler bodies are raise NotImplementedError, so they cannot
  silently become load-bearing.

• Comments live next to the line they explain, not in docstrings; single backticks for code
  refs; match the surrounding comment density (one-liners next to one-liners). No
  from __future__ import annotations (py311+ repo).

• Test work doesn't change src/ as a side effect — the one mechanical exception is deleting a
  pragma a new test now covers. If a test can't be written without a library change, raise it
  as a finding or defer the test; don't quietly edit src/.

Process (for agent-driven work)

• Small chunks. ≤10 tests per human-reviewed batch; when fanning out to multiple agents,
  ≤5 per agent. Review quality scales inversely with batch size: the observed shortcuts
  (over-stacked tests, a timing assertion, wrong abstraction level) all surfaced in one
  oversized 27-test batch.

• Design → review → implement, as separate steps. The design deliverable is not just the
  plan — it's the judgement calls (abstraction level, deferrals, contested assertions) stated
  explicitly, so the reviewer vetoes them before implementation rather than discovering them
  in the diff.

• High-stakes areas get fresh adversarial reviewers on both the design and the
  implementation — fresh, because whoever designed it (or saw the proposed fix) anchors on the
  same layer. SERIOUS findings (wrong assertion, missed MUST/SHOULD, an unrecorded known gap)
  re-loop; style doesn't. Reserve the full panel for areas where being wrong is worse than
  being slow.

• Investigations pair a full-context look with a fresh agent given only the problem —
  never the proposed fix — and compare conclusions. The unbiased read regularly catches
  anchoring on the wrong layer.

• When review questions a decision, reconsider genuinely: re-derive the tradeoff, state the
  options, recommend with reasons. Defending the original choice is a valid outcome;
  reflexively agreeing with every challenge is as bad as ignoring it.

• Validate the reference artifact before building on it. Whatever you treat as ground truth
  (a spec import, a baseline, a generated list), check it first — discovering it was
  incomplete after five batches costs far more than before batch one.

• Verify as you go: per-file uv run --frozen pytest <file> -q + pyright + ruff while
  iterating; full suite, coverage, and ./scripts/test (when src/ was touched) at
  integration.

• Every agent writes notes: what it did, what broke, and — most importantly — what it
  couldn't decide, so open questions surface instead of being silently resolved by whoever hit
  them.

• Quality over speed is a stated goal, not a preference ("rushing something out the door is
  not the goal here, explicitly so"). Don't quietly de-scope agreed work mid-stream;
  renegotiate scope explicitly.

• Don't copy patterns from existing test code by default — much of the repo's older test
  code is below the current bar (sleeps, mocks, Test classes, raciness). These rules define
  the bar for new work.