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; never Any/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 (py310+ 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.