Skip to content

Prevent codecov from notifying of failure too soon#9031

Merged
jcrist merged 1 commit intodask:mainfrom
jcrist:fixup-codecov
May 5, 2022
Merged

Prevent codecov from notifying of failure too soon#9031
jcrist merged 1 commit intodask:mainfrom
jcrist:fixup-codecov

Conversation

@jcrist
Copy link
Copy Markdown
Member

@jcrist jcrist commented May 5, 2022

Currently codecov pushes an initial failing status on every PR that is
later updated to passing once more CI builds finish. This is annoying.

As far as I can tell from https://docs.codecov.com/docs/merging-reports
this shouldn't be happening, but it is. Here we attempt to stop this
behavior by:

  • Only running code coverage on the main test suite that hits most of
    the codebase.
  • Setting a minimum number of builds before codecov should push a status
    update.

Hopefully this fixes the problem.

Currently codecov pushes an initial failing status on every PR that is
later updated to passing once more CI builds finish. This is annoying.

As far as I can tell from https://docs.codecov.com/docs/merging-reports
this shouldn't be happening, but it is. Here we attempt to stop this
behavior by:

- Only running code coverage on the main test suite that hits most of
  the codebase.
- Setting a minimum number of builds before codecov should push a status
  update.

Hopefully this fixes the problem.
@jcrist jcrist added the tests Unit tests and/or continuous integration label May 5, 2022
@jcrist jcrist merged commit cac9d6b into dask:main May 5, 2022
@jcrist jcrist deleted the fixup-codecov branch May 5, 2022 15:23
Copy link
Copy Markdown
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree the early red "X" isn't great. Did we need to drop coverage on the additional builds, or would increasing after_n_builds higher to like 5 or 6 be sufficient?

@jcrist
Copy link
Copy Markdown
Member Author

jcrist commented May 5, 2022

We could have done that, but the mindeps builds are a subset of the normal builds afaict, and so extra coverage there shouldn't be necessary.

@jrbourbeau
Copy link
Copy Markdown
Member

The mindeps builds are a subset, but they provide coverage for guarded imports and some version-dependent logic like

if not PANDAS_GT_110:

To be clear, this definitely isn't many lines of code, but I don't see why we shouldn't include it if it doesn't introduce problems elsewhere

erayaslan pushed a commit to erayaslan/dask that referenced this pull request May 12, 2022
Currently codecov pushes an initial failing status on every PR that is
later updated to passing once more CI builds finish. This is annoying.

As far as I can tell from https://docs.codecov.com/docs/merging-reports
this shouldn't be happening, but it is. Here we attempt to stop this
behavior by:

- Only running code coverage on the main test suite that hits most of
  the codebase.
- Setting a minimum number of builds before codecov should push a status
  update.

Hopefully this fixes the problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Unit tests and/or continuous integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants