Repository navigation
feat: add coverage reporting with a ratcheting CI gate - #15
Conversation
Benkapner
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Coverage reporting is something the workspace needs, and the ruff fix in here is a genuine unblock (more on that below).
i checked the branch out and ran it locally, and there are a few things that need to change before this can land. There are no CI statuses on the PR yet, so none of this has been exercised on GitHub either.
1. There is no threshold in the diff
The full change to ci.yml is:
- - name: Run tests
- run: uv run pytest -v
+ - name: Run tests with coverage
+ run: uv run pytest -v --cov --cov-report=termThere is no --cov-fail-under=80, not in ci.yml and not in pyproject.toml. Coverage is printed, never enforced, so CI passes at any percentage including 0%.
The title says "80% threshold" and the body says "Enforce 80% coverage threshold" and "Fail CI if coverage falls below threshold." Right now the PR adds the reporting but not the gate.
2. The threshold would not pass if it were added
Running the PR's exact command on this branch:
TOTAL 333 99 70%
177 passed in 2.30s
70%, below the 80% in the title. Adding --cov-fail-under=80 on top of this branch turns CI red immediately. And 70% is optimistic, see the next point.
3. Bare --cov measures the wrong set of files
With no source specified, pytest-cov measures whatever gets imported during the run. That has two effects here, and both undercut the gate.
Test files end up in the denominator. tests/test_command_prompts.py (93%) and tests/test_workspace_scripts.py (100%) are counted as covered code. Tests always cover themselves. Scoping to the actual workspace code:
$ uv run pytest --cov=.ai-workspace --cov-report=term
.ai-workspace/lib/config.py 26 5 81%
.ai-workspace/lib/repository_status.py 138 138 0%
.ai-workspace/lib/session_context.py 20 20 0%
.ai-workspace/scripts/transpile-commands.py 138 87 37%
------------------------------------------------------------
TOTAL 322 250 22%
Real coverage is 22%. The whole 48-point gap is test files measuring themselves.
Unimported modules disappear from the report entirely. align-workspace.py, setup.py, session-start.py and mktmpdir.py show up in neither report, because no test imports them. They are not reported at 0%, they are just absent. session-start.py runs on every session, and repository_status.py is the largest module in the workspace at 138 statements and measured 0%.
So the number is highest exactly where the testing is thinnest. A threshold on this metric would certify the untested parts as healthy, which is worse than having no threshold at all.
4. The XML report in the description is not in the diff
The body says "Add pytest coverage reporting (XML and terminal)" but only --cov-report=term is here. Looks like cdb92ca dropped it. Harmless today since nothing consumes coverage.xml, but worth either restoring or editing the description.
5. The markdown changes are unrelated, and correct
skills/python-patterns/references/{api-clients,testing}.md have nothing to do with coverage, so normally i would ask to split them out. i checked before assuming, and they are a real fix: ruff format --check . currently fails on origin/main for those two files, so main is red right now and this PR incidentally repairs it. Good catch.
The root cause is worth a separate fix though. pyproject.toml pins ruff>=0.11 and CI resolves to 0.16.9, which formats Python inside markdown code blocks. main went red with no code change at all, purely from a transitive ruff upgrade. Any unpinned formatter behind a --check gate will do this again.
Suggested split
PR A, pin ruff and unblock main. The two markdown reformats already in 051b4ed, plus:
"ruff>=0.16,<0.17",PR B, the coverage gate. Scope the measurement first, then pick a threshold that reflects reality:
[tool.coverage.run]
source = [".ai-workspace"]
omit = ["*/tests/*"]
[tool.coverage.report]
fail_under = 50 # 22% today; ratchet upward as tests land- name: Run tests with coverage
run: uv run pytest -v --cov --cov-report=term --cov-report=xmlThe threshold number is the main thing to get right. 80% against 22% actual coverage means either the gate gets deleted the first time it fires, or this PR has to carry ~250 statements of new tests. A ratchet starting near the current number and moving up as coverage improves is the version that survives contact with the repo.
Happy to pair on PR B if that helps, the repository_status.py and session-start.py gaps are the highest-value tests to write first.
|
Thanks for the detailed feedback, Ben! I've opened a new PR #16 to handle PR A as you suggested:
I'm ready to start on PR B next to implement coverage properly. Before I dive in, I want to clarify your point about the threshold: So for PR B, should we start with ~22% as the initial threshold (close to actual coverage) and gradually ratchet it up as the codebase improves? Or what do you think the best starting point would be? |
Bare `--cov` counted tests/ as covered code, reporting 70% while actual coverage of .ai-workspace is 22%. Scope coverage to the workspace source via [tool.coverage.run], add the XML report the PR description promised, and set an enforceable --cov-fail-under floor. The 80% target is kept as a ratchet goal rather than the initial floor, since an 80% gate would fail CI on day one at current coverage.
|
Pushed The gate did not exist. The diff added The measured number was inflated. Bare The 70% is mostly the test files reporting themselves at 93 to 100%. What I pushed:
I set the floor at 20 rather than 80 deliberately. At 22.36% actual, an 80% gate fails on the first CI run and the PR cannot merge. A floor slightly under the current number blocks regressions immediately and can be raised as coverage improves. I have retitled the PR accordingly, since "80% threshold" no longer describes it. To raise the floor later, the two files to attack are One caveat on the denominator: coverage only auto-discovers unimported files inside Verification: all four CI jobs pass locally. Test job reports |
|
Thanks @Aliciapet11 for opening this. Getting a coverage signal into CI is the right instinct and it supports the quarterly quality-gate KPI. Here is what i did on it, so nothing is a surprise: Pushed
Set the floor at 20 instead of 80. At 22.36% actual, an 80% gate fails on the first CI run and the PR cannot merge. A floor just under the current number blocks regressions starting today, and the 80% target is now recorded as a ratchet comment in Retitled the PR, since "80% threshold" no longer describes what it does. To raise the floor later, the two files to go after are All four CI jobs pass locally. Merge order across the open PRs is #16, then #17, then this one. |
# Conflicts: # pyproject.toml
Add code coverage reporting and enforce a coverage floor in CI.
Changes:
pytest-covto dev dependencies.ai-workspacevia[tool.coverage.run]--cov-fail-under=20in the test jobOn the threshold
This PR originally claimed an 80% gate. Measured coverage does not support that yet:
--covtests/as covered code)--cov=.ai-workspace--cov-fail-under=80would have failed CI on the first run. The gate now starts at the achievable 20%, which blocks regressions from day one, and the 80% target is documented as a ratchet inci.yml.Biggest gaps if someone wants to raise the floor:
.ai-workspace/lib/repository_status.py(138 statements, 0%) and.ai-workspace/lib/session_context.py(20 statements, 0%).This supports the quarterly KPI: every repo should have a quality gate.
Verification: all four CI jobs pass locally. Coverage run reports
Required test coverage of 20% reached. Total coverage: 22.36%.