feat: Code Coverage Reporting via CI #167
Conversation
nathan-weinberg
requested review from
cdoern,
nathan-weinberg,
danmcp,
alimaredia and
bjhargrave
courtneypacheco
force-pushed
the
dev-docs-ci-code-coverage
branch
3 times, most recently
from
5dc709c to
e818651
Compare
This document provides design suggestions for CI code coverage reporting, as well as describe high-level goals. Also, I've added unrecognized words to the spell checking dictionary Signed-off-by: Courtney Pacheco <[email protected]>
courtneypacheco
force-pushed
the
dev-docs-ci-code-coverage
branch
from
e818651 to
06fd0a3
Compare
|
|
||
| * Code coverage should be reported in PR builds as a GitHub _bot_ comment | ||
| * New code: | ||
| * Should show % coverage on new lines of code. |
There was a problem hiding this comment.
Should we require this explicitly for all repos? There are some repos like instructlab/training which are challenging to test meaningfully for certain aspects due to how we expect them to run (distributed, multi-processed).
Maybe it would make more sense to uphold a certain % of coverage maintained? So that if new changes increase the lines, we are not falling below a certain threshold.
There was a problem hiding this comment.
This doesn't seem to be suggesting a requirement, just a communication of how much of the new code is being covered by testing
| * Free to use in open source projects. | ||
| * Codecov reports all the data on GitHub in a comment. [Example](https://docs.codecov.com/docs/pull-request-comments). | ||
| * Cons / Callouts: | ||
| * Not applicable at this time, but if we want to use it in >1 private repo, we must purchase a customer plan. |
There was a problem hiding this comment.
We don't have any private repos (or plan to), so this shouldn't affect us.
| * Pros: | ||
| * Free to use via [MIT license here](https://github.com/coverallsapp/github-action/blob/main/LICENSE.md). | ||
| * Appears to be free for open source repos, but like with Codecov, we should validate that the ToS doesn’t have fine print, etc. | ||
| * Can report all the data in a GitHub comment. [Example](https://github.com/coverallsapp/coveralls-node-demo/pull/19#issuecomment-1035344985). |
There was a problem hiding this comment.
This is a really nice addition.
| * Appears to be free for open source repos, but like with Codecov, we should validate that the ToS doesn’t have fine print, etc. | ||
| * Can report all the data in a GitHub comment. [Example](https://github.com/coverallsapp/coveralls-node-demo/pull/19#issuecomment-1035344985). | ||
| * Cons / Callouts: | ||
| * Code coverage visuals, etc. are all viewed through the Coveralls front end (coveralls.io), so if that service were to go down, then we’re technically at their mercy. |
There was a problem hiding this comment.
If it's FOSS/MIT licensed, could we not stand up our own instance?
There was a problem hiding this comment.
True, but I'd prefer to avoid the overhead (is possible)
|
I am not sure the utility of this. Unless it is someone's job to increase the coverage amount, the coverage amount is just a metric no one cares about and is more emails from GitHub for each PR run. I don't think I have ever worked on an open source project where coverage reporting has ever been really helpful. A lighter weight idea would be to simply upload the coverage data/reports as a build artifact which someone can go get if they really care. For example (the example is test reports, but same basic idea): |
Yep, I know what you mean about metrics. I don't think the number itself tells us much about code quality either because in theory, a repo can have "100% code coverage," yet still suffer quality issues. After all, if we write code wrong and then write our unit tests wrong as well, it is possible that the unit tests pass when they shouldn't. However, if the code coverage is sitting at 53% and a contributor creates a large PR that reduces the overall code coverage to 30% (as a gross exaggeration), then we might want to look at it with more critical eyes. Same if someone creates a PR that adds 200 lines with only 5% code coverage. Ultimately, my goal is not to bombard anyone with tons of emails, especially from a code coverage bot! But I do want to make it easy for contributors and reviewers to see if a certain file or critical piece of code was accidentally left untested. Yes, contributors can look at the actual code coverage report file instead of us having a bot commenting about untested code lines, but having coverage information summarized right in the PR would likely be helpful for PR reviewers. What do you think? |
Yes, but adding the comments sends emails. :-( In any case, we need to be careful in the GitHub workflows to avoid security issues which can lead to supply chain attacks. |
Yep, agreed. A couple of options off the top of my head to mitigate the number notifications:
For security concerns regarding workflows: Ansible, Santa and other popular open source repos do actively use Codecov or Coveralls to assess code coverage. I can definitely look into how these repos have mitigated security risks via their workflow logic and work to identify any concerns we may have if we were to introduce such workflows in any of our repos. |
|
|
||
| * Code coverage should be reported in PR builds as a GitHub _bot_ comment | ||
| * New code: | ||
| * Should show % coverage on new lines of code. |
There was a problem hiding this comment.
This doesn't seem to be suggesting a requirement, just a communication of how much of the new code is being covered by testing
| * Free to use in open source projects. | ||
| * Codecov reports all the data on GitHub in a comment. [Example](https://docs.codecov.com/docs/pull-request-comments). | ||
| * Cons / Callouts: | ||
| * Not applicable at this time, but if we want to use it in >1 private repo, we must purchase a customer plan. |
| * Appears to be free for open source repos, but like with Codecov, we should validate that the ToS doesn’t have fine print, etc. | ||
| * Can report all the data in a GitHub comment. [Example](https://github.com/coverallsapp/coveralls-node-demo/pull/19#issuecomment-1035344985). | ||
| * Cons / Callouts: | ||
| * Code coverage visuals, etc. are all viewed through the Coveralls front end (coveralls.io), so if that service were to go down, then we’re technically at their mercy. |
There was a problem hiding this comment.
True, but I'd prefer to avoid the overhead (is possible)
| * Explains what isn’t covered and gives detailed suggestions on how to increase code coverage. (Seems better than Codecov, but… it’s also a more refined product?). | ||
| * Keeps a running history of your code coverage reporting, if desired. | ||
| * Cons / Callouts: | ||
| * Appears to be "free to use," but Sonar wants you to store coverage data in their cloud and storage costs will apply unless you agree to host your own Sonar service -- in which case, we'll need to pay for a license. (Not ideal.) |
There was a problem hiding this comment.
I say we avoid this one
|
|
||
| ## Code Coverage Reporting Bot: Options | ||
|
|
||
| ### [Codecov](https://github.com/marketplace/codecov) |
There was a problem hiding this comment.
This would be my preference
This document provides design suggestions for CI code coverage reporting. Initially, the high-level goal is to provide a report in each pull request to highlight:
mainfor each InstructLab repoWe can ultimately decide if we want to enforce a minimum coverage % in PRs, but for now, I just want to report the coverage for informational purposes