Skip to content

test: fix spawn on windows - #7049

Merged
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:test-fix-win32-child-process-flush-stdio
Jun 7, 2016
Merged

test: fix spawn on windows#7049
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:test-fix-win32-child-process-flush-stdio

Conversation

@mscdex

@mscdexmscdex commented May 29, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • test
Description of change

Most Windows systems do not have an external echo program installed, so any attempts to spawn echo as a child process will fail with ENOENT. This commit forces the use of the built-in echo provided by cmd.exe.

@mscdexmscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. labels May 29, 2016
@mscdex

Copy link
Copy Markdown
ContributorAuthor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you just do const opts = {shell: common.isWindows};

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed.

@cjihrig

Copy link
Copy Markdown
Contributor

LGTM. How was this working on the CI machines?

@mscdex

Copy link
Copy Markdown
ContributorAuthor

@cjihrig Probably they have some sort of mingw/cygwin/etc. environment installed? I noticed it while running tests on a local Windows 7 VM that doesn't have any of that kind of stuff installed.

@mscdex
mscdexforce-pushed the test-fix-win32-child-process-flush-stdio branch from cddfef0 to 883f253CompareMay 29, 2016 18:22
@mscdex

Copy link
Copy Markdown
ContributorAuthor

@joaocgreis

Copy link
Copy Markdown
Member

Windows CI machines have Git for Windows installed, and use the executables provided by that. There should be a few more tools required to run the tests, I once compiled a list but it should be quite outdated now: https://github.com/nodejs/node/wiki/Installation#building-on-windows (bottom of that section).

It's good to see one of those dependencies go away, LGTM.

@mscdex
mscdexforce-pushed the test-fix-win32-child-process-flush-stdio branch 2 times, most recently from 94cd580 to b04f1baCompareJune 7, 2016 03:36
@mscdex

Copy link
Copy Markdown
ContributorAuthor

CI one last time before landing: https://ci.nodejs.org/job/node-test-pull-request/2942/

@addaleax

Copy link
Copy Markdown
Member

LGTM

Most Windows systems do not have an external `echo` program installed,
so any attempts to spawn `echo` as a child process will fail with
`ENOENT`. This commit forces the use of the built-in `echo` provided
by `cmd.exe`.
PR-URL: nodejs#7049
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mscdex
mscdexforce-pushed the test-fix-win32-child-process-flush-stdio branch from b04f1ba to e18a926CompareJune 7, 2016 04:05
@mscdex
mscdex merged commit e18a926 into nodejs:masterJun 7, 2016
@mscdex
mscdex deleted the test-fix-win32-child-process-flush-stdio branch June 7, 2016 04:06
evanlucas pushed a commit that referenced this pull request Jun 15, 2016
Most Windows systems do not have an external `echo` program installed,
so any attempts to spawn `echo` as a child process will fail with
`ENOENT`. This commit forces the use of the built-in `echo` provided
by `cmd.exe`.
PR-URL: #7049
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@evanlucasevanlucas mentioned this pull request Jun 16, 2016
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
Most Windows systems do not have an external `echo` program installed,
so any attempts to spawn `echo` as a child process will fail with
`ENOENT`. This commit forces the use of the built-in `echo` provided
by `cmd.exe`.
PR-URL: #7049
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Most Windows systems do not have an external `echo` program installed,
so any attempts to spawn `echo` as a child process will fail with
`ENOENT`. This commit forces the use of the built-in `echo` provided
by `cmd.exe`.
PR-URL: #7049
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Most Windows systems do not have an external `echo` program installed,
so any attempts to spawn `echo` as a child process will fail with
`ENOENT`. This commit forces the use of the built-in `echo` provided
by `cmd.exe`.
PR-URL: #7049
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorinsMylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Most Windows systems do not have an external `echo` program installed,
so any attempts to spawn `echo` as a child process will fail with
`ENOENT`. This commit forces the use of the built-in `echo` provided
by `cmd.exe`.
PR-URL: #7049
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Most Windows systems do not have an external `echo` program installed,
so any attempts to spawn `echo` as a child process will fail with
`ENOENT`. This commit forces the use of the built-in `echo` provided
by `cmd.exe`.
PR-URL: #7049
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@sxa

sxa commented Dec 29, 2016

Copy link
Copy Markdown
Member

@thealphanerd While this landed cleanly on V4 it doesn't work and the test case still fails there. The shell:true option only exists in V6 and later. I'm submitting a separate PR to implement the fix differently in that branch

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

child_processIssues and PRs related to the child_process subsystem.testIssues and PRs related to the tests.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@mscdex@cjihrig@joaocgreis@addaleax@sxa@MylesBorins