-
-
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
Fix nvm_ls_current
fast unit test
#2322
Fix nvm_ls_current
fast unit test
#2322
Conversation
It's not clear to me why this test is passing in CI but fails locally, and I'm hesitant to change the test without knowing why this change fixes it. |
I think I mixed up return_zero being truthy/falsey in my initial explanation, and thought that that was the issue 😅 Digging deeper I believe I've figured out some things:
Based on this, I'm working on a fix that links the correct |
Thanks, I really appreciate the digging, and this sounds like the right track. |
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.
Thanks, almost there :-)
test/fast/Unit tests/nvm_ls_current
Outdated
@@ -19,20 +19,21 @@ fi | |||
|
|||
rm -rf "$TEST_DIR" | |||
mkdir "$TEST_DIR" | |||
ln -s "$(command which which)" "$TEST_DIR/which" | |||
# Ensure that the system version of which is used, not node_modules/.bin/which | |||
ln -s "$(PATH="/usr/bin" command which which)" "$TEST_DIR/which" |
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 don't think /usr/bin
will always be where it's located. i think the issue is that node_modules/.bin
is in the PATH when npm run
is used - so, can we remove that from the PATH rather than hardcoding one possible location of which
?
ln -s "$(command which dirname)" "$TEST_DIR/dirname" | ||
ln -s "$(command which printf)" "$TEST_DIR/printf" | ||
|
||
[ "$(PATH="$TEST_DIR" nvm_ls_current)" = "none" ] || die 'when node not installed, nvm_ls_current did not return "none"' | ||
[ "@$(PATH="$TEST_DIR" nvm_ls_current 2> /dev/stdout 1> /dev/null)@" = "@@" ] || die 'when node not installed, nvm_ls_current returned error output' | ||
|
||
echo "#!/bin/bash" > "$TEST_DIR/node" | ||
echo "echo 'VERSION FOO!'" > "$TEST_DIR/node" | ||
echo "echo 'VERSION FOO!'" >> "$TEST_DIR/node" |
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.
(-‸ლ) oops
Thanks for the review! Made the change :) |
@@ -19,20 +19,21 @@ fi | |||
|
|||
rm -rf "$TEST_DIR" | |||
mkdir "$TEST_DIR" | |||
ln -s "$(command which which)" "$TEST_DIR/which" | |||
# Ensure that the system version of which is used, not node_modules/.bin/which | |||
ln -s "$(PATH=$(echo $PATH | tr ":" "\n" | grep -v "node_modules/.bin" | tr "\n" ":") command which which)" "$TEST_DIR/which" |
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.
hmm, too bad there's no ergonomic way to use nvm_strip_path
here
Fixes one of the failing tests in #2181
nvm_ls_current
returns 'system' whennvm_tree_contains_path
is aliased to return zero, as if nvm is deactivated. This fix instead setsNVM_DIR
to matchTEST_DIR
so thatnvm_tree_contains_path
passes and the mock executable is actually used. Let me know if this approach is reasonable!Relevant branches in
nvm_ls_current
:nvm/nvm.sh
Lines 915 to 925 in e01060f