FLUID-6529: Uploading coverage reports to codecov.io

[jobara]

https://issues.fluidproject.org/browse/FLUID-6529

[jobara]

@gtirloni and @cindyli do you think you could look over this PR? It doesn't look like Coveralls is able to report results to the PR because of the GitHub Actions restrictions related to secrets and tokens. I was hoping the coveralls bot would be able to do that but it might just be for private repos. You should be able to view the results from https://coveralls.io/github/fluid-project/infusion

[gtirloni]

Yeah, I think the default token won't work because it gets read-only access only.

Have you looked at codecov.io? It looks like that one would work with webhooks only and not need a GH Actions step but I could be mistaken.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (main@daeff32). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1002   +/-   ##
=======================================
  Coverage        ?   88.12%           
=======================================
  Files           ?       81           
  Lines           ?    10891           
  Branches        ?     2346           
=======================================
  Hits            ?     9598           
  Misses          ?      728           
  Partials        ?      565           

Flag	Coverage Δ
#unit-tests	88.12% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

[jobara]

@cindyli, @amb26, and @gtirloni would you mind to review this PR to add the coverage reporting to the PRs. It's using https://codecov.io right now. A few things to note:

• Although it uses the coverage report from testem/nyc, the coverage reporting is a bit different from what we get in the testem summary. (See: https://docs.codecov.io/docs/frequently-asked-questions#how-is-coverage-calculated )
• Either the partials aren't working or they don't work the way I assume they do. I have enabled them though. (See: https://docs.codecov.io/docs/node)
• In the current configuration the code coverage comment will update after 6 builds have completed. This is the number of build configurations we run (macOS, Ubuntu, Windows; Node 10, 12). The comment will just be edited so you can see the history of the edits which should update after each commit. If the comment isn't being updated we may need to have it updated after each build configuration finishes.
• I've also configured things to fail if the coverage drops below 90% or if it reduces by more than 2%.
• Once we have coverage reports for master, we should be able to see a diff with master.

[amb26]

CCing in @the-t-in-rtf

[the-t-in-rtf]

@amb26, were you just keeping me informed, or did you want help figuring out specific issues here?

[the-t-in-rtf]

I applaud the goal and don't see anything particularly worrying about their approach. I like that you started with a package that has both nyc and testem tests, that's a helpful example for other work.

I guess we can't see some of the value because we haven't already been using it for a while. If you could dig up an example PR in an existing project that demonstrates the full output, that would be good. I know that their docs probably have examples, but it'd be nice to see someone other than them using it in anger and confirm what it should look like down the road.

[jobara]

@the-t-in-rtf here's an example of it being used by their own repo for their GitHub Action codecov/codecov-action#96 and here's another example where there was a coverage change and caused a failure codecov/codecov-node#160

[the-t-in-rtf]

Is there any option to have a shorter (or no) summary if there are no problems? The full details are always available in the checks block anyway.

[jobara]

Is there any option to have a shorter (or no) summary if there are no problems? The full details are always available in the checks block anyway.

@the-t-in-rtf, the coverage summary comment can be removed if there are no changes https://docs.codecov.io/docs/pull-request-comments#requiring-changes The coverage summary comment can also be completely disabled https://docs.codecov.io/docs/pull-request-comments#disable-comment Please let me know your thoughts on these options.

[the-t-in-rtf]

I'm not sure how valuable it is as a comment, and it's huge. If it were just up to me I'd leave it just in the checks. What do other people think?

[greatislander]

@the-t-in-rtf @jobara I'm all for leaving it in the checks rather than having the comment appear.

[greatislander]

Status badge needs to be updated to point to main.

[greatislander]

Suggested change

	[](https://codecov.io/github/fluid-project/infusion?branch=master)
	[](https://codecov.io/github/fluid-project/infusion?branch=main)

[jobara]

@the-t-in-rtf and @greatislander I've made the changes recommended. Ready for more review.

[jobara]

@cindyli would you be able to take a pass over this PR and merge if good?

[cindyli]

This PR sets the minimum coverage ratio to 90%. The coverage report from the check in this PR shows the current coverage is 88.16%, which is below the minimum ratio of 90%, but the check didn't report a failure. Please let me know if I misunderstand something. Thanks.

Instead of setting a hard ratio, I wonder if it makes more sense to use the value auto for the coverage ratio. According to the codecov doc, auto will use the coverage from the base commit (pull request base or parent commit) coverage to compare against.

[greatislander]

@cindyli I think that makes a lot of sense.

[greatislander]

Suggest changing target to auto as per @cindyli.

[greatislander]

Suggested change

	target: 90%
	target: auto

[jobara]

This PR sets the minimum coverage ratio to 90%. The coverage report from the check in this PR shows the current coverage is 88.16%, which is below the minimum ratio of 90%, but the check didn't report a failure. Please let me know if I misunderstand something. Thanks.

Instead of setting a hard ratio, I wonder if it makes more sense to use the value auto for the coverage ratio. According to the codecov doc, auto will use the coverage from the base commit (pull request base or parent commit) coverage to compare against.

This is because there is a 2% threshold set, so the coverage is allowed to drop below the target by that amount before reporting an error. That was sort of their to help us get back up, but we can remove the threshold and as well and switch to auto. Thoughts @cindyli and @greatislander?

[greatislander]

@jobara Ah, that makes sense. I don't think we need to remove the threshold, then.

[cindyli]

This is because there is a 2% threshold set, so the coverage is allowed to drop below the target by that amount before reporting an error. That was sort of their to help us get back up, but we can remove the threshold and as well and switch to auto. Thoughts @cindyli and @greatislander?

Makes sense. This explains why the check didn't report a failure.

I agree to keep the 2% threshold but leaning towards to use target: auto. If we keep target: 90% and the next PR drops the coverage anywhere more than 0.16%, a failure will occur. In other words, the implied actual threshold is 0.16% rather than 2%.

[jobara]

This is because there is a 2% threshold set, so the coverage is allowed to drop below the target by that amount before reporting an error. That was sort of their to help us get back up, but we can remove the threshold and as well and switch to auto. Thoughts @cindyli and @greatislander?

Makes sense. This explains why the check didn't report a failure.

I agree to keep the 2% threshold but leaning towards to use target: auto. If we keep target: 90% and the next PR drops the coverage anywhere more than 0.16%, a failure will occur. In other words, the implied actual threshold is 0.16% rather than 2%.

The problem though is that each subsequent commit could continuously lower the baseline.. so the next one reduce coverage to 88%, followed by 87%, 86% and etc. without ever reporting an error.

[cindyli]

This is because there is a 2% threshold set, so the coverage is allowed to drop below the target by that amount before reporting an error. That was sort of their to help us get back up, but we can remove the threshold and as well and switch to auto. Thoughts @cindyli and @greatislander?

Makes sense. This explains why the check didn't report a failure.
I agree to keep the 2% threshold but leaning towards to use target: auto. If we keep target: 90% and the next PR drops the coverage anywhere more than 0.16%, a failure will occur. In other words, the implied actual threshold is 0.16% rather than 2%.

The problem though is that each subsequent commit could continuously lower the baseline.. so the next one reduce coverage to 88%, followed by 87%, 86% and etc. without ever reporting an error.

Right. Depending on how much dropping tolerance will be picked for the coverage, I'd prefer it's explicitly expressed without a hidden meaning. For example, if it's zero tolerance, as you said, values could be target: auto; threshold: 0%. Probably we could give a try to the zero tolerance to find out how easy or hard to never drop the coverage. It feels to me won't be easy sometimes. ;)

[jobara]

@cindyli ready for more review. I've changed to target: auto and threshold: 0%