Bugfix: for whitespace in homedir paths

[MarcusPope]

Fixes istanbuljs/nyc#784

Tested on windows 10 in powershell and cmd.

[coreyfarrell]

CC @demurgos

[coreyfarrell]

I don't really have access to Windows, can you confirm that this test fails without the change in lib/mungers/cmd.js?

[demurgos]

The change looks good enough to support spaces. I'll run tests on Windows.

[demurgos]

I believe that test does not correspond to the code change. The code adds support for shim dirs with spaces (shim dir = home dir by default). The test checks support for node binaries in a path with spaces, but it still uses the home dir. I believe the test should instead spawn the subprocess with SPAWN_WRAP_SHIM_ROOT set to dir.

Could you check it and fix it?

[demurgos]

The file basic.js is not suited for this. sw is called at the very top, before you can create the temporary directory and configure the environment. You'll need to move the test to its own script.

[demurgos]

I confirm that I managed to reproduce an issue fixed by this PR: using cp.exec (spawn in shell) on windows to spawn npm (e.g. cp.exec("npm --version")). Spawning Node did not seem to have any issues (and isn't covered by the branch changed by the PR).

What I recommend for the test: use a separate script, create a temporary directory with a space, spawn a sub-script (or itself, check how the existing files are structured) with SPAWN_WRAP_SHIM_ROOT set to the dir path, in this sub-process enable spawn-wrap and use cp.exec("npm --version").
Without the PR, this fails. With the PR this succeeds.

Edit: A simpler test would be a unit-test based off #99 that only checks mungeCmd. It's not end-to-end, but is simpler to set-up.

[MarcusPope]

I believe that test does not correspond to the code change. The code adds support for shim dirs with spaces (shim dir = home dir by default). The test checks support for node binaries in a path with spaces, but it still uses the home dir. I believe the test should instead spawn the subprocess with SPAWN_WRAP_SHIM_ROOT set to dir.
Could you check it and fix it?

I don't think that distinction matters in this case, as all three variables (my test's node binary path with spaces, SPAWN_WRAP_SHIM_ROOT, and os.homedir) are passed into cp.exec at the same argument index.

os.homedir / SHIM_ROOT:

  spawnargs:
   [ 'C:\\windows\\system32\\cmd.exe',
     '/s',
     '/c',
     '" "C:\\Users\\Marcus Pope\\.node-spawn-wrap-13412-96b49b176f57/node.cmd" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "test""' ],

my unit test:

  spawnargs:
   [ 'C:\\windows\\system32\\cmd.exe',
     '/d',
     '/s',
     '/c',
     '""C:\\\\data\\\\istanbuljs\\\\spawn-wrap-fork\\\\test\\\\fixtures\\\\space path\\\\node.exe" "C:\\Users\\Marcus Pope\\.node-spawn-wrap-13728-8c3db94bb9d8\\node" C:\\data\\istanbuljs\\spawn-wrap-fork\\test\\fixtures\\script.js xyz"' ],

We're only concerned about the first "argument" in the last index of the spawnargs array getting wrapped in quotes. Before my fix, both failed because of the space in my user name and the space in the node.exe path, and after my fix both succeed. Granted, my test only checks the latter scenario but the mungeCmd logic gets invoked after the homedir.js require so both will be quoted properly.

Please let me know if I'm missing something as a result of my admittedly brief review of the code base. If you are concerned about future proofing the logic I would be ok with adding a legit "unit" test to mungeCmd that ensures quotes are wrapped around the first argument. But otherwise I think the SHIM_ROOT-specific test would ultimately just duplicate the work of this test without any testing benefit.

[MarcusPope]

Spawning Node did not seem to have any issues (and isn't covered by the branch changed by the PR).

Yeah, I didn't need to change that side of the if-branch as it was already properly quoted. I presumed there was already a test somewhere that covered that. But there is a potential conflict with that logic and another test that tries to invoke a node binary renamed to something else. I posted about it here but it didn't get much traction: https://node-tooling.slack.com/archives/C6FL6C221/p1561761510098800

[demurgos]

@MarcusPope I saw that you sent some changes, is the PR ready?

[coreyfarrell]

@demurgos #100 appears to show that the new test fails without the cmd munger change. I have no objections to this change, I can merge this on your approval.

[MarcusPope]

@demurgos Yup, it's ready for merge.

[demurgos]

The fix was simple but the test required more work: good job. I am OK to merge.

[coreyfarrell]

@MarcusPope @demurgos thanks for this contribution!