Conversation
e8a26e7 added `process` to the internal module wrapper. This broke the utility used to write coverage information due to a SyntaxError that `process` had already been declared.
|
Any objections to me going ahead and landing? |
maclover7
left a comment
There was a problem hiding this comment.
LGTM
Quick question -- how was the initial problem with generating code coverage found out? Should an issue be opened at nodejs/build about this (not sure if coverage.nodejs.org falls under build wg or not...)?
|
Hmm, I see that the coverage reports on coverage.nodejs.org end on the 27th of Nov. The coverage job https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage/ also shows as read. I'm pretty sure it used to be on the main page for Node.js in CI which I think would have made sure it was noticed earlier.... I'm going to add it back there and can remove if anybody objects. |
|
The other thing we can do is add people to to an email alias for notifications when the job fails. I've had trouble finding volunteers in the past through other jobs. |
|
Landed in 9f61c70 |
e8a26e7 added `process` to the internal module wrapper. This broke the utility used to write coverage information due to a SyntaxError that `process` had already been declared. PR-URL: #17651 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
@mhdawson Would you be able to open up an issue on nodejs/build about this explaining the email alias |
|
@addaleax thanks for landing! :] @mhdawson feel free to add me to the email list. I use the coverage pretty often and would be happy to make sure it stays working. @maclover7 i found it by just trying to run make coverage. The error appeared there when trying to run the tests. |
e8a26e7 added `process` to the internal module wrapper. This broke the utility used to write coverage information due to a SyntaxError that `process` had already been declared. PR-URL: #17651 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
Setting as don't land for LTS as it appears the commit that this is fixing landed in a semver major for 9.x |
e8a26e7 added
processto theinternal module wrapper. This broke the utility used to write
coverage information due to a SyntaxError that
processhadalready been declared.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
process