Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure GitHub to require codecov checks #1727

Closed
pquentin opened this issue Oct 31, 2019 · 6 comments
Closed

Configure GitHub to require codecov checks #1727

pquentin opened this issue Oct 31, 2019 · 6 comments

Comments

@pquentin
Copy link
Member

Since the codecov reports are noisy, we frequently merge pull requests that lower coverage or even completely break coverage upload.

Indeed, we're trained to look at status checks, but not at the coverage report. But codecov is able to report its results with a status check too:

Capture d’écran 2019-10-31 à 17 01 46

But how can we enable that? I think the trick is to add a branch protection rule in Settings > Branches > Add rule and require the travis and appveyor statuses.

Here's an example from pquentin/urllib3:

Capture d’écran 2019-10-31 à 16 55 56

For python-trio/trio and python-trio/urllib3, it looks like setting this was enough to see codecov checks. We can try to enforce the codecov checks too, and decide to sometimes ignore them when merging. Trio does not do that because they're actually quite noisy.

@Zac-HD
Copy link

Zac-HD commented Nov 7, 2019

It looks like you're really close to full coverage - have you considered failing the build if it's less than 100%? We've found knowing there are zero lines not covered really helpful for Hypothesis. (and judicious use of # pragma: no cover doesn't mess with this; not least because it's greppable)

@pquentin
Copy link
Member Author

pquentin commented Nov 7, 2019

@Zac-HD We had 100% coverage for some time now, and we'll actually be back at 100% coverage with #1726. The problem we face is that we have OS-specific code, so there's no one build at 100% coverage, it's only the combination that gets us to 100%. Which is why we have to rely on codecov.

@Zac-HD
Copy link

Zac-HD commented Nov 7, 2019

Right! To be explicit: I think using a status check is a great idea, and the only one you need is "does the project have 100% coverage" (of non-pragma'd branches) 😄

@pquentin
Copy link
Member Author

pquentin commented Nov 7, 2019

Ah, I see! You're not suggesting failing individual jobs using say a pytest plugin, you're suggesting that we require the codecov GitHub status check before allowing merging pull requests?

Well, the problem here is that codecov is not always reliable, so requiring this check can sometimes block pull requests for bad reasons. But we could try and see how much this is a problem in practice. Sometimes it's enough to just close/reopen the pull request to fix this kind of issues.

@pquentin
Copy link
Member Author

pquentin commented Apr 1, 2020

Works as expected in #1806, yay! Thanks @shazow for setting this up. (It turns out @shazow does a lot of thankless but yet invaluable work behind the scenes.)

Forgetting about coverage will no longer be an option. :) We may need to relax some requirements if codecov fails at some point, but let's wait until this actually happens, as I believe codecov is more reliable these days.

@pquentin pquentin closed this as completed Apr 1, 2020
@shazow
Copy link
Member

shazow commented Apr 1, 2020

Very welcome, happy to help. ❤️ 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants