-
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
test: use npm sandbox in test-npm-install #9079
Conversation
npm should run in a sandbox to avoid unwanted interactions. Without this change, npm would read the userconfig file $HOME/.npmrc which may contain configs that break this test. Fixes: nodejs#9074
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.
seems fine to me
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
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
LGTM |
1 similar comment
LGTM |
LGTM if CI is clean |
Landed in 1847670 |
npm should run in a sandbox to avoid unwanted interactions. Without this change, npm would read the userconfig file $HOME/.npmrc which may contain configs that break this test. Fixes: #9074 PR-URL: #9079 Reviewed-By: Jeremiah Senkpiel <Fishrock123@rocketmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
npm should run in a sandbox to avoid unwanted interactions. Without this change, npm would read the userconfig file $HOME/.npmrc which may contain configs that break this test. Fixes: #9074 PR-URL: #9079 Reviewed-By: Jeremiah Senkpiel <Fishrock123@rocketmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
npm should run in a sandbox to avoid unwanted interactions. Without this change, npm would read the userconfig file $HOME/.npmrc which may contain configs that break this test. Fixes: #9074 PR-URL: #9079 Reviewed-By: Jeremiah Senkpiel <Fishrock123@rocketmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
npm should run in a sandbox to avoid unwanted interactions. Without this change, npm would read the userconfig file
$HOME/.npmrc
which may contain configs that break this test.Not completely sure if we should set the
HOME
variable as I decided to do here, or track down all the variables that it influences withnpm config ls -l
and set them all individually. This seems more future-proof, and since it's just to run npm it should be ok.Taken inspiration from https://github.com/gibfahn/node/blob/a3c1505cd9fd5e26aebdbb2269515f9d3deb11db/tools/test-npm.sh#L44-L48 (ongoing PR #7867) with the
HOME
var change that may be a good idea there as well, cc @gibfahn .Fixes: #9074 - test run: https://ci.nodejs.org/job/node-test-commit-freebsd/4799/
cc @nodejs/testing @thealphanerd @Trott