Repository navigation
Require 80% coverage on the lines a pull request changes, with diff-cover - #4200
Merged
Merged
Conversation
Tests and proof fixtures for api.quality.diff-coverage: diff-cover --fail-under=80 rejects 7 of 10 changed lines covered and accepts 8 of 10; pr.yml's diff-coverage job downloads the test shards' and integration tests' reports, runs diff-cover against the base commit, and is needed by CI complete with no condition of its own; the rule, its proof and Codecov's patch status use the same minimum; and diff-cover is a locked dev dependency. All 4 tests fail: diff-cover isn't installed and there is no such job, rule or setting yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A new diff-coverage job in pr.yml runs after the test shards and the integration tests, downloads every shard's coverage.xml and the integration tests' coverage-v2.xml, and runs diff-cover --fail-under=80 against the pull request's base commit. CI complete needs it, so a pull request below the minimum can't merge; with no condition of its own, it is skipped where the tests are. - diff-cover 10.6.0 is in the dev extra and locked (11.0 is less than two weeks old). - New enforced metric rule api.quality.diff-coverage, with a proof at 70% and 80%; Codecov's patch status shows the same 80% target, still informational; testing.md has a "Changed lines" section. The 4 new tests pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4200 +/- ##
==========================================
+ Coverage 88.63% 92.11% +3.48%
==========================================
Files 304 297 -7
Lines 18442 16673 -1769
Branches 3084 2719 -365
==========================================
- Hits 16346 15359 -987
+ Misses 1357 798 -559
+ Partials 739 516 -223
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Brings in master's declared-dependencies guard (deptry). The conflicts in pyproject.toml's dev extra keep both deptry and diff-cover; uv.lock was regenerated with uv 0.13.4, and docs/generated/rules.md and AGENTS.md were rendered again from the merged registry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ling) Review found three ways the diff-coverage job can score the wrong lines: - It compares with the pull_request event's base.sha, which often stays where the branch left master, while CI checks out a merge into master as it is now; master's own changes since then count as the pull request's. - coverage.py 6.5 lists a module no test imports only inside a package, and policyengine_api/libs and policyengine_api/routes have no __init__.py, so a new untested module there would not be measured and would pass. - Imports under `if TYPE_CHECKING:` never run and count as missed. New tests run the job's own command on a merge commit, run coverage.py with the repository's .coveragerc, and check the step's wiring. Four fail: - test_the_job_measures_only_the_lines_the_pull_request_changes: 90%, exit 0, with master_change.py counted (should be 50%, exit 1) - test_coverage_reports_every_module_no_test_imports: 15 modules in libs/ and routes/ unreported - test_type_only_imports_and_pragmas_are_not_counted_as_missed: line 4 missed - test_pull_requests_check_the_coverage_of_their_changed_lines: the step still reads BASE_SHA test_a_line_counts_as_covered_if_any_report_covers_it passes already. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- pr.yml's diff-coverage job compares with HEAD^1, the base branch that GitHub's merge commit is built on, instead of the pull_request event's base.sha. base.sha often stays where the branch left master (17 of 88 recent pr.yml runs, up to 1,077 lines of master's changes under policyengine_api in between), and diff-cover counted those lines too. - policyengine_api/libs and policyengine_api/routes get an __init__.py. coverage.py 6.5 reports a module no test imports only inside a package, so their untested modules were left out of every report and their changed lines passed. pyproject.toml's ruff comment no longer says they have none. - .coveragerc excludes `if TYPE_CHECKING:` blocks, which never run, and keeps coverage.py's default `pragma: no cover`, which setting exclude_lines replaces. - v2-integration-check.yml's comment says the diff-coverage job reads the integration report too. The testing skill and the rule's statement describe the HEAD^1 comparison and the __init__.py requirement. The four tests that failed now pass, with the other four in tests/unit/test_diff_coverage.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 2 found that the coverage reports name files by an absolute source directory: a report written in another checkout directory, such as a container job's, matches no changed file, and diff-cover then passes with "No lines with coverage information". It also found that .coveragerc's `pragma: no cover` is narrower than coverage.py's default pattern, which the comment says it repeats. - New test_reports_match_the_changed_files_in_another_checkout measures a module with the repository's .coveragerc in one directory and runs the job's command in another checkout of the same change. It fails: "No lines with coverage information in this diff.", exit 0 (should be 50%, exit 1). - test_type_only_imports_and_pragmas_are_not_counted_as_missed adds the `#pragma:no cover` and `# PRAGMA NO COVER` spellings. It fails: lines 14 and 18 counted as missed. - The proof's below fixture is now 799 of 1,000 changed lines (79.9%), just below the minimum, and the at fixture 800 of 1,000. The proof passes: diff-cover truncates 79.9% to 79. - test_pull_requests_check_the_coverage_of_their_changed_lines also checks that pr.yml runs only on pull_request, which HEAD^1 relies on. It passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- .coveragerc sets relative_files = True. The reports then name files by the relative source `policyengine_api`, so a report written in another checkout directory still matches the changed files; with absolute paths diff-cover found none and passed. - exclude_lines lists coverage.py's default pattern itself, written with `\#` because a value line that starts with `#` is a comment, instead of the narrower text `pragma: no cover`. - The rule statement, the testing skill and pr.yml's comment say that the lines counted are the ones coverage.py measures: each statement's first line. The skill also says a line with a branch that never ran still counts, which tests are measured, that moved code counts as changed, and how the local command keeps Python 3.12 and starts from no `.coverage`. - codecov.yml's patch comment says how Codecov's figure can differ from the job's. The two tests that failed now pass, with the other seven in tests/unit/test_diff_coverage.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ing) Review round 3 found two problems with .coveragerc's exclude_lines: - `if TYPE_CHECKING:` matches anywhere in a line, so a comment that mentions it on a `def` line excludes the whole untested function. - The `...` bodies of Protocol methods never run and count as missed: 89 such lines are measured and never run in CI run 42046754813. test_type_only_imports_and_pragmas_are_not_counted_as_missed becomes test_only_lines_that_can_run_are_counted, with a Protocol method stub and an untested function whose comment mentions `if TYPE_CHECKING:`. It fails: missing lines are [23] (the stub) instead of [27] (the untested function). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- .coveragerc's `if TYPE_CHECKING:` pattern now matches only the whole statement, with an optional comment. coverage.py searches each line of text, so the unanchored text also matched a comment on a `def` line and excluded the whole untested function. - A new pattern excludes `...` alone on a line: the bodies of Protocol methods and other stubs, which never run. Over the 346 modules under policyengine_api, the only change from the previous configuration is 89 such statements, in 16 files, leaving the count. - The testing skill lists the stub exclusion and says that git's rename detection pairs a new file at least 50% similar to a deleted one, so only its edited lines count. - The testing skill, pr.yml's comment and the rule statement say that for a pull request stacked on another, HEAD^1 is the lower one's merge commit: refs/pull/4200/merge (c0882ac) is built on refs/pull/4196/merge (5ff9f72), not on quality/codecov-flags. test_only_lines_that_can_run_are_counted now passes, with the other eight in tests/unit/test_diff_coverage.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk
marked this pull request as ready for review
November 5, 2026 18:54
anth-volk
added a commit
that referenced
this pull request
Nov 5, 2026
Brings in #4196's Codecov flags, #4200's diff-coverage job and #4205's v2 file-layer exceptions. pr.yml merged cleanly: CI complete needs every job, including diff-coverage and the coverage uploads, and the quality-guards job still runs make quality, which runs #4205's v2-layers guard. No rule refers to the removed lint or typecheck-v2 jobs. docs/generated/rules.md and the Rules block in AGENTS.md were rendered again, keeping this branch's make quality instruction in AGENTS.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk
added a commit
that referenced
this pull request
Nov 5, 2026
Brings in master (#4196, #4200, #4205) through #4203's branch. The generated docs/generated/rules.md and the Rules block in AGENTS.md were rendered again, keeping this branch's make install and hook instructions in AGENTS.md, which #4203's branch doesn't change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4199
Based on
master. It was stacked on #4196, which merged. #4196'sv2-integration-check.ymlkeeps the integration tests' coverage report as theintegration-coverageartifact that this job downloads.masterwas merged in at8ab6df648, the first run of the full pull-request workflow, and so of thediff-coveragejob itself, on this pull request.Summary
diff-coverageinpr.yml:testandv2-integration-check.coverage.xml(test-coverage-shard-*) and the integration tests'coverage-v2.xml(integration-coverage).uv run --frozen --extra dev diff-cover "$RUNNER_TEMP"/coverage/*/coverage*.xml --compare-branch=HEAD^1 --fail-under=80.HEAD^1is the commit that merge is built on: the base branch, or for a pull request stacked on another, the lower one's merge commit. For example,refs/pull/4200/merge(c0882ac1) is built onrefs/pull/4196/merge(5ff9f725). The job runs only on the lowest pull request of a stack. Either way the diff holds only the pull request's changes. The event'sbase.shaisn't used; see "WhyHEAD^1" below.CI completeneeds the job, so a pull request with less than 80% of its changed lines inpolicyengine_api/covered can't merge. No ruleset change is needed.diff-cover>=10.6,<11is added to thedevextra and locked with uv 0.13.4.chardet7.7.0 (released 2026-10-18).api.quality.diff-coverage.tests/unit/test_diff_coverage.py::test_rule_proof_diff_coverage, commits a module, adds 1,000 lines, and runs the real diff-cover with--fail-under=80. It must reject 799 covered lines (79.9%, which diff-cover truncates to 79%) and accept 800 (80%).pr.ymlwith the locked diff-cover on a merge commit. One checks thatmaster's changes after the branch left it aren't counted, with the event'sbase.shaat that older commit. The other checks that a line counts as covered if any report covers it, with one shard report and one integration report..coveragercand no imports, and checks that every module underpolicyengine_api/is reported..coveragercin one directory, then runs the job's command in another checkout of the same change, and checks that the change is measured.test_only_lines_that_can_run_are_countedchecks that.coveragercleavesif TYPE_CHECKING:imports, a Protocol method's...body and three spellings ofpragma: no coverout of the missed lines. It also checks that an untested function whose comment mentionsif TYPE_CHECKING:still counts.policyengine_api/libs/__init__.pyandpolicyengine_api/routes/__init__.pyare new.__init__.py, so their untested modules were in no report, and diff-cover would pass a new untested module there.libs/user_auth.py, which an integration test imports. Five of the six shard reports left outlibs/simulation_api.pyandlibs/simulation_api_modal.py.pyproject.toml's ruffnamespace-packagescomment no longer says they have none..coveragercchanges:if TYPE_CHECKING:blocks, which never run. Ruff's TC rules move imports there, so a fully tested change that adds type-only imports would otherwise fall below 80%. 192 modules underpolicyengine_api/have such a block. The pattern matches only the whole statement: coverage.py searches each line of text, and a matching line that opens a block excludes the block, so the plain text would also hide an untested function whosedefline has a comment mentioning it....alone on a line: the bodies of Protocol methods and other stubs, which never run. In CI run 42046754813, 89 such statements were measured and none ran. Over the 346 modules, removing them is the only change this pattern makes.exclude_linesreplaces coverage.py's default, so its first entry is the default pattern itself,\#\s*(pragma|PRAGMA)[:\s]?\s*(no|NO)\s*(cover|COVER). It is written with\#because a value starting with#is read as a comment.relative_files = True. Reports then carry<source>policyengine_api</source>and file names relative to it, instead of the runner's absolute checkout path. diff-cover looks a changed file up by its absolute path or its path from the repository root. A report written in another checkout directory, such as acontainer:job's, would otherwise match no changed file, and the job would pass with "No lines with coverage information".codecov.yml's patch status now shows the sametarget: 80%, stillinformational, since this job enforces the minimum.docs/engineering/skills/testing.mdhas a "Changed lines" section, which the rule cites, with the command to check locally aftermake test.Why
HEAD^1The
pull_requestevent'sbase.shaoften stays at the commit where the branch leftmaster, while the checkout merges intomasteras it is now. In 17 of 88 recentpr.ymlruns the two differed, and in each of thosebase.shawas the older commit. Comparing with it counts every change that reachedmasterin between as the pull request's own. Themasterruleset doesn't require branches to be up to date, so a pull request can merge on that result.Run 42039645804 (#4191) checked out
8b441b8amerged into4f2960b6, withbase.sha22bb5e4d. Between those twomastercommits,masteradded 1,077 lines underpolicyengine_api/. That run predates the integration artifact, so it has six coverage reports, one per test shard. On those six, with their checkout path rewritten to a local copy of the merge commit, the locked diff-cover measured:--compare-branch22bb5e4d(base.sha)HEAD^1(4f2960b6)The 39 lines are #4191's own changes.
In CI, the test shards, the integration tests and this job all check out the same commit at the same path, so the reports' paths match without rewriting. The decision behind the 80% minimum is in tracker task 6.4 · 6: of the last 40 merged pull requests, 13 changed measured code, and only #4179 (75%) would have failed.
Stacked pull requests and the skip
The job has
needsand no condition of its own, astests/unit/test_ci_complete_gate.pyrequires. On a pull request above the lowest one in a stack,testandv2-integration-checkare skipped, sodiff-coverageis skipped too.CI completefails there, as it already does for the other skipped jobs. On the lowest pull request, it runs once both have passed. If either fails, it is skipped andCI completefails with them.Design choices for review
devenvironment (uv run --frozen --extra dev), including the country models, because the decision put diff-cover in thedevextra. A dependency group of its own (uv run --frozen --only-group, astelemetry-error-issuesdoes) would install only diff-cover. That would make the job faster but leave diff-cover out ofmake install. Say if you prefer it.policyengine_api/count, because.coveragercmeasures nothing else. A pull request that changes only tests, workflows or docs passes.# pragma: no coveror a pattern to.coveragerc'sexclude_lines, or change the job or its--fail-undervalue along with the test that pins it. Themasterruleset doesn't require code-owner review, so none of these files needs a particular reviewer. Code that runs only in the Alembic checks, the environment-variable tests,push.yml's live tests or a subprocess counts as uncovered. Moving code into a new file counts its lines as changed, unless git pairs the new file with a deleted one as a rename (at least 50% similar), when only the edited lines count. The testing skill documents these.tests/unit/test_runtime_evaluated_annotations.pywalks the packages withpkgutil.walk_packages, which skippedlibs/androutes/while they had no__init__.py. It now imports their 15 modules too. Its 5 tests passed by node ID in a review run.__init__.pyfiles rather than a coverage.py upgrade. coverage.py 7 has[report] include_namespace_packages, which would report these modules without the two files. Nothing holds coverage.py at 6.5.0; it is simply the locked version. Upgrading it would change how every CI test run is measured, so I kept that separate. The test that every module is reported fails for any future directory underpolicyengine_api/without an__init__.py. Ruff's INP001 (implicit namespace package) can't catch that, becausepyproject.tomldeclares all ofpolicyengine_apia namespace package.CI wiring and environment variables
HEAD^1, from the checkout itself.RUNNER_TEMP: the runner's default variable, where the two download steps write (${{ runner.temp }}/coverage).UV_PYTHON: the literal"3.12"in the job'senv:, as the other jobs that run uv set it.testand the reusablev2-integration-check), and downloading them needs no secret.env:and uses neither variable there:ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION(the literaltrue) andOLD_SIMULATION_GATEWAY_URL(the repository Actions variable of that name, which exists). The job doesn't authenticate to GCP or Codecov.actions/checkoutandastral-sh/setup-uvuse the automaticgithub.tokenwith the job'scontents: read; the downloads use the runner's token for the same run's artifacts.actions/checkout@v4withfetch-depth: 0gives the merge commit's first parent for diff-cover'sgit diff HEAD^1...HEAD; a depth-1 clone lacks it.Testing
Red, at the first commit (tests and fixtures only):
diff-cover wasn't installed, and there was no job, rule or patch target yet. Green, at the second commit, the same 4 pass.
Review fixes, red at
72485ef66(tests only), with the 8 node IDs intests/unit/test_diff_coverage.py, 4 failed:test_the_job_measures_only_the_lines_the_pull_request_changes: 90%, exit 0, withmaster_change.pycounted (should be 50%, exit 1)test_coverage_reports_every_module_no_test_imports: the 15 modules inlibs/androutes/unreportedtest_type_only_imports_and_pragmas_are_not_counted_as_missed: theTYPE_CHECKINGimport counted as missedtest_pull_requests_check_the_coverage_of_their_changed_lines: the step still readBASE_SHAtest_a_line_counts_as_covered_if_any_report_covers_italready passed. Green at1eda3287f: all 8 pass.Second review round, red at
d91472a2e(tests and fixtures only), with the 9 node IDs, 2 failed:test_reports_match_the_changed_files_in_another_checkout: "No lines with coverage information in this diff.", exit 0 (should be 50%, exit 1)test_type_only_imports_and_pragmas_are_not_counted_as_missed: the#pragma:no coverand# PRAGMA NO COVERlines counted as missedThe proof at 799 of 1,000 lines, and the check that
pr.ymlruns only onpull_request(whichHEAD^1relies on), passed already. Green atabbfd7c46: all 9 pass.Third review round, red at
729351590(tests only):test_only_lines_that_can_run_are_countedfailed with missing lines[23](the Protocol stub) instead of[27](the untested function whose comment mentionsif TYPE_CHECKING:). Green ateedee597a: all 9 pass.Also run locally:
tests/unit/test_codecov_coverage.pyfrom Measure the unit tests' coverage on master and flag every Codecov upload #4196, which read the changedcodecov.ymlandpr.yml: pass;python scripts/run_quality_guards.py: every guard passes;python -m scripts.rules guard --base-ref origin/quality/codecov-flags: passes, noting the new rule;uv lock --checkwith uv 0.13.4,ruff check .,ruff format --check ., Codecov's validator, and the changelog fragment check;__init__.pyfiles:mypy(190 files, no issues),lint-imports(5 contracts kept), and importing both packages.Per this workstream's rules, only the new and touched tests were run locally. The rest, including
tests/unit/test_ci_complete_gate.py, run in CI once this PR is the lowest in its stack. Until then the full workflow, and so thediff-coveragejob itself, hasn't run in CI. The tests above run the job's exact command.Codecov after this PR
libs/androutes/modules no integration test imports are now reported, with no hits.if TYPE_CHECKING:imports and stub bodies no longer count as missed.codecov.ymlsets no status per flag, so the project status uses the combined figure.<source>policyengine_api</source>to the repository's files. Each name is the repository path once joined with the source, so it should.🤖 Generated with Claude Code