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

test: ensure test-npm-install uses correct node #6658

Merged
merged 1 commit into from
May 11, 2016

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented May 9, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Currently it is possible that the shelled out instance of npm will use
the system copy of node. This PR changes the test to shim the build
directory into the path. This will ensure that npm will use the correct
version of node.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 9, 2016
@MylesBorins
Copy link
Contributor Author

MylesBorins commented May 9, 2016

ci: https://ci.nodejs.org/job/node-test-pull-request/2550/
/cc @nodejs/testing
edit: CI is green

@mscdex mscdex added the npm Issues and PRs related to the npm client dependency or the npm registry. label May 9, 2016
@MylesBorins
Copy link
Contributor Author

fixes: #6648

@@ -29,11 +29,13 @@ const pkgContent = JSON.stringify({
});

const pkgPath = path.join(common.tmpDir, 'package.json');

Copy link
Contributor

Choose a reason for hiding this comment

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

nit?

@addaleax
Copy link
Member

Related to #6220 in any way?

@MylesBorins
Copy link
Contributor Author

@addaleax yes and no.

It is the same problem with npm v2 not respecting the node that called it. Not the same problem in that #6220 needs to be fixed via documentation afaik

@addaleax
Copy link
Member

Okay, LGTM then.

@MylesBorins
Copy link
Contributor Author

cwd: common.tmpDir
cwd: common.tmpDir,
env: {
path: path.dirname(process.execPath)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that spell PATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed... good eye

@MylesBorins
Copy link
Contributor Author

nits addressed @bnoordhuis @Fishrock123

@Fishrock123
Copy link
Contributor

lgtm if it works

@MylesBorins
Copy link
Contributor Author

MylesBorins commented May 10, 2016

ci mater: https://ci.nodejs.org/job/node-test-pull-request/2562/
ci v4.x: https://ci.nodejs.org/job/node-test-commit/3256/

I'm running CI against both master + v4.x as this fix should only really matter with npm v2

@bnoordhuis
Copy link
Member

LGTM

Currently it is possible that the shelled out instance of npm will use
the system copy of node. This PR changes the test to shim the build
directory into the path. This will ensure that npm will use the correct
version of node.

fixes: nodejs#6648

PR-URL: nodejs#6658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins merged commit 738a1d6 into nodejs:master May 11, 2016
MylesBorins pushed a commit that referenced this pull request May 11, 2016
Currently it is possible that the shelled out instance of npm will use
the system copy of node. This PR changes the test to shim the build
directory into the path. This will ensure that npm will use the correct
version of node.

fixes: #6648

PR-URL: #6658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins deleted the npm-test-explicit-path branch May 12, 2016 18:14
evanlucas pushed a commit that referenced this pull request May 17, 2016
Currently it is possible that the shelled out instance of npm will use
the system copy of node. This PR changes the test to shim the build
directory into the path. This will ensure that npm will use the correct
version of node.

fixes: #6648

PR-URL: #6658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
Currently it is possible that the shelled out instance of npm will use
the system copy of node. This PR changes the test to shim the build
directory into the path. This will ensure that npm will use the correct
version of node.

fixes: #6648

PR-URL: #6658
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Issues and PRs related to the npm client dependency or the npm registry. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants