-
-
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 exec
: Do a version check on nvm-exec
#3308
base: master
Are you sure you want to change the base?
Conversation
This check would display a message in case the `.nvmrc` version is not installed, and would not alter the output otherwise.
This check would display a message in case the `.nvmrc` version is not installed, and would not alter the output otherwise.
c6cfc3a
to
c20db2a
Compare
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.
This would need tests
A test case was added that checks if the message printed out contains the relevant information (different version of 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.
Thanks! (I'm going to tweak the test to be "fast" instead of "slow", and to use a "never can exist" version instead of needing to remove a version, but otherwise this is great!)
nvm-exec
nvm exec
: Do a version check on nvm-exec
50480e4
to
05feeaa
Compare
Unfortunately there's 10 tests failing. It's possible it's just that the expected output needs to be updated to reflect the change, but I might not have time to work on it soon, so it'd be great if you could take a look :-) |
…ed other tests to fail. Updated version color printing test because it did not pass.
Gave it a quick look. Also saw that the version color printing test was failing, and went ahead and updated it. Currently staying on 7 errors when running in the docker container, but all other failures seem unrelated to the current changes and a bit more time consuming to debug. |
@@ -16,3 +16,4 @@ No NODE_VERSION provided; no .nvmrc file found"; | |||
|
|||
# Skip install, we want to test the error message | |||
diff <(echo "${EXPECTED}") <(echo "${OUTPUT}") | |||
rm .nvmrc |
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.
aha, good catch. note tho that this doesn't actually restore what might already be present - we should back up and restore the existing .nvmrc
in setup/teardown files
i'll take a crack at this part tho
No NODE_VERSION provided; no .nvmrc file found"; | ||
|
||
# Skip install, we want to test the error message | ||
diff <(echo "${EXPECTED}") <(echo "${OUTPUT}") |
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.
this line can't work tho, due to https://www.shellcheck.net/wiki/SC3001 - i'll change this to a normal =
comparison unless you have a better idea
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.
Yeah, sound reasonable.
Was hoping that the #!/bin/bash
on top would allow for that to pass, but I think =
should work too.
This check would display a message in case the
.nvmrc
version is not installed, and would not alter the output otherwise.nvm_ensure_version_installed
returns the message that we care for.It requires an argument (version) against which to do the check, which we get after calling
nvm_rc_version
Fixes #1769