diff --git a/tests/docs/README.md b/tests/docs/README.md index ba7bd5e192a3..d44212a287ab 100644 --- a/tests/docs/README.md +++ b/tests/docs/README.md @@ -79,4 +79,5 @@ treated. | File | Content | |------|---------| | [authoring.md](authoring.md) | How to add a unit test to the suite | +| [reviewing.md](reviewing.md) | How a test change is reviewed | | [linting.md](linting.md) | How ruff is used on the suite | diff --git a/tests/docs/reviewing.md b/tests/docs/reviewing.md new file mode 100644 index 000000000000..a9b002a6dabd --- /dev/null +++ b/tests/docs/reviewing.md @@ -0,0 +1,164 @@ +# Reviewing a test change + +This is the rubric a reviewer applies to a change that adds or edits +tests, and the bar a contributor should meet before sending one. It is +the review-time companion to [authoring.md](authoring.md): authoring.md +says how to write a good test, this says how such a change is judged. + +## Contents + +- [One function per commit](#one-function-per-commit) +- [Test and fix land together](#test-and-fix-land-together) +- [Prove the fix matters](#prove-the-fix-matters) +- [Green at every commit](#green-at-every-commit) +- [Assertions and boundaries](#assertions-and-boundaries) +- [Scratch files](#scratch-files) +- [Coverage](#coverage) +- [Commit message](#commit-message) +- [Reviewer checklist](#reviewer-checklist) + +## One function per commit + +A commit covers one function under test. A module with a single +function is one commit; a module with five functions is five commits, +one per function. All the tests for that function (happy path, guards, +boundaries, error paths) belong in the same commit, because they share +the function and its setup; do not split a commit per assertion. + +The subject line names the test file and the function: + +```text +tests/unit/test_: test () +tests/unit/test_: test () and fix +``` + +The second form is for when a fix to the wic source rides along (see +below). Keep the subject short: it says only "and fix", and the full +explanation of what was fixed goes in the commit body. There is no +leading `wic:`; the subject is keyed to the test file. + +## Test and fix land together + +A test asserts the behaviour wic is *expected* to provide, never the +behaviour it currently happens to have. When a test exposes a defect, +the fix to the wic source lands in the same commit, so the test asserts +the correct behaviour and passes. A change that adds a test asserting a +known-wrong result, or that marks a failing case `xfail` to defer the +fix, does not meet the bar: it either bakes in the bug or leaves a red +test behind. Reject both. + +## Prove the fix matters + +When a commit carries a fix, the test must actually exercise the bug. +The cheap proof is to back the source change out and watch the test go +red, then restore it and watch it pass: + +```bash +git stash push -- src/wic/ +tests/run-tests.sh tests/unit/test_.py # the new tests fail +git stash pop +tests/run-tests.sh tests/unit/test_.py # the new tests pass +``` + +A test that stays green with the fix removed is not testing the fix. +Either the assertion is too weak or the wrong code path is exercised; +in review, treat that as a finding. + +## Green at every commit + +The suite is green at every commit boundary, not just at the tip of the +series: zero failures and zero `xfail` markers, with +`tests/run-tests.sh --lint-tests` passing 100%, reporting nothing. The +test tree is held to a clean bar; a single lint finding fails the +commit. A series that only goes green at the end cannot be bisected and +is not acceptable; check intermediate commits, not just `HEAD`. + +`--lint-src` is a preview report over the wic source, not a gate (see +[linting.md](linting.md)); findings there do not block a test change. + +## Assertions and boundaries + +Every case asserts a specific outcome, an exact value or a specific +exception, never merely that nothing was raised. A test that only +checks for the absence of a crash passes against badly wrong output and +gives false confidence. + +A good test probes the boundary of each parameter and just past it, +rather than only the comfortable middle of its range. The classes worth +covering (empty, zero, negative, maximum and one beyond, wrong types, +path traversal, stale state, and so on) are enumerated in +authoring.md under "Probe the boundaries"; in review, look for the ones +that matter for the function at hand and note the ones that are +missing. + +If an assertion was weakened to make a test pass, the change is going +the wrong way. The answer to a hard-to-pass test is in the code or in +understanding the correct behaviour, never in loosening the assertion. + +## Scratch files + +A test that needs scratch space uses pytest's `tmp_path` fixture, which +gives each test its own directory and removes it automatically when the +test passes (a failing test's directory is kept for inspection). A +change that reaches for `tempfile.mkdtemp()`, `/tmp` directly, or any +other hand-rolled scratch path is going the wrong way: those leak +directories across runs and are worth a finding. + +The scratch tree lands under the system temporary directory by default. +To send it elsewhere, when `/tmp` is small or a run produces a lot of +scratch, use the standard pytest controls rather than a custom setting; +both are passed straight through by `run-tests.sh` and are described in +the suite [README](README.md#scratch-files): + +```bash +TMPDIR=/path/to/scratch tests/run-tests.sh +tests/run-tests.sh --basetemp=/path/to/scratch +``` + +## Coverage + +Coverage is a guide, not a score to maximise. Run it for the module +under review and read which lines and branches the new tests reach: + +```bash +tests/run-tests.sh --coverage tests/unit/test_.py +``` + +Use it to confirm the boundary rule above was actually met: the +function's own guards and error paths should be reached, not just its +happy path. A reachable branch of the function under review that no +test exercises usually means a boundary case is missing, so the fix is +to add that case, not to chase a coverage number. A branch that cannot +sensibly be triggered is not a gap. + +## Commit message + +Each commit stands alone. The body explains the function, the tests, +and, when a fix rides along, what the bug was, why it was invisible +before, and why the test and fix must land together; a short failing +snippet helps. Do not reference other commits by number or hash, since +the series may be reordered or rebased and such a reference would go +stale. When a fix rides along, the body is where the fix is explained +in full, since the subject only says "and fix". Do not weaken the +explanation to save space: a reviewer reading the commit in isolation +should understand the whole change. + +## Reviewer checklist + +- The commit covers exactly one function, with the subject in the form + above. +- All of that function's cases (happy path, guards, boundaries, error + paths) are present and each asserts a specific value or exception. +- If a fix rides along, it is in the same commit and the test fails + without it (verified by backing the fix out). +- No `xfail`, no test asserting a known-wrong result, no weakened + assertion. +- Scratch space uses the `tmp_path` fixture, not `mkdtemp()` or a raw + `/tmp` path. +- The suite is green and `--lint-tests` is clean at this commit, not + only at the tip. +- Coverage confirms the function's own guards and error paths are + reached; any reachable but untested branch is treated as a missing + boundary case. +- The commit message stands alone, references no other commit, and + fully explains any fix that the short "and fix" subject only names.