Coverage report in Github CI for standard library

How does everyone feel about adding a new CI job with coverage report using coverage.py?

It came to me after a bug report was filed against a piece of code I wrote but missed the tests for, mostly because other projects that I work on has a CI to remind me of missing coverage.

I don’t know if others would be okay with making it a mandatory check, but it would be nice to at-least have the information available for the interested module maintainers.

We could use a 3rd party service, like codecov.io, which other sub projects under Python seem to be using. I personally haven’t used them on any of my projects, but I don’t enjoy the very verbose output from their bot in every single PR. Not sure if it can be customized to just a regular Github check with URL to details or not.

I would prefer to just go with diff-cover library which can analyze the git diff of the PR with the coverage’s XML report to determine if any lines in the PR are missing the tests. We have been using diff-cover with Mailman project for quite some time and it has been great!

I am totally open to any other suggestion that y’all might have.

Thoughts?

FYI, we are using codecov.io already.
See https://codecov.io/gh/python/cpython

I think that a mandatory check might be a bit too obstructive, but I would definitely welcome some form of GitHub PR integration with the existing codecov.io, if that’s available. Otherwise, some other form of code coverage service with GitHub PR integration would work as well.

The coverage run takes a long time to execute.

1 Like

I did not know about this, thanks!

Was any decision made about this? At the moment it is hard to see if a PR has full test coverage without clicking through to the CI, so I think adding this should make code reviews easier. Maybe the PR comment would be too obstructive, but I have seen other libraries use codecov as one of the checks and that seems to work pretty well. Anyone reviewing the PR can then see straight away if new code isn’t fully covered.

The decision was people hated the comment.

Fair enough. What about adding codecov as one of the checks instead? (see https://docs.codecov.io/docs/commit-status)

I think you may have to set a minimum threshold for this to work, but it could always be set generously. I imagine that pretty much all new code should have close to 100% coverage, although there will obviously be some exceptions.

Pinging @vstinner around https://bugs.python.org/issue40993

Looks like we recently disabled codecov for pull requests. It looks like it’s a feature request here and I’ve made some use of it in the past to show that a particular piece of C code was dead and unreachable.

Arguably we get pretty similar information from it running after merge but does anyone here have any strong opinions on running it on pull requests? It does add a fair bit of overhead in terms of CI resource usage and run-times.

Thanks Ammar, didn’t realise that change had been made. I guess that makes this discussion redundant then!

Seems like things have moved in the complete opposite direction, so that coverage on changed code will not be viewable at all by someone reviewing a PR. This seems like it will lead to fewer lines being covered, and likely more bugs in the future.

Maybe the CI could run the coverage just for modules which have been changed in the PR?

Hi Ammar, I disabled coverage on pull requests because for an unknown reason, Travis CI started to take up to 50 min to report “tests passed” status to GitHub. Since Travis CI is now a mandatory CI, it prevents to merge a PR.

After I disabled coverage, it takes again less than 30 min to be able to merge a PR. The minimum time to merge a PR has a direct impact on our workflow performance.

See also Travis CI doesn't report its status or doesn't run on Python pull requests · Issue #371 · python/core-workflow · GitHub It seems like Travis CI behavior changed last weeks, and I got multiple issues with it.

Moreover, when I asked “who use coverage?” in April, nobody replied and nobody knew how it is supposed to work: https://bugs.python.org/issue40237

So IMO coverage can wait until a change is merged into master.

2 Likes

By the way, C coverage has two issues for at least 2 months:

  • test_distutils fails: https://bugs.python.org/issue40237 “ImportError: (…)/xx.cpython-39-x86_64-linux-gnu.so: undefined symbol: __gcov_merge_add”
  • the whole job is killed by Travis CI because it runs longer than 50 min

Aah gotcha, I believe those jobs were marked as optional to prevent this but something must have changed with Travis. Your fix seems reasonable for now, it’s way better to have faster merge times :slight_smile:

This is true, I guess it’s just a nice to have in the background rather than an integral part of any developer’s workflow. From the sounds of it it looks like it’s becoming a maintenance burden so it seems fine to disable it until someone has the motivation to fix these issues.

Yes, Travis CI behavior changed. Coverage tests were run for a long time on PRs, and previously the status (passed) was reported as soon as all mandatory jobs passed.

But I don’t have the bandwidth to investigate Travis CI, and it’s not like it’s something that I can fix myself.

By the way, I never saw anyone mentioning coverage on a pull requests. I don’t remind any comment like “oh, these lines are not tested, I saw that on coverage”.

I’m not a believer of “100% coverage”, sorry :slight_smile: