FLUID-6529: Uploading coverage reports to codecov.io #1002

jobara · 2020-07-16T14:07:43Z

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

jobara · 2020-07-17T19:58:37Z

@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 · 2020-07-20T18:43:12Z

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 · 2020-07-21T12:50:58Z

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 · 2020-07-21T17:16:08Z

@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 · 2020-07-21T19:45:06Z

CCing in @the-t-in-rtf

the-t-in-rtf · 2020-07-24T08:17:47Z

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

the-t-in-rtf · 2020-07-29T13:22:40Z

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 · 2020-07-29T14:39:56Z

@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 · 2020-07-30T12:50:14Z

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 · 2020-07-30T13:13:01Z

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 · 2020-07-30T14:23:55Z

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 · 2020-10-27T14:08:43Z

@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 · 2020-10-27T14:10:01Z

README.md

@@ -1,5 +1,8 @@
 # Infusion

+![CI build status badge](https://github.com/fluid-project/infusion/workflows/CI/badge.svg)
+[![Coverage status badge](https://codecov.io/github/fluid-project/infusion/coverage.svg?branch=master)](https://codecov.io/github/fluid-project/infusion?branch=master)


Suggested change

[![Coverage status badge](https://codecov.io/github/fluid-project/infusion/coverage.svg?branch=master)](https://codecov.io/github/fluid-project/infusion?branch=master)

[![Coverage status badge](https://codecov.io/github/fluid-project/infusion/coverage.svg?branch=main)](https://codecov.io/github/fluid-project/infusion?branch=main)

The PR comment is quite large and may clutter PR UI. Instead just rely on the GitHub checks for reporting coverage.

jobara · 2020-10-27T14:42:13Z

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

jobara · 2020-10-27T15:00:30Z

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

cindyli · 2020-10-27T16:17:59Z

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 · 2020-10-27T16:24:46Z

@cindyli I think that makes a lot of sense.

greatislander

Suggest changing target to auto as per @cindyli.

greatislander · 2020-10-28T13:22:58Z

.codecov.yml

+  status:
+    project:
+      default:
+        target: 90%


Suggested change

target: 90%

target: auto

jobara · 2020-10-28T13:47:54Z

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 · 2020-10-28T13:48:57Z

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

cindyli · 2020-10-28T14:01:54Z

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 · 2020-10-28T14:05:22Z

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 · 2020-10-28T14:16:23Z

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 · 2020-10-29T17:58:57Z

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

jobara force-pushed the FLUID-6529 branch 4 times, most recently from ccfc3f2 to e6264b4 Compare July 17, 2020 01:10

jobara marked this pull request as ready for review July 17, 2020 19:55

jobara force-pushed the FLUID-6529 branch from 2c2c2d7 to e3bf8a4 Compare July 21, 2020 13:37

jobara changed the title ~~FLUID-6529: Uploading coverage reports to coveralls~~ FLUID-6529: Uploading coverage reports to codecov.io Jul 21, 2020

gtirloni self-requested a review July 21, 2020 18:10

gtirloni approved these changes Jul 21, 2020

View reviewed changes

jobara added 10 commits August 4, 2020 08:37

FLUID-6529: Uploading coverage reports to coveralls

f3f642d

FLUID-6529: Add CI badges

73b3d2f

FLUID-6529: Uploading coverage reports to Codecov

3ad170d

FLUID-6529: Add CI badge for Codecov reports

8a90944

FLUID-6529: configuring codecov

b672a7d

FLUID-6529: Updating npmignore

e36dfdf

FLUID-6529: Include javascript partial coverage in reports

a98035d

NOJIRA: remove unused file

8026baa

FLUID-6529: correcting invalid codecov config.

80a667e

FLUID-6529: remove coveralls code coverage reports

e0655ce

FLUID-6529: Switching to json output of coverage data.

38609af

jobara force-pushed the FLUID-6529 branch from fdf2980 to 38609af Compare August 4, 2020 12:37

jobara changed the base branch from master to main October 22, 2020 14:37

greatislander requested changes Oct 27, 2020

View reviewed changes

jobara added 3 commits October 27, 2020 10:35

Merge branch 'main' into FLUID-6529

71a0340

FLUID-6529: Update due to renaming default branch to main

5d7759c

FLUID-6529: Remove PR comment

97f7e82

The PR comment is quite large and may clutter PR UI. Instead just rely on the GitHub checks for reporting coverage.

greatislander approved these changes Oct 27, 2020

View reviewed changes

greatislander requested changes Oct 28, 2020

View reviewed changes

.codecov.yml Outdated

status:

project:

default:

target: 90%

Copy link

Collaborator

greatislander Oct 28, 2020

Choose a reason for hiding this comment

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

Suggested change

target: 90%

target: auto

greatislander approved these changes Oct 28, 2020

View reviewed changes

jobara added 2 commits October 29, 2020 13:33

Merge branch 'main' into FLUID-6529

a9e266b

FLUID-6529: Report on dropped code coverage

fb839b3

greatislander approved these changes Oct 29, 2020

View reviewed changes

cindyli approved these changes Oct 29, 2020

View reviewed changes

cindyli merged commit 5734c12 into fluid-project:main Oct 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FLUID-6529: Uploading coverage reports to codecov.io #1002

FLUID-6529: Uploading coverage reports to codecov.io #1002

jobara commented Jul 16, 2020

jobara commented Jul 17, 2020

gtirloni commented Jul 20, 2020

codecov bot commented Jul 21, 2020 •

edited

jobara commented Jul 21, 2020

amb26 commented Jul 21, 2020

the-t-in-rtf commented Jul 24, 2020

the-t-in-rtf commented Jul 29, 2020

jobara commented Jul 29, 2020 •

edited

the-t-in-rtf commented Jul 30, 2020

jobara commented Jul 30, 2020

the-t-in-rtf commented Jul 30, 2020

greatislander commented Oct 27, 2020

greatislander left a comment

greatislander Oct 27, 2020

jobara commented Oct 27, 2020

jobara commented Oct 27, 2020

cindyli commented Oct 27, 2020

greatislander commented Oct 27, 2020

greatislander left a comment

greatislander Oct 28, 2020

jobara commented Oct 28, 2020

greatislander commented Oct 28, 2020

cindyli commented Oct 28, 2020

jobara commented Oct 28, 2020 •

edited

cindyli commented Oct 28, 2020

jobara commented Oct 29, 2020

	[![Coverage status badge](https://codecov.io/github/fluid-project/infusion/coverage.svg?branch=master)](https://codecov.io/github/fluid-project/infusion?branch=master)
	[![Coverage status badge](https://codecov.io/github/fluid-project/infusion/coverage.svg?branch=main)](https://codecov.io/github/fluid-project/infusion?branch=main)

FLUID-6529: Uploading coverage reports to codecov.io #1002

FLUID-6529: Uploading coverage reports to codecov.io #1002

Conversation

jobara commented Jul 16, 2020

jobara commented Jul 17, 2020

gtirloni commented Jul 20, 2020

codecov bot commented Jul 21, 2020 • edited

Codecov Report

jobara commented Jul 21, 2020

amb26 commented Jul 21, 2020

the-t-in-rtf commented Jul 24, 2020

the-t-in-rtf commented Jul 29, 2020

jobara commented Jul 29, 2020 • edited

the-t-in-rtf commented Jul 30, 2020

jobara commented Jul 30, 2020

the-t-in-rtf commented Jul 30, 2020

greatislander commented Oct 27, 2020

greatislander left a comment

Choose a reason for hiding this comment

greatislander Oct 27, 2020

Choose a reason for hiding this comment

jobara commented Oct 27, 2020

jobara commented Oct 27, 2020

cindyli commented Oct 27, 2020

greatislander commented Oct 27, 2020

greatislander left a comment

Choose a reason for hiding this comment

greatislander Oct 28, 2020

Choose a reason for hiding this comment

jobara commented Oct 28, 2020

greatislander commented Oct 28, 2020

cindyli commented Oct 28, 2020

jobara commented Oct 28, 2020 • edited

cindyli commented Oct 28, 2020

jobara commented Oct 29, 2020

codecov bot commented Jul 21, 2020 •

edited

jobara commented Jul 29, 2020 •

edited

jobara commented Oct 28, 2020 •

edited