-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
build: fix coverage after gcovr update #18958
Conversation
Landed nodejs/build#1145 and validated that coverage was successfully generated with that update and this PR. Regular Node.js CI run: https://ci.nodejs.org/job/node-test-pull-request/13403/ |
CI run for coverage job using repo that includes this PR: https://ci.nodejs.org/job/node-test-commit-linux-coverage/549/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -176,8 +176,8 @@ coverage-build: all | |||
--single-branch git://github.com/gcovr/gcovr.git; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We possibly can put a commit hash of the current latest gcovr
at here for this change, then we can keep it as is a bit more easily.
One more run of the coverage job, hopefully with the right parameters to include the fix this time: https://ci.nodejs.org/job/node-test-commit-linux-coverage/551/ @watilde that's a good point, I'll look at doing that in a follow-on PR. |
@mhdawson But if we lock the commit and miss some gcovr version, upgrade will be harder. |
In my local env, print the
@mhdawson Please login the benchmark machine, check what’s going wrong. |
Ok both builds good now will land |
PR-URL: #18958 Fixes: #18938 Ref: nodejs/build#1145 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
PR-URL: nodejs#18958 Fixes: nodejs#18938 Ref: nodejs/build#1145 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
PR-URL: nodejs#18958 Fixes: nodejs#18938 Ref: nodejs/build#1145 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
does this need to be backported to 8.x? |
The patch will fix the broken coverage report.
It can be merged after nodejs/build#1145 accept
Fixes: #18938
Refs: nodejs/build#1145
Checklist
Affected core subsystem(s)