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

Fix Node.js' code coverage/code coverage job #35646

Closed
bcoe opened this issue Oct 14, 2020 · 5 comments · Fixed by #35653
Closed

Fix Node.js' code coverage/code coverage job #35646

bcoe opened this issue Oct 14, 2020 · 5 comments · Fixed by #35653
Assignees
Labels
coverage Issues and PRs related to native coverage support. test Issues and PRs related to the tests.

Comments

@bcoe
Copy link
Contributor

bcoe commented Oct 14, 2020

  • Version: 15.x.x
  • Platform: All
  • Subsystem: test

The problem(s)

1. new file paths are breaking the coverage reporting

File paths for Node.js' own files now have this form node:/internal/cluster/shared_handle:

  • there's now anode: prefix.
  • there's no longer a .js prefix.

I will happily fix this problem in the test reporter, but could someone point me towards where these changes were made (@watilde, @guybedford?) -- I want to make sure I link to the change in the reporter codebase.

2. nightly coverage is broken again

Coverage has been broken for several nights in a row. I believe this is due to an unrelated CI/CD upgrade?

Suggested fix

  1. let's update c8 to support the new path format (I've tested that this fix is pretty trivial).
  2. let's move code coverage to GitHub Actions (It seems like we need to fix Jenkins once every few months, and I can't see a reason why not to use actions).

CC: @nodejs/testing

@aduh95
Copy link
Contributor

aduh95 commented Oct 14, 2020

could someone point me towards where these changes were made

I believe this has been introduced by #35498.

@bcoe
Copy link
Contributor Author

bcoe commented Oct 14, 2020

@aduh95 thank you! (sorry for looping in the wrong folks, I assumed it was potentially the new URL parser).

@bcoe bcoe changed the title Fixe Node.js' code coverage/code coverage job Fix Node.js' code coverage/code coverage job Oct 14, 2020
@joyeecheung
Copy link
Member

Have we ever tried adding c8 to https://github.com/nodejs/citgm/ ? (I searched for it and I don't think so?)

@MylesBorins
Copy link
Member

we should add it, it isn't included right now afaik. For reference this is the lookup table

https://github.com/nodejs/citgm/blob/main/lib/lookup.json

@bcoe
Copy link
Contributor Author

bcoe commented Oct 14, 2020

Have we ever tried adding c8 to https://github.com/nodejs/citgm/ ? (I searched for it and I don't think so?)

@joyeecheung @MylesBorins not a bad idea, but it wouldn't have actually caught this, because it only breaks on Node.js' own coverage -- which now has different file paths (c8 would continue working for userland.)

@watilde watilde added coverage Issues and PRs related to native coverage support. test Issues and PRs related to the tests. labels Oct 15, 2020
bcoe added a commit that referenced this issue Oct 22, 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>
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
coverage Issues and PRs related to native coverage support. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants