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

Try rounding codecov down instead of up #1413

Merged
merged 6 commits into from Nov 6, 2020
Merged

Try rounding codecov down instead of up #1413

merged 6 commits into from Nov 6, 2020

Conversation

dsherry
Copy link
Contributor

@dsherry dsherry commented Nov 6, 2020

Perhaps I had this backwards previously? I hope this helps with the recent codecov flakes we've been seeing occur when people delete lines of code.

https://docs.codecov.io/docs/codecovyml-reference#coverageround

@dsherry dsherry added the documentation Improvements or additions to documentation label Nov 6, 2020
@dsherry dsherry marked this pull request as ready for review November 6, 2020 14:54
@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #1413 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1413   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         213      213           
  Lines       13936    13936           
=======================================
  Hits        13929    13929           
  Misses          7        7           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c8de52...61e43a4. Read the comment docs.

@@ -1,3 +1,3 @@
coverage:
round: up
round: down
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need to change the precision? I think rounding down is the default behavior and we were seeing issues like what is happening in #1388 before we changed to rounding up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@freddyaboulton ah you are correct, this is the default behavior...

Copy link
Contributor

Choose a reason for hiding this comment

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

And the same thing is happening in #1374 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The codecov.io docs are super unclear about what "round" means:

Which direction to you want to round the coverage value

They should have included an example... what's their algorithm for determining whether or not a job passes codecov? I think the algorithm is

  • Compute coverage percentage before and after
  • Take the signed difference, after - before
  • Round that difference using the round policy, then truncate using precision digits after the decimal
  • If the resulting number is less than 0, fail

But... I haven't found their docs which say whether or not this is true, hence further trial and error

Copy link
Contributor

Choose a reason for hiding this comment

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

And in #1388 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops I spoke too soon:

round((hits / (hits + partials + misses)) * 100, 5) = Coverage Ratio

According to this, rounding up means a higher coverage ratio, which I think is what we want. So I'll decrease the precision but leave this set to "up". Does that make sense?

round: up
precision: 2
round: down
precision: 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@freddyaboulton ok let's try this :)

… we rely on linux unit tests to report coverage

codecov:
notify:
wait_for_ci: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will theoretically only run a codecov report once CI is green. We'll see :)

Copy link
Contributor

Choose a reason for hiding this comment

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

No more premature +/ 8% changes in coverage? 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@freddyaboulton I sure hope not 😆

@jeremyliweishih
Copy link
Contributor

jeremyliweishih commented Nov 6, 2020

@dsherry I think we can also try to widen the threshold and see how that goes.

threshold: Allow the coverage to drop by X%, and posting a success status

@freddyaboulton
Copy link
Contributor

@jeremyliweishih I think that is also worth looking into! I don't think rounding can save us if we ever decide to delete a whole file or something like that.

status:
project:
default:
threshold: 1%
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeremyliweishih good point! Let's try it out. I haven't been able to find docs for threshold other than the link you found.

I saw someone on their forum used this and didn't complain about it breaking, beyond some sort of caching bug which is hopefully now irrelevant.

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

🤞

@dsherry dsherry added testing Issues related to testing. and removed documentation Improvements or additions to documentation labels Nov 6, 2020
@dsherry
Copy link
Contributor Author

dsherry commented Nov 6, 2020

I just tested this out in a PR #1414 , deleted a unit test to trigger a <0.01% coverage drop, and the codecov job was marked as passing!

We may wanna decrease the threshold to 0.1% or something, but this is fine for now. Merging soon!

@dsherry dsherry merged commit 03bf466 into main Nov 6, 2020
@dsherry dsherry mentioned this pull request Nov 24, 2020
@freddyaboulton freddyaboulton deleted the ds_debug_codecov_io branch May 13, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Issues related to testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants