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.

Sign up for GitHub

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

Jump to bottom

process: fix coverage generation #17651

Closed
wants to merge 1 commit into from
Closed

process: fix coverage generation #17651

wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

process

@evanlucas
process: fix coverage generation
45159ee 
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.
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Dec 13, 2017
@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 13, 2017
@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/14805/

@evanlucas
Copy link
Contributor Author

Any objections to me going ahead and landing?

maclover7
maclover7 approved these changes Dec 13, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
mhdawson
mhdawson approved these changes Dec 13, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

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.

@mhdawson
Copy link
Member

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.

@addaleax
Copy link
Member

Landed in 9f61c70

@addaleax addaleax closed this Dec 13, 2017
addaleax pushed a commit that referenced this pull request Dec 13, 2017
@evanlucas @addaleax
process: fix coverage generation
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@maclover7
Copy link
Contributor

@mhdawson Would you be able to open up an issue on nodejs/build about this explaining the email alias

@evanlucas
Copy link
Contributor Author

@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.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@evanlucas @MylesBorins
process: fix coverage generation
bf0a7b6 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@MylesBorins
Copy link
Contributor

Setting as don't land for LTS as it appears the commit that this is fixing landed in a semver major for 9.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@evanlucas @addaleax @mhdawson @maclover7 @MylesBorins @jasnell @TimothyGu @cjihrig @BridgeAR @nodejs-github-bot