fix(ci): enforce the coverage threshold once, where the bypass can reach it - #242
Closed
gcko wants to merge 1 commit into
Closed
fix(ci): enforce the coverage threshold once, where the bypass can reach it#242gcko wants to merge 1 commit into
gcko wants to merge 1 commit into
Conversation
…ach it DRC-4278 asked whether the threshold should be enforced twice, on two interpreters. Answering it turned up something better: the documented `coverage-exception` bypass has never worked. The `test` job ended its suite step with a bare `coverage report`. That reads `fail_under` from pyproject and exits non-zero, and it ran BEFORE the "Check coverage-exception label" step and before the exception-aware step that actually honours the label. So a PR below threshold failed at that line and never reached its own bypass. AGENTS.md documents the label as a way to merge below threshold; it was not one. `runtime-floor` carried a second copy of the same bare call, pre-empting it again. Demonstrated rather than argued: `coverage report --fail-under=99` exits 2, `coverage report --fail-under=0` exits 0. The first is the line removed here; the second is the form the bypass uses. The threshold is now enforced in one place, the step that knows about the label. On the original question, per the decision recorded on DRC-4278: runtime-floor stops running the suite under `coverage`. Nothing consumed its coverage data, and nothing in the product branches on `sys.version_info`, so the second measurement told us nothing the 3.12 run did not. What the job is for is the floor itself, and it still does that: the direct-launch smoke plus the whole suite on 3.11. It can then install validation requirements only. The rest of requirements-dev.txt is ruff, mypy and coverage, which run on 3.12 in their own jobs; PyYAML stays because scripts.tests.test_validate_plugins imports the validator. The pip cache added in #241 is repointed to match what the job now installs. All seven script test modules are kept. `platform-tests` runs only five, and copying its shape here would have quietly dropped test_bench_event_latency, which #241 had just finished adding, and test_derive_prompt_shapes. Verified by running the job's new commands on this tree: 2,006 dashboard tests and 192 script tests, green, with no dev toolchain involved beyond the interpreter. Implements DRC-4278. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@variable.team>
Contributor
CoverageThreshold: |
Contributor
Author
|
Merged as
Verified before closing:
Closing rather than re-merging, which would either be rejected as having no commits or duplicate the change. DRC-4278 is delivered. |
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
1 participant
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.
Implements DRC-4278 — Decide whether the coverage threshold should be enforced twice, on two interpreters.
The decision, and the better bug behind it
The ticket asked a yes/no question. Answering it surfaced something worth more than the answer: the
coverage-exceptionbypass thatAGENTS.mddocuments has never worked.The
testjob ended its suite step with a barecoverage report. That readsfail_underfrompyproject.tomland exits non-zero — and it ran before the "Check coverage-exception label" step and before the exception-aware step that honours the label. A PR below threshold failed at that line and never reached its own bypass.runtime-floorcarried a second copy of the same bare call, pre-empting it again.So the label was documented as a way to merge below threshold and was not one.
The decision itself
runtime-floorstops running the suite undercoverage. Nothing consumed its coverage data —coverage json, the artifact, the label check and the sticky comment all live intest— and nothing in the product branches onsys.version_info, so a second measurement on 3.11 told us nothing the 3.12 run did not. The job's purpose is the floor, and it still does that: the direct-launch smoke plus the whole suite on 3.11.This does not lower the ratchet.
fail_underis untouched at 73; it is now enforced once, in the step that can see the label.It also lets the job install validation requirements only. The rest of
requirements-dev.txtis ruff, mypy and coverage, all of which run on 3.12 in their own jobs. PyYAML stays becausescripts.tests.test_validate_pluginsimports the validator. The pip cache added in #241 is repointed to match.All seven script test modules are kept. The ticket suggested copying
platform-tests' step shape; that job runs only five, so copying it would have silently droppedtest_bench_event_latency— which #241 had just finished adding — andtest_derive_prompt_shapes.Verification
This change edits only workflow YAML and one
AGENTS.mdsentence, so there is no product behaviour to pin with a test. Nothing in the repo readsquality-gate.yml(grep -rn "quality-gate.yml" --include="*.py" .is empty). Evidence instead:coverage report --fail-under=99exits 2;coverage report --fail-under=0exits 0. The first is the line removed here, the second is the form the bypass uses — so the removed line provably aborted the job before the label was read.python -m unittest discover -s cargento/skills/cargento/tests -t .→ 2,006 tests, OK. The seven script modules → 192 tests, OK. Neither needs anything fromrequirements-dev.txt.validate_pluginsneeds PyYAML, which is inrequirements-validation.txt.actionlint(required check) is the parser-level proof the YAML is well formed.ruff check .andscripts/validate_plugins.pyclean.Note for the reviewer
The one thing worth checking hardest is that removing the bare
coverage reportfrom thetestjob does not remove enforcement. It does not: theelsebranch of "Coverage report and threshold" still runs a plaincoverage report, which fails below threshold exactly as before. What changed is that a labelled PR can now reach it.