Skip to content
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

test: make a test path-independent #12945

Closed
wants to merge 1 commit into from
Closed

test: make a test path-independent #12945

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented May 10, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test, child_process

This is the first step to fix the #12773.

I've decided to start from the most whimsical one (section 4 in the #12773): parallel/test-spawn-cmd-named-pipe.js fails if spaces exist both in node.exe and the test paths. If there are no spaces or there are spaces only in one of the paths (either), the test passes.

If I get it right, the cause is the very complicated rules for cmd.exe command line syntax concerning spaces and quotes: see "Quote Characters in a command" here and "Examples:" here.

The fixed test passes with all 4 paths variants (spaceless + spaceless, spacy + spaceless, spaceless + spacy, and spacy + spacy).

Please, test in more environments and propose better fixes if you have some)

cc @nodejs/testing, @nodejs/platform-windows, @bnoordhuis, @cjihrig

@vsemozhetbyt vsemozhetbyt added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels May 10, 2017
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 10, 2017
@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

The test runs only on Windows and Windows is OK in CI. One Linux fail must be irrelevant.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go this route, the test may need to be renamed since it no longer uses spawn().

'<', stdinPipeName, '>', stdoutPipeName];

const child = spawn(comspec, args);
const child = exec(`"${process.execPath}" "${__filename}" child < ${
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would quoting the args and specifying the shell work with spawn()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried and failed. It seems the args are re-quoted internally in an inappropriate way for this case. But I may miss some variants. Can anybody else test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does windowsVerbatimArguments make a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this documented? I can't find.

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found out a way with spawn():

  const args = ['/c', `""${process.execPath}"`, `"${__filename}"`, 'child',
                '<', stdinPipeName, '>', `${stdoutPipeName}"`];

  const child = spawn(comspec, args, { shell: true });

But it seems strange. And we execute cmd.exe with cmd.exe first, if I get this right. Is this way better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

windowsVerbatimArguments is not documented. See

node/lib/child_process.js

Lines 424 to 428 in cfe7b34

if (process.platform === 'win32') {
file = typeof options.shell === 'string' ? options.shell :
process.env.comspec || 'cmd.exe';
args = ['/d', '/s', '/c', '"' + command + '"'];
options.windowsVerbatimArguments = true;
.

When you run exec() or spawn() with shell: true, this code path is hit. It's probably what you're looking for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way:

  const args =
    [`"${__filename}"`, 'child', '<', stdinPipeName, '>', stdoutPipeName];

  const child = spawn(`"${process.execPath}"`, args, { shell: true });

Slightly better maybe than the previous one.

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work:

  const args = ['/c', `"${process.execPath}"`, `"${__filename}"`, 'child',
                '<', stdinPipeName, '>', stdoutPipeName];

  const child = spawn(comspec, args,
                      { shell: true, windowsVerbatimArguments: true });

Should I prefer one of the beforementioned variants with spawn()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variant in #12945 (comment) seems reasonable enough.

parallel/test-spawn-cmd-named-pipe.js failed with spaces
both in node.exe and test paths.
@vsemozhetbyt
Copy link
Contributor Author

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green

@refack
Copy link
Contributor

refack commented May 10, 2017

@vsemozhetbyt you could update the first comment, since you eventually found a way to use spawn
IMHO you could also change title to something simpler like: test: make test path-robust or path agnostic

@vsemozhetbyt vsemozhetbyt changed the title test: make parallel/test-spawn-cmd-named-pipe.js path-independent test: make a test path-independent May 10, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CI

@vsemozhetbyt
Copy link
Contributor Author

@refack I've unified with the #12812

@vsemozhetbyt
Copy link
Contributor Author

Windows CI is green.

@vsemozhetbyt
Copy link
Contributor Author

Landed in 529e4f2

@vsemozhetbyt vsemozhetbyt deleted the test-spawn-cmd-named-pipe branch May 12, 2017 08:37
vsemozhetbyt added a commit that referenced this pull request May 12, 2017
parallel/test-spawn-cmd-named-pipe.js failed with spaces
both in node.exe and test paths.

PR-URL: #12945
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
parallel/test-spawn-cmd-named-pipe.js failed with spaces
both in node.exe and test paths.

PR-URL: nodejs#12945
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants