diff --git a/AGENTS.md b/AGENTS.md index 0c31b77461f..2eec53c47f1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -257,7 +257,32 @@ is non-obvious, a short reproducer or a paragraph on root cause is welcome. Long, multi-section essays with bolded sub-headings are not the style here. -### 6. Commit hygiene +### 6. Run the docs spell check before pushing + +The `lint` CI job builds the docs with `sphinxcontrib.spelling` +under `make doc-spelling`, which is invoked with `-W +--keep-going` so any unknown word is a hard failure. The spell +checker reads every `CHANGES/*.rst` fragment as part of the +build, so a technical word in your news fragment (`codecov`, +`monkeypatch`, `parametrize`, `repr`, and so on) that is +not in +[`docs/spelling_wordlist.txt`](docs/spelling_wordlist.txt) +will fail `make doc-spelling` and burn a CI run before a human +even sees the PR. + +Before pushing: + +```bash +make doc-spelling +``` + +If it flags a word you actually meant to use, add it to +`docs/spelling_wordlist.txt` (one word per line, roughly +alphabetical) in the same commit as the fragment. If it flags +a typo, fix the typo. Do not paper over real misspellings by +adding them to the wordlist. + +### 7. Commit hygiene - One logical change per PR. If a refactor and a bugfix are bundled together, split them. @@ -352,6 +377,48 @@ CodSpeed benchmark leg, and wheel builds for manylinux, musllinux, macOS, and Windows. Do not regress the benchmarks without flagging the trade-off in the PR body. +### Every line in a test must be covered + +Coverage in this repo tracks both `aiohttp/` and `tests/`; the +CI test jobs run pytest with `--cov=aiohttp/ --cov=tests/` and +the cython-coverage job does the same, so uncovered lines in +`tests/` show up in the codecov patch report on every PR. +Reviewers do look at that report. This catches a class of +mistake agents make all the time: a defensive +`raise RuntimeError("must not be called")` inside a +monkeypatched stub the happy path never invokes, a cleanup +branch behind `if had_own_attr:` that the test never enters, an +`else` arm guarding a condition that is always true under the +fixture. From the perspective of the unit suite all of those +lines are dead code, and the codecov report flags them the same +as dead code in `aiohttp/`. + +Design tests so every line runs: + +- Drive the fixture deterministically so both arms of any + conditional are hit, or drop the conditional entirely and + assert the single shape you actually set up. +- Do not add `raise TypeError("must not be invoked")` guards + inside stubs the test installs; if the stub is never meant to + fire, either omit it or assert at the call site that it did + not. An unreachable `raise` is the most common form of this + failure. +- Cleanup branches that only run when setup took a particular + shape (`if had_own_attr: ...` style restores) need a second + test, or a parametrize, that exercises the other shape. If + you cannot justify the second case, unconditionally restore + instead. +- Prefer `monkeypatch` (which auto-reverts) over hand-rolled + save/restore blocks; the auto-revert path has no untaken + branch for coverage to flag. + +See [aio-libs/yarl#1687](https://github.com/aio-libs/yarl/pull/1687) +for the canonical example: a test added an unreachable `raise` +inside a patched `__getstate__` and a conditional restore of the +original attribute, both of which the codecov report rejected as +uncovered. The same pattern recurs in aiohttp test PRs and the +same review feedback applies. + ## Cython and generated files `aiohttp/_http_parser.pyx`, `aiohttp/_http_writer.pyx`, and @@ -443,6 +510,10 @@ spell-check leg that CI also runs. - Do not invent a `## What / ## Why / ## How / ## Testing` PR body; use the shipped template at `.github/PULL_REQUEST_TEMPLATE.md`. +- Do not push without running `make doc-spelling` first if you + edited any `.rst` file (including a `CHANGES/` fragment). The + docs build fails on unknown words and burns a CI run; see + _Run the docs spell check before pushing_ above. - Do not skip the `CHANGES/` fragment "because the change is small". Even a one-line bugfix needs one. - Do not add `Co-Authored-By` trailers for LLM tools, in either @@ -462,6 +533,11 @@ spell-check leg that CI also runs. source changes. - Do not change one backend without checking the other; see "Dual-backend discipline" above. +- Do not leave unreachable lines in tests (defensive `raise` + inside a stub the suite never invokes, cleanup branches that + only run for a setup shape the test does not exercise). Tests + are part of the coverage report; see _Every line in a test + must be covered_ above. - Do not open PRs against the release branches (`3.12`, `3.13`, etc.); target `master` and let `patchback` handle the backport after merge. diff --git a/CHANGES/12580.contrib.rst b/CHANGES/12580.contrib.rst new file mode 100644 index 00000000000..fd30647dbd2 --- /dev/null +++ b/CHANGES/12580.contrib.rst @@ -0,0 +1,8 @@ +Documented in :file:`AGENTS.md` that the coverage report covers +``tests/`` as well as ``aiohttp/`` (so unreachable defensive +``raise`` guards in patched stubs and one-sided cleanup branches +in tests will surface as uncovered on the codecov patch report), +and that the docs spell check (``make doc-spelling``) is a hard +CI gate that reads every ``CHANGES/*.rst`` fragment and should be +run locally before pushing +-- by :user:`bdraco`. diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 43ff5c75748..c81e125a3d4 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -72,6 +72,7 @@ CIMultiDict ClientSession cls cmd +codecov codebase codec Codings