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

test: turn off coverage comments #35800

Merged
merged 1 commit into from Oct 26, 2020
Merged

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Oct 25, 2020

Turns off coverage comments for the time being, until we can sort out #35759

Refs #35759, #35779, #35753

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@bcoe bcoe requested review from Trott, watilde and Qard October 25, 2020 15:53
@bcoe
Copy link
Contributor Author

bcoe commented Oct 25, 2020

@thomasrockhu, we're still getting value out of the reports 👏 (for instance, see #35797 (comment)), but quite a few folks have been confused by reported drops in coverage, not related to their PRs.

An idea

Rather than providing information about drops in coverage as a comment, it would be useful to simply link to the coverage report for a sha in a comment, this would allow folks to see whether new tests they've added have actually increased coverage.

@nodejs-github-bot

This comment has been minimized.

@bcoe bcoe added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Oct 25, 2020
@bcoe
Copy link
Contributor Author

bcoe commented Oct 25, 2020

@Qard @watilde @Trott any objection to fast tracking this, I'd like to stop confusing folks until we sort out why the coverage reports jump around a bit.

👍 to fast track.

@nodejs-github-bot
Copy link
Collaborator

Turns off coverage comments for the time being, until we can sort out
issues.

PR-URL: nodejs#35800
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 26, 2020

Landed in 4ace92f

@Trott Trott merged commit 4ace92f into nodejs:master Oct 26, 2020
targos pushed a commit that referenced this pull request Nov 3, 2020
Turns off coverage comments for the time being, until we can sort out
issues.

PR-URL: #35800
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Nov 3, 2020
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
Turns off coverage comments for the time being, until we can sort out
issues.

PR-URL: #35800
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Turns off coverage comments for the time being, until we can sort out
issues.

PR-URL: #35800
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
@targos targos added dont-land-on-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants