feat: ensure that tests have 100% coverage (i.e. no dead code in tests) - #3128
feat: ensure that tests have 100% coverage (i.e. no dead code in tests)#3128paul-nechifor wants to merge 1 commit into
Conversation
7b2009c to
0bd4859
Compare
Greptile SummaryThis PR adds a CI gate for detecting test code that never runs. The main changes are:
Confidence Score: 5/5No additional blocking issue qualifies for this follow-up review.
Important Files Changed
Reviews (2): Last reviewed commit: "feat: ensure test coverage" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # line-covered by the union of all CI pytest jobs' coverage data. | ||
| # No `if:` needed: plain `needs` skips this job whenever an input job | ||
| # is skipped (fork PRs skip self-hosted-tests) or failed/cancelled. | ||
| needs: [tests, self-hosted-tests, rust] |
There was a problem hiding this comment.
self-hosted-tests is skipped for fork pull requests, and this unconditional needs dependency causes GitHub to skip the coverage gate too. A fork PR can therefore add or orphan a test file without this check running against the available test and Rust coverage.
0bd4859 to
3076de9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #3128 +/- ##
==========================================
+ Coverage 73.14% 73.65% +0.51%
==========================================
Files 1033 1033
Lines 93633 93054 -579
Branches 8518 8583 +65
==========================================
+ Hits 68487 68543 +56
+ Misses 22806 22170 -636
- Partials 2340 2341 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| python -m pytest -c /dev/null --rootdir . --noconftest --import-mode=importlib -p no:cacheprovider -v \ | ||
| # Under coverage so the test-file-coverage gate can prove these | ||
| # files run: they importorskip themselves out of every other job. | ||
| python -m coverage run -m pytest -c /dev/null --rootdir . --noconftest --import-mode=importlib -p no:cacheprovider -v \ |
There was a problem hiding this comment.
Why does this skip the conftest in the first place...?
There was a problem hiding this comment.
This feels rather messy and convoluted. I'd much rather just have a path split in codecov and just add that as a required status check. No additional code needed. No extra CI steps needed.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, okay. Do you want to do that?
There was a problem hiding this comment.
I'll add a task to look at it (looks like we can use regex or globbing there), though I still think it'd be much cleaner to have a separate tests/ directory like other Python projects.
github-actions
Bot
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
2 participants
Closes DIM-1318
Problem
We need to make sure that all tests are executed.If they're not, they're dead code and should be deleted.
Solution