[New] nvm use/nvm install: add --save option

[maartin0]

In this PR I've:

• added a --save option as per nvm use should write a .nvmrc file by default #2849
• implemented it for both nvm use and nvm install (which actually just carries over the option)
• manually tested it in bash, sh and zsh
• written one test, however I can't get the slow tests to run on my system so hopefully it runs okay 🤞 (if someone would be able to check that, that'd be great)

[ljharb]

Let's add a bunch more tests - here's some example input:

• nvm use --save --lts
• nvm use --save lts/argon
• nvm use --save lts/*
• nvm use --save node
• nvm use --save iojs
• nvm use --save foo (where foo is a custom user alias)
• nvm use --save (where the version is read from a .nvmrc in a higher directory)

Let's also add some error tests for the cases where the .nvmrc file fails to be written (no permissions in the current directory, or the disk is full, for example)

[maartin0]

Thanks for all the advice, it's been really helpful!
With the tests, would you suggest making seperate files for each one or loosely grouping them?

[ljharb]

If you can make them be "fast" tests, then one file is totally fine - mainly it'll depend on how much setup is required. Generally each test file should only have one batch of setup at the beginning, and one batch of cleanup at the end.

[ljharb]

The nvm install and nvm use tests should definitely be in separate files.

Also, could the tests all be moved to fast? they'd be able to use mocks for nvm ls-remote calls, and as long as the dynamically-looked-up expected versions were fake-installed already, then they wouldn't need to actually install anything.

[maartin0]

Also, could the tests all be moved to fast? they'd be able to use mocks for nvm ls-remote calls, and as long as the dynamically-looked-up expected versions were fake-installed already, then they wouldn't need to actually install anything.

I've moved all of the test to 'fast' and split them into different files, however I'm not sure how to do fake installs (have you got any existing functions to do that?)

[ljharb]

however I'm not sure how to do fake installs (have you got any existing functions to do that?)

i do! grep for make_fake_node

[maartin0]

however I'm not sure how to do fake installs (have you got any existing functions to do that?)

i do! grep for make_fake_node

Hmm, I tried that (from common.js) and it didn't seem to prevent downloading the latest version

[maartin0]

If you'd like I can prepend that to all of the nvm install statements anyway

[ljharb]

ah right, the other piece is that you have to mock out nvm_ls_remote - see https://github.com/nvm-sh/nvm/blob/master/test/fast/Unit%20tests/nvm%20ls-remote#L59-L66 for an example.

[maartin0]

I'm not quite sure how to mock nvm_ls_remote, so I've added 3 todo notes as to where it needs to be ran;
All of the tests run fine, just some of them need to download specific versions of node

[ljharb]

I added the mocks; I think this might be good to go!

It'll have to wait until the broken travis tests on master are fixed; please keep the fork/branch undeleted and the PR open until then :-)

[ljharb]

I'll get the tests fixed and get this landed shortly.

[ljharb]

I intentionally use nonexistent node versions in the tests at some point; make_fake_node should be working fine already.

[maartin0]

I've refactored it a bit by moving the block under nvm_write_nvmrc and then only running it on nvm use (I removed the local declaration in the nvm use block so NVM_WRITE_TO_NVMRC is passed through). Can you figure out why the tests aren't working? They run fine as independent shells scripts or by running urchin directly in the test/fast/Unit tests directory, but they fail as soon as you try running it further up and I can't get any error messages out of it- is something in one of the setup or setup_dir scripts interfering with it?

[ljharb]

This is looking pretty good assuming tests pass! (you can ignore "automatic rebase" and the failing WSL tests)

[maartin0]

@ljharb The issue with the tests was that I was using set -e when sourcing nvm.sh which I only just realised you haven't done in any of the other tests in fast/Unit tests - is that ok? It seems a bit dodgy

[maartin0]

Just pushed again with a 3 more things:

• Added -w alias for --save and a test to go with it
• Fixed a bug in nvm_auto where it tries to resolve the alias default which resolves to N/A which is a non-empty string but invalid version so when it calls nvm use or nvm install it'll exit 3 if set -e is set -- this has got the number of failing fast tests from 7 down to 2
• Added set -e flags back to the tests I wrote since nvm.sh can be sourced successfully now

[ljharb]

@ljharb The issue with the tests was that I was using set -e when sourcing nvm.sh which I only just realised you haven't done in any of the other tests in fast/Unit tests - is that ok? It seems a bit dodgy

yeah, that's fine - sometimes i have to set -e after sourcing nvm.sh.

setting -e isn't even really a good practice anymore, so i'm not too worried about it. good to set it in tests if possible tho :-)

[maartin0]

Just rebased again if you think this is ready to merge?

[ljharb]

Unfortunately the Travis tests are still failing - https://app.travis-ci.com/github/nvm-sh/nvm/jobs/619295015

[maartin0]

@ljharb There was only one issue which was surprisingly simple to fix, but annoying to figure out - I just needed to add a couple of extra bits to the cleanup functions in all the tests, since they were leaving behind installed versions which the other tests didn't expect, which in turn changed the output format of nvm ls-remote to literally just have one blue rather than black line:

And this is what I needed to change cleanup to (which I stole from nvm_install_no_progress_bar)

cleanup () {
  nvm cache clear
  nvm deactivate
  nvm unalias default
  rm -rf ${NVM_DIR}/v* .nvmrc
  if [ -f .nvmrc.orig ]; then mv .nvmrc.orig .nvmrc; fi
  unset -f nvm_ls_remote nvm_ls_remote_iojs
}