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

Coverage report left in comments is "noisy" #35696

Closed
bcoe opened this issue Oct 18, 2020 · 7 comments
Closed

Coverage report left in comments is "noisy" #35696

bcoe opened this issue Oct 18, 2020 · 7 comments
Labels
test Issues and PRs related to the tests.

Comments

@bcoe
Copy link
Contributor

bcoe commented Oct 18, 2020

Problem

I noticed on a couple PRs, that the coverage comment left by codecov.io was hidden.

I'm guessing that part of why folks wanted to hide this notification, is that it takes up a lot of screen space on the PR 👇

Screen Shot 2020-10-17 at 9 12 24 PM

Proposed Solution

I have a PR open here that:

  • enables coverage for a couple more platforms.
  • uses codecov's layout setting to reduce the screen real-estate used by a coverage report.
  • only posts the report if coverage has changed.

What if folks still aren't happy with the report?

If folks are still finding they find coverage a little bit noisy (and are inclined to hide the comment), we can turn off the comments in configuration.

@nodejs/testing

@bcoe bcoe added the test Issues and PRs related to the tests. label Oct 18, 2020
@aduh95
Copy link
Contributor

aduh95 commented Oct 18, 2020

If folks are still finding they find coverage a little bit noisy (and are inclined to hide the comment), we can turn off the comments in configuration.

Could we change the comment content to put it in a <detail> tag?

@bcoe
Copy link
Contributor Author

bcoe commented Oct 18, 2020

Could we change the comment content to put it in a tag?

I didn't see a way to do this in the configuration. @thomasrockhu is there a way to customize the coverage report, so that some of the information is hidden under a <detail> rollup?

@gengjiawen
Copy link
Member

Maybe also skip draft PR. Pretty annoying in such case.

@richardlau
Copy link
Member

We could also skip coverage for doc-only changes like we do with the asan workflow:

paths-ignore:
- 'doc/**'

@thomasrockhu
Copy link

@bcoe, I don't believe that's possible, but that's really good feedback. I'll bring it to the product team to see what we can do here, but I'm not sure what the turnaround time would be. What's the ideal result for you?

@gengjiawen, also great feedback. Thanks for bringing this up!

@aduh95
Copy link
Contributor

aduh95 commented Oct 20, 2020

@thomasrockhu Ideally, something like that:

Merging #35670 into master will decrease coverage by 7.59%.
@@            Coverage Diff             @@
##           master   #35670      +/-   ##
==========================================
- Coverage   96.40%   88.80%   -7.60%     
==========================================
  Files         220      474     +254     
  Lines       73681   112964   +39283     
  Branches        0     9079    +9079     
==========================================
+ Hits        71031   100319   +29288     
- Misses       2650     7033    +4383     
- Partials        0     5612    +5612     
Impacted Files Coverage Δ
src/node_i18n.cc 76.00% <0.00%> (ø)
src/inspector/worker_inspector.cc 95.52% <0.00%> (ø)
src/node.h 93.33% <0.00%> (ø)
src/inspector_agent.h 87.50% <0.00%> (ø)
src/inspector/node_string.cc 54.28% <0.00%> (ø)
src/api/hooks.cc 80.18% <0.00%> (ø)
src/env.cc 87.19% <0.00%> (ø)
src/node_contextify.h 70.83% <0.00%> (ø)
src/inspector/worker_agent.cc 89.61% <0.00%> (ø)
src/node_os.cc 75.63% <0.00%> (ø)
... and 264 more

The most relevant info is still available at a glance, and it doesn't take forever to scroll by.

@thomasrockhu
Copy link

@aduh95, awesome, we'll look into making this configurable in the yaml. Thanks again for the feedback!

@bcoe bcoe closed this as completed in 7657f62 Oct 22, 2020
targos pushed a commit that referenced this issue Nov 3, 2020
Collect Windows and C++ coverage. Configure codecov so that
comments are more concise and are only left when coverage
varies.

PR-URL: #35670
Fixes: #35696
Refs: #35653
Refs: #35646
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
BethGriggs pushed a commit that referenced this issue Dec 8, 2020
Collect Windows and C++ coverage. Configure codecov so that
comments are more concise and are only left when coverage
varies.

PR-URL: #35670
Fixes: #35696
Refs: #35653
Refs: #35646
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
BethGriggs pushed a commit that referenced this issue Dec 10, 2020
Collect Windows and C++ coverage. Configure codecov so that
comments are more concise and are only left when coverage
varies.

PR-URL: #35670
Fixes: #35696
Refs: #35653
Refs: #35646
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants