-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[Tests] Run fast test suite in clean environment #2336
Conversation
@memark if you have the time could you run |
@reasonablytall This is the only test failing now in your branch:
On a sidenote, I had to enter my password (sudo somewhere?), which I did. I don't remember if I had to do that last time (it's been a while). |
Thanks for the check @memark! The sudo request is a normal part of the test suite :) It looks like that failing test is doing everything correctly, but an added help message exclusive to MacOS causes the output to be different from expected (which I think explains why the test passes on my linux machine, as well as on CI). Previously I figure that this PR can be reviewed as-is, and I can fix |
package.json
Outdated
@@ -7,7 +7,7 @@ | |||
}, | |||
"scripts": { | |||
"test": "shell=$(basename -- $(ps -o comm= $(ps -o ppid= -p $PPID)) | sed 's/^-//'); make test-$shell", | |||
"test/fast": "shell=$(basename -- $(ps -o comm= $(ps -o ppid= -p $PPID)) | sed 's/^-//'); make TEST_SUITE=fast test-$shell", | |||
"test/fast": "shell=$(basename -- $(ps -o comm= $(ps -o ppid= -p $PPID)) | sed 's/^-//'); env -i TERM=\"$TERM\" /bin/bash -lc \"make TEST_SUITE=fast test-$shell\"", |
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.
can you help me understand these changes? env
i get, you're setting up the $TERM
variable. What does -lc
do? I see /bin/bash
, what happens if that's not the path to bash, and what about all the other shells (zsh, sh, dash, ksh)?
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.
I'll break down my thinking:
env -i
runs the following in a fully cleared environmentTERM="$TERM"
brings$TERM
into that environment/bin/bash -lc
runs the make command in a login (-l
) shell which first populates the environment by executing various profile files. I figure that running from a login shell is more ~robust~ than running from a blank environment, though I'm definitely not sure. When I run the same command without-l
the tests all pass but the git checks at the start fail due to a path issue.
Using /bin/bash
was an oversight. Both bash
and sh
work, and I'll make a change to use bash
instead (since sh
doesn't list -l
as an option in the man page) unless you prefer otherwise.
The shell urchin uses to run the tests is taken from the make target test-$shell
, so the shell used to run make
doesn't matter.
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! Ci for this repo is currently broken; once it's fixed, i'll rebase and land this.
Fixes #2181
This makes
npm run test/fast
start the test suite from within a fresh login shell, effectively removing theNPM_CONFIG_...
variables that caused most tests to fail in #2181. I've run this in both zsh and bash, and all fast tests pass.$TERM
is included in the fresh environment to prevent tests that rely on the color output ofnvm_print_formatted_alias
from failing.node_modules/.bin
is still added to the path by the makefile, so I temporarily remove it intest/fast/Running "nvm use iojs" uses latest io.js version
for the same reason as #2322.This change could probably work for the other suites too! I just haven't dug into that yet... let me know what you think! :)