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

coverage: fix broken coverage setup #25289

Merged
merged 3 commits into from
Jan 2, 2019
Merged

coverage: fix broken coverage setup #25289

merged 3 commits into from
Jan 2, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 31, 2018

This PR replaces console.warn() and console.error() in coverage setup with process._rawDebug(), as console hasn't been properly bootstrapped at this point. This PR also prevents a problematic call to process.cwd(), which also hasn't been bootstrapped yet.

Fixes: #25287

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

@nodejs-github-bot
Copy link
Collaborator

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2114/pipeline

@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Dec 31, 2018
@addaleax addaleax added coverage Issues and PRs related to native coverage support. and removed process Issues and PRs related to the process subsystem. labels Dec 31, 2018
@cjihrig cjihrig mentioned this pull request Dec 31, 2018
Closed
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 1, 2019
@addaleax
Copy link
Member

addaleax commented Jan 1, 2019

Green CI @ https://ci.nodejs.org/job/node-test-pull-request/19900/

@cjihrig
coverage: use process._rawDebug() during setup
d32f769 
console is not ready to use at this point in the bootstrapping
process, so switch to process._rawDebug() instead.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig
coverage: pass cwd to path.resolve() in setup
a779ee4 
During coverage setup, path.resolve() is called.
path.resolve() can potentially call process.cwd(), which
hasn't been bootstrapped yet. This commit passes the
current working directory directly so that path.resolve()
doesn't attempt to compute it.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig
test: make test-v8-coverage.js more strict
8881a5a 
Update the coverage test to verify that nothing is printed to
stderr (which happens when coverage errors happen). Also add a
test case to verify that non-absolute coverage paths work.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig merged commit 8881a5a into nodejs:master Jan 2, 2019
@cjihrig cjihrig deleted the 25287 branch January 2, 2019 00:56
@addaleax
Copy link
Member

addaleax commented Jan 5, 2019

I’m adding the backport-requested-v11.x label, but I think this needs to be backported together with #25127 anyway.

@joyeecheung joyeecheung mentioned this pull request Jan 6, 2019
Closed
2 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@cjihrig @refack
coverage: use process._rawDebug() during setup
e812e0d 
console is not ready to use at this point in the bootstrapping
process, so switch to process._rawDebug() instead.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@cjihrig @refack
coverage: pass cwd to path.resolve() in setup
4cfbcb0 
During coverage setup, path.resolve() is called.
path.resolve() can potentially call process.cwd(), which
hasn't been bootstrapped yet. This commit passes the
current working directory directly so that path.resolve()
doesn't attempt to compute it.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@cjihrig @refack
test: make test-v8-coverage.js more strict
27b5dbb 
Update the coverage test to verify that nothing is printed to
stderr (which happens when coverage errors happen). Also add a
test case to verify that non-absolute coverage paths work.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Jan 14, 2019
Closed
@addaleax
Copy link
Member

@cjihrig When landing multiple commits, could you still add a Landed in ... comment when using the merge variant? Thank you :)

addaleax pushed a commit to addaleax/node that referenced this pull request Jan 14, 2019
@cjihrig @addaleax
coverage: use process._rawDebug() during setup
172466c 
console is not ready to use at this point in the bootstrapping
process, so switch to process._rawDebug() instead.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Jan 14, 2019
@cjihrig @addaleax
coverage: pass cwd to path.resolve() in setup
dcd7767 
During coverage setup, path.resolve() is called.
path.resolve() can potentially call process.cwd(), which
hasn't been bootstrapped yet. This commit passes the
current working directory directly so that path.resolve()
doesn't attempt to compute it.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Jan 14, 2019
@cjihrig @addaleax
test: make test-v8-coverage.js more strict
066985a 
Update the coverage test to verify that nothing is printed to
stderr (which happens when coverage errors happen). Also add a
test case to verify that non-absolute coverage paths work.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@cjihrig @addaleax
coverage: use process._rawDebug() during setup
daca318 
console is not ready to use at this point in the bootstrapping
process, so switch to process._rawDebug() instead.

PR-URL: #25289
Fixes: #25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #25496
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@cjihrig @addaleax
coverage: pass cwd to path.resolve() in setup
63fbd00 
During coverage setup, path.resolve() is called.
path.resolve() can potentially call process.cwd(), which
hasn't been bootstrapped yet. This commit passes the
current working directory directly so that path.resolve()
doesn't attempt to compute it.

PR-URL: #25289
Fixes: #25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #25496
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@cjihrig @addaleax
test: make test-v8-coverage.js more strict
067d38f 
Update the coverage test to verify that nothing is printed to
stderr (which happens when coverage errors happen). Also add a
test case to verify that non-absolute coverage paths work.

PR-URL: #25289
Fixes: #25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #25496
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@cjihrig @BridgeAR
coverage: use process._rawDebug() during setup
d7534bf 
console is not ready to use at this point in the bootstrapping
process, so switch to process._rawDebug() instead.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#25496
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@cjihrig @BridgeAR
coverage: pass cwd to path.resolve() in setup
0213d0a 
During coverage setup, path.resolve() is called.
path.resolve() can potentially call process.cwd(), which
hasn't been bootstrapped yet. This commit passes the
current working directory directly so that path.resolve()
doesn't attempt to compute it.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#25496
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@cjihrig @BridgeAR
test: make test-v8-coverage.js more strict
c648bb6 
Update the coverage test to verify that nothing is printed to
stderr (which happens when coverage errors happen). Also add a
test case to verify that non-absolute coverage paths work.

PR-URL: nodejs#25289
Fixes: nodejs#25287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#25496
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@hiroppy hiroppy hiroppy approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. coverage Issues and PRs related to native coverage support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cjihrig @nodejs-github-bot @addaleax @jasnell @lpinca @hiroppy @joyeecheung @devsnek