From 4fef9d0af403853a22bed25ddf633924f1fbb5dc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 16 May 2026 20:54:30 -0700 Subject: [PATCH 1/2] Document in AGENTS.md that the coverage report covers tests The CI test runs use `--cov=aiohttp/ --cov=tests/`, so uncovered lines in `tests/` show up in the codecov patch report alongside uncovered lines in `aiohttp/`. Spell out the common shapes that trip agents up (defensive `raise` inside a monkeypatched stub, one-sided cleanup branches behind `if had_own_attr:`) and the patterns that keep tests fully covered, with a pointer to yarl#1687 as the canonical example of the failure mode. --- AGENTS.md | 47 +++++++++++++++++++++++++++++++++++++++ CHANGES/12580.contrib.rst | 5 +++++ 2 files changed, 52 insertions(+) create mode 100644 CHANGES/12580.contrib.rst diff --git a/AGENTS.md b/AGENTS.md index 0c31b77461f..910eee2fe38 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -352,6 +352,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 @@ -462,6 +504,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..44fb0908929 --- /dev/null +++ b/CHANGES/12580.contrib.rst @@ -0,0 +1,5 @@ +Documented in :file:`AGENTS.md` that the coverage report also +covers ``tests/``, so unreachable defensive ``raise`` guards in +patched stubs and one-sided cleanup branches in tests will be +flagged on the codecov patch report +-- by :user:`bdraco`. From ed635cd7a39e79b780dfd1cc1bbac9dfa0f7ead6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 16 May 2026 21:03:55 -0700 Subject: [PATCH 2/2] Document the docs spell check gate in AGENTS.md CI runs `make doc-spelling` under `-W --keep-going` and treats unknown words as hard failures, including every `CHANGES/*.rst` fragment in the build. Spell out the pre-push step and the wordlist policy so a missing technical word does not burn a CI run. Adds `codecov` to `docs/spelling_wordlist.txt` for the fragment from the previous commit. --- AGENTS.md | 31 ++++++++++++++++++++++++++++++- CHANGES/12580.contrib.rst | 11 +++++++---- docs/spelling_wordlist.txt | 1 + 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 910eee2fe38..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. @@ -485,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 diff --git a/CHANGES/12580.contrib.rst b/CHANGES/12580.contrib.rst index 44fb0908929..fd30647dbd2 100644 --- a/CHANGES/12580.contrib.rst +++ b/CHANGES/12580.contrib.rst @@ -1,5 +1,8 @@ -Documented in :file:`AGENTS.md` that the coverage report also -covers ``tests/``, so unreachable defensive ``raise`` guards in -patched stubs and one-sided cleanup branches in tests will be -flagged on the codecov patch report +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