Node: test: tests fails on Windows with spaces in paths

[vsemozhetbyt]

• Version: 8.0.0-nightly20170430
• Platform: Windows 7 x64
• Subsystem: test

CONTRIBUTING.md states:

You can run tests directly with node:

$ ./node ./test/parallel/test-stream2-transform.js

Though this example shows specific command, a user may assume that tests can be launched with a node in any path, as well as the tests themselves can be placed in a folder with any path.

This is mostly true. However, there are some tests that fail if a space character exists in any of these paths, even if the command elements are enclosed in quotes.

I've tried to find these tests, however, this list may be not complete.

It seems these cases better be fixed to ease quick tests verification during various editing.

And what is more, a user can clone the repository in the folder with spaces in path and test run will be compromised: in this case both the tests and the executable (in Release folder) will contain spaces in parent paths.

1. Tests fail with spaces in node.exe path. You can use either an absolute path to node.exe (as in the examples below) or just node (if the PATH resolves this to the absolute path with spaces — which is the default behavior on Windows). Parent test folders do not contain spaces (you can use either absolute paths outside or relative ones inside the repo folder).

the list with test commands:

"j:/temp/node master/Release/node.exe" --expose_internals j:/temp/node-master/test/parallel/test-child-process-exec-kill-throws.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-0.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-1.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-2.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-3.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-4.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-5.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-6.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-7.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-8.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-no-error-handler-abort-on-uncaught-9.js

2. Tests fail with spaces in test path. Use an absolute path to node.exe without spaces and path to tests with spaces (or just run this with relative tests paths inside repo folder with spaces in parent path).

the list with test commands:

j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-cli-eval.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-cli-node-options.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/sequential/test-module-loading.js"

3. Tests fail with spaces in node.exe OR test path (either variant with the same tests fail).

the list with test commands:

"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/sequential/test-domain-abort-on-uncaught.js
"j:/temp/node master/Release/node.exe" --expose_internals j:/temp/node-master/test/parallel/test-child-process-bad-stdio.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-child-process-exec-encoding.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-child-process-exec-timeout.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-throw-error-then-throw-from-uncaught-exception-handler.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-domain-with-abort-on-uncaught-exception.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-env-var-no-warnings.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-http-chunk-problem.js
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/parallel/test-preload.js

j:/temp/node-master/Release/node.exe "j:/temp/node master/test/sequential/test-domain-abort-on-uncaught.js"
j:/temp/node-master/Release/node.exe --expose_internals "j:/temp/node master/test/parallel/test-child-process-bad-stdio.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-child-process-exec-encoding.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-child-process-exec-timeout.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-domain-throw-error-then-throw-from-uncaught-exception-handler.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-domain-with-abort-on-uncaught-exception.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-env-var-no-warnings.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-http-chunk-problem.js"
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/parallel/test-preload.js"

// should not fail on Windows
"j:/temp/node master/Release/node.exe" j:/temp/node-master/test/known_issues/test-stdout-buffer-flush-on-exit.js
j:/temp/node-master/Release/node.exe "j:/temp/node master/test/known_issues/test-stdout-buffer-flush-on-exit.js"

4. Tests fail with spaces in node.exe AND test path (both paths should contain spaces to fail).

the list with test commands:

"j:/temp/node master/Release/node.exe" "j:/temp/node master/test/parallel/test-spawn-cmd-named-pipe.js"

[vsemozhetbyt]

cc @nodejs/platform-windows

[refack]

Ref: #12084
We should document that (for now) you can't build or test on paths with spaces...

[addaleax]

@refack This issue is about our tests, so it should actually be fixable.

[refack]

@refack This issue is about our tests, so it should actually be fixable.

Ok. I'm going to dig in test.py and test/common.js, maybe I can find a quick fix.
IMHO the tests are a major components of the build process, so I think it is also a build issue.

@vsemozhetbyt is it only a Windows issue?

[vsemozhetbyt]

@addaleax @refack I can't test on other OSs. I shall try to fix this though, step by step, using this as a tracking issue. This bothered me while I have been making a big churn for tests (like https://github.com/nodejs/node/pull/12735) trying to test a test after each fix with my editor automation that calls node from C:\Program Files\nodejs\node.exe for an opened file: some files just fail due to the path.

[vsemozhetbyt]

@refack I think we may better fix this without changing test.py or test/common.js so any test can be launched autonomically despite the paths.

[refack]

test/common.js is required by almost all tests, if it's sensitive to spaces in paths, it will affect a lot of tests.

P.S. Break it down into a few PRs 😉
P.P.S. I'm trying to fix building in a path with a space (#12084, On Windows first)
P.P.P.S @vsemozhetbyt (1) WSL it's very nice. (2) VirtualBox/HyperV, because life...

[vsemozhetbyt]

@refack If I detect this sensitivity from the test/common.js side, I will try to address it. I suspect this is maybe connected with unquoted process.argv0 or something in tests, but I am not sure yet.

[vsemozhetbyt]

Will be fixed in https://github.com/nodejs/node/pull/12945 and https://github.com/nodejs/node/pull/12972.

One fix is turned over to https://github.com/nodejs/node/issues/12971.

[sam-github]

I can't even build node on Linux if there are spaces in the path. I tried checking node out as "space node", but build failed. So I tried again, checking it out as /home/sam/w/core/space dir/node, that still fails to build:

tools/icu/icuucx.target.mk:289: warning: overriding recipe for target '/home/sam/w/core/space'
tools/icu/icuucx.target.mk:286: warning: ignoring old recipe for target '/home/sam/w/core/space'
tools/icu/icuucx.target.mk:288: *** mixed implicit and normal rules: deprecated syntax
tools/icu/icuucx.target.mk:289: warning: overriding recipe for target 'dir/node/out/Release/obj.target/icuucx/%.o'
tools/icu/icuucx.target.mk:286: warning: ignoring old recipe for target 'dir/node/out/Release/obj.target/icuucx/%.o'
tools/icu/icuucx.target.mk:309: warning: overriding recipe for target '/home/sam/w/core/space'
tools/icu/icuucx.target.mk:289: warning: ignoring old recipe for target '/home/sam/w/core/space'
v8_inspector_compress_protocol_json.host.mk:13: warning: overriding recipe for target '/home/sam/w/core/space'
tools/icu/icuucx.target.mk:309: warning: ignoring old recipe for target '/home/sam/w/core/space'
v8_inspector_compress_protocol_json.host.mk:86: warning: overriding recipe for target '/home/sam/w/core/space'
v8_inspector_compress_protocol_json.host.mk:13: warning: ignoring old recipe for target '/home/sam/w/core/space'
/home/sam/w/core/space:2: *** missing separator.  Stop.
make[1]: Leaving directory '/home/sam/w/core/space dir/node/out'
Makefile:75: recipe for target 'node' failed
make: *** [node] Error 2
space dir/node (master $ u=) % pwd
/home/sam/w/core/space dir/node

I'm not convinced that having some restrictions on the paths that you do node development in is a bad thing. On the other hand, if some enterprising folks want to fix the build system to allow that, that's great.

[vsemozhetbyt]

@sam-github I think while Node.js is installed in the "C:\Program Files\nodejs" by default on Windows, it is worth to be extra careful.