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

enforce positive coverage #1993

Merged
merged 5 commits into from Jan 2, 2022
Merged

enforce positive coverage #1993

merged 5 commits into from Jan 2, 2022

Conversation

akaszynski
Copy link
Member

@akaszynski akaszynski commented Jan 1, 2022

Resolves #1906 by enforcing a diff of 0%, which means that the change in coverage should not go down.

As commented in #1906 (comment), we cannot enforce a positive as some commits may not incur a diff change (e.g. style changes or documentation).

Also updates our codecov upload to use a github action as the bash uploader is deprecated. See:
https://docs.codecov.com/docs/about-the-codecov-bash-uploader

@adeak
Copy link
Member

adeak commented Jan 1, 2022

What does "enforce 0%" mean? What is there to enforce about "at least 0% has to be covered"? I'm probably missing the whole point here, but I can't figure it out on my own.

@akaszynski akaszynski changed the title enforce 0% coverage diff enforce positive coverage Jan 1, 2022
@codecov
Copy link

codecov bot commented Jan 1, 2022

Codecov Report

Merging #1993 (1a6a68f) into main (5828416) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1993   +/-   ##
=======================================
  Coverage   92.99%   92.99%           
=======================================
  Files          74       74           
  Lines       15482    15482           
=======================================
  Hits        14398    14398           
  Misses       1084     1084           

@akaszynski
Copy link
Member Author

What does "enforce 0%" mean? What is there to enforce about "at least 0% has to be covered"? I'm probably missing the whole point here, but I can't figure it out on my own.

I've updated the title and description of the PR to explain it better. I agree that it's not clear.

@adeak
Copy link
Member

adeak commented Jan 1, 2022

Ah, I get it now, thanks.

Would it perhaps be possible to distinguish PRs? Maybe based on branch name (ignore doc/*), or labels on github (something like a [no-code] tag that's explicitly exempt from coverage checks)? For bugfixes and new features it would be pretty nice to have some pressure to cover changes.

@akaszynski
Copy link
Member Author

For bugfixes and new features it would be pretty nice to have some pressure to cover changes.

I can enforce patch changes as well. I'm still trying to figure out the yml and it takes a bit of time for the PRs to update, so it might take a bit of time.

@tkoyama010 tkoyama010 added the maintenance Low-impact maintenance activity label Jan 2, 2022
@akaszynski
Copy link
Member Author

I've added untested code in 2ceb39b, and codecov reported that the code wasn't tested (which is great in the web interface):
image

Looks like the checks also work as expected
image

@akaszynski akaszynski merged commit 6f44c89 into main Jan 2, 2022
@akaszynski akaszynski deleted the ci/enforce_codecov branch January 2, 2022 17:51
@akaszynski akaszynski mentioned this pull request Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we have net positive code coverage diff requirement on PRs?
3 participants