Require 90% test coverage on the lines a pull request changes - #5653
Open
bgoncal wants to merge 3 commits into
Open
Require 90% test coverage on the lines a pull request changes#5653bgoncal wants to merge 3 commits into
bgoncal wants to merge 3 commits into
Conversation
Coverage was reported but never enforced, so a pull request could add any amount of untested code and still come back green. Tools/diff_coverage.py intersects the lines a branch adds or changes with the LCOV tracefile the test run already produces. The new patch-coverage job fails and requests changes when fewer than 90% of those lines are run by the unit tests, and dismisses the review once a push clears the bar. Only coverable lines count: comments and declarations carry no execution count, files the unit test targets never build have no coverage data, and Tests, Sources/SharedTesting and the resource directories are excluded to match codecov.yaml. A change with nothing coverable in it passes, and a missing tracefile leaves the gate quiet rather than blocking on a number it could not measure.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The workflow can leave a stale CHANGES_REQUESTED review in place when coverage can’t be measured (missing tracefile/artifact), which can unintentionally keep PRs blocked.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces an enforced “patch coverage” gate in CI so pull requests must have ≥90% unit test coverage on the executable lines they add or modify, using the existing Xcode→LCOV conversion output.
Changes:
- Add
Tools/diff_coverage.pyto compute diff-based coverage fromcoverage.lcovplus a Markdown report and GitHub Actions outputs. - Add a new
patch-coverageGitHub Actions job that downloads the LCOV artifact, measures patch coverage, and (when below threshold) requests changes + fails the job. - Align Codecov’s patch target to 90% (kept informational) and update internal tooling/docs.
File summaries
| File | Description |
|---|---|
| Tools/README.md | Documents the new diff-based coverage tool and its usage/exit codes. |
| Tools/diff_coverage.py | New Python script to compute patch coverage from LCOV + git diff. |
| codecov.yaml | Sets Codecov patch target to 90% while keeping it informational. |
| .github/workflows/ci.yml | Upload LCOV as an artifact; add patch-coverage job to enforce the threshold and manage PR reviews. |
| .agents/skills/ha-ios-workflow-ci/SKILL.md | Documents the new CI patch coverage gate and how to reproduce it locally. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+292
to
+309
| # Coverage that could not be measured must not block: when the conversion or the | ||
| # artifact is missing, this step is skipped, no output is set, and every step | ||
| # below no-ops rather than requesting changes nobody can act on. The step itself | ||
| # is `continue-on-error` because the script exits nonzero to report the shortfall. | ||
| - name: Measure patch coverage | ||
| id: coverage | ||
| if: steps.tracefile.outcome == 'success' | ||
| continue-on-error: true | ||
| env: | ||
| BASE_REF: ${{ github.base_ref }} | ||
| run: | | ||
| git fetch origin "+$BASE_REF:refs/remotes/origin/$BASE_REF" | ||
| python3 Tools/diff_coverage.py fastlane/test_output/coverage.lcov \ | ||
| --base "origin/$BASE_REF" \ | ||
| --threshold "$COVERAGE_THRESHOLD" \ | ||
| --summary "$GITHUB_STEP_SUMMARY" \ | ||
| --github-output "$GITHUB_OUTPUT" | ||
|
|
Comment on lines
+48
to
+50
| Measures how much of a pull request's own diff the unit tests cover, out of the LCOV | ||
| tracefile `xccov_to_lcov.py` writes. CI's `patch-coverage` job gates on it: below 90%, it | ||
| fails and requests changes on the pull request. |
A run with no tracefile skipped the gate but left an earlier request for changes in place, so an unmeasurable run could keep a pull request blocked. Dismiss any outstanding request in that case too, and document the skip.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Failed-test cleanup is skipped, and the bot cannot dismiss its own blocking review.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+400
to
+402
| for (const review of outstanding) { | ||
| await github.rest.pulls.dismissReview({ owner, repo, pull_number, review_id: review.id, message }) | ||
| core.info(`Dismissed review ${review.id}`) |
| patch-coverage: | ||
| name: Patch coverage | ||
| needs: test | ||
| if: github.event_name == 'pull_request' |
The job only needed the tests to have run, not to have passed: with a plain `needs: test` it was skipped whenever they failed, so the run that has no tracefile to measure could not dismiss a request an earlier run left behind. Report a dismissal GitHub turns down as a warning as well, rather than leaving the pull request blocked with nothing in the log saying why.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5653 +/- ##
=======================================
Coverage 35.49% 35.49%
=======================================
Files 1013 1013
Lines 73406 73406
=======================================
+ Hits 26056 26057 +1
+ Misses 47350 47349 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
AI Policy
Select exactly one option that describes AI usage in this contribution:
Summary
Coverage was reported but never enforced, so a pull request could add untested code and still come back green.
A new
patch-coverageCI job measures how much of a pull request's own diff the unit tests run. Below 90% it fails and requests changes, and it dismisses that review once a push clears the bar.Tools/diff_coverage.pycomputes the number from the LCOV tracefile the test run already produces.Only coverable lines count: comments and declarations carry no execution count, files the unit test targets never build have no coverage data, and
Tests,Sources/SharedTestingand the resource directories are excluded to matchcodecov.yaml. A change with nothing coverable in it passes, and a missing tracefile leaves the gate quiet rather than blocking on a number it could not measure.Screenshots
N/A, this only changes CI.
Link to pull request in Documentation repository
Documentation: N/A, no user-facing functionality changes.
Any other notes
Making the job a required check is a branch protection setting, so it needs to be enabled in repository settings. Until then the changes-requested review is what blocks the merge, and a maintainer can dismiss it for code that genuinely cannot be unit tested.