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

[RFC] consider droping codecov #6994

Closed
RonnyPfannschmidt opened this issue Mar 31, 2020 · 15 comments · Fixed by #7294
Closed

[RFC] consider droping codecov #6994

RonnyPfannschmidt opened this issue Mar 31, 2020 · 15 comments · Fixed by #7294
Labels
type: infrastructure improvement to development/releases/CI structure type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@RonnyPfannschmidt
Copy link
Member

With codecov i observe a lot of failures/issues/false positives.

So i would appreciate if we found and switched to a more stable alternative.

@blueyed
Copy link
Contributor

blueyed commented Mar 31, 2020

This is due to flaky coverage - e.g. observed with Windows+xdist - not using xdist makes it stable (for that part). But there are other parts that are racy, would need to be covered explicitly.
You could disable the project status for now.

@RonnyPfannschmidt RonnyPfannschmidt changed the title [RFC] conder droping codecov [RFC] consider droping codecov Mar 31, 2020
@Zac-HD Zac-HD added type: infrastructure improvement to development/releases/CI structure type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature labels Mar 31, 2020
@Zac-HD
Copy link
Member

Zac-HD commented Apr 1, 2020

IMO it's important to have coverage measurements, but we don't need to use an external service.

For Hypothesis we just use the --fail-under=100 option to enforce full code coverage, and explicitly annotate each branch that isn't expected to be covered. This would look noisy at first, but is really helpful in preventing test regressions longer term!

@The-Compiler
Copy link
Member

I don't see how switching to another service would change anything - all that codecov does is presenting the data, if that data is flaky, switching to e.g. coveralls won't change much.

+1 on disabling the PR status check as long as that's the case, though.

@RonnyPfannschmidt
Copy link
Member Author

im fine with that - given the presented arguments i believe status checks should pause but at the same time we should open a project for trying for 100% reported coverage (for which well placed+reasoned coverage ignores may be acceptable)

@Zac-HD
Copy link
Member

Zac-HD commented Apr 1, 2020

we should open a project for trying for 100% reported coverage (for which well placed+reasoned coverage ignores may be acceptable)

I'd approach this from the other direction:

  1. Add an new ignore pattern, e.g. # pragma: TODO cover this
  2. --fail-under=100
  3. Standing ticket for contributors to write a test that allows for an instance to be removed, or (rarely) convert it to a standard # pragma: no cover. This would be a great one for sprints or new contributors generally.

This involves a little more code churn, but means we get to enforce 'all new code must be covered' instantly. It's also pretty easy to check our progress with git grep, remove the ignore patter to check progress, and so on.

@RonnyPfannschmidt
Copy link
Member Author

@Zac-HD i love that suggestion

from my pov it expands mine by enabling us to place coverage todos and issues while trying to enforce full coverage on new commits

so in effect putting much better and stricter tooling into the project i proposed

the project would just group the issues/tickets/pr's around sorting out coverage

@nicoddemus
Copy link
Member

I also like the idea of ensuring full coverage for new commits, and the proposed solution by @Zac-HD would take care of getting they flaky coverage out of the way.

FWIW the coverage checks are not required at the moment:

image

@blueyed
Copy link
Contributor

blueyed commented Apr 1, 2020

codecov's diff status is usually correct. It defaults to requirering increased coverage, and could be bumped to 100%.
The project status could be disabled. It will naturally also be red for when you remove code etc, and the total coverage drops.
I find codecov much more easier to revisit then manual coverage reports (which you would need to combine then) - that's also a problem with "--fail-under" then: some things are only covered by a subset of jobs etc.

@RonnyPfannschmidt RonnyPfannschmidt added this to Review in progress in full coverage Apr 3, 2020
@RonnyPfannschmidt RonnyPfannschmidt moved this from Review in progress to In progress in full coverage Apr 3, 2020
@RonnyPfannschmidt
Copy link
Member Author

i created https://github.com/pytest-dev/pytest/projects/6 to organize tracking this effort

@bluetech
Copy link
Member

bluetech commented Apr 8, 2020

I find the "patch" report useful, it just shows which parts of the diff are covered and fails if something isn't. But the "project" one, not so much, often it fails without any apparent reason and no way to fix in the PR.

@sobolevn
Copy link
Member

There's also https://github.com/wemake-services/coverage-conditional-plugin to cover some complex cases.

@gnikonorov
Copy link
Member

+1 for disabling codecov/project status but keeping codecov/patch

@nicoddemus
Copy link
Member

How about this then:

  1. Disable codecov/project, keep codecov/patch: this is really quick to do and will provide immediate benefits.
  2. In a separate PR, we go with @Zac-HD's suggestion of annotating all places missing coverage, and then we can enable codecov/project again.

I propose this because it might take a while to get to 2, while we can ripe the benefits of 1 fairly quickly.

@bluetech
Copy link
Member

Sounds good to me 👍

@gnikonorov
Copy link
Member

I like this idea

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 2, 2020
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 2, 2020
full coverage automation moved this from In progress to Done Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: infrastructure improvement to development/releases/CI structure type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
Status: Done
full coverage
  
Done
Development

Successfully merging a pull request may close this issue.

8 participants