test: fix test-child-process-flush-stdio on windows (Node V4 only) #10523

sxa · 2016-12-29T17:06:54Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

When 65b5ccc landed in V4, the fix didn't work there - it relies on the V6 "shell:true" option on child_process.spawn() that was added after V4, therefore the test failes when there is no "echo.exe" on Windows systems. This fix implements the same functionality (cmd/c echo) that the shell:true option would have given.

The "shell" option was added in c3bb4b1 and I'm assuming we have no desire to land that option back in V4 just to fix the test case, so I am adjusting the test.

FYA: @thealphanerd @mscdex

sam-github · 2016-12-29T17:26:46Z

Looks to me like shell was intended to land in v4, it probably got overlooked: #4598 (comment)

MylesBorins · 2016-12-29T17:41:43Z

I've put the lts-agenda tag on it. A bit back the tag was deleted accidentally and we lost the issues we had tagged. I'm planning to do an audit of semver minors for each release line and putting together a meta issue in the near future

sam-github · 2016-12-29T18:02:42Z

That said, this LGTM, though I wonder if there is a Windows equivalent to /bin/echo, perhaps a print.exe (reaching into my dusty DOS memories)? That would allow the change to be a bit smaller.

sxa · 2016-12-29T18:08:37Z

Nothing that I know off from the top of my head. The DOS/Windows shell has a lot of the stuff integrated into it so you don't tend to need so many externals. Plus even if there was an equivalent that wasn't called "echo" you'd still have to special case windows to invoke it, although it might look a bit cleaner.

mhdawson

LGTM

In most cases, named functions match the variable or property to which they are being assigned. That also seems to be the practice in a series of PRs currently being evaluated that name currently-anonymous functions. This change applies that rule to instances in the code base that don't comply with that practice. This will be enforceable with a lint rule once we upgrade to ESLint 3.8.0. PR-URL: nodejs#9113 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell · 2017-01-06T17:54:31Z

Can you rebase this? It appears to have become out of sync since the most recent v4 release

The cherry-pick of nodejs#7612 to v4.x (4369055) added in nodejs#9298 wasn't quite correct as it depends on a runtime function %SymbolDescriptiveString that doesn't exist on v4.x. We can use %SymbolDescription instead. Ref: nodejs#7612 Ref: nodejs#9298 PR-URL: nodejs#10732 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This backport does not include the original changes to SLOW_DCHECK as it does not exist in the V8 in node v4.x Original commit message: Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.org Review-Url: https://codereview.chromium.org/2078403002 Cr-Commit-Position: refs/heads/master@{nodejs#37108} PR-URL: nodejs#10668 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

This backport does not include the changes to `src/heap/scavenger.cc` as it does not exist in the V8 included in the v4.x stream. Original commit message: Filter out stale left-trimmed handles for scavenges The missing part from https://codereview.chromium.org/2078403002/ R=jochen@chromium.org BUG=chromium:621869 LOG=N Review-Url: https://codereview.chromium.org/2077353004 Cr-Commit-Position: refs/heads/master@{nodejs#37184} PR-URL: nodejs#10668 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

This commit does not include the changes to `src/heap/scavenger.cc`. These changes would revert the changes that should have come in 086bd5aede, meaning that there is no issue with that change missing in the previous commit. Original commit message: Iterate handles with special left-trim visitor BUG=chromium:620553 LOG=N R=hpayer@chromium.org Review-Url: https://codereview.chromium.org/2102243002 Cr-Commit-Position: refs/heads/master@{nodejs#37366} PR-URL: nodejs#10668 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

PR-URL: nodejs#10668 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

Provide means to inspect information about the separate heap spaces via a callable API. This is helpful to analyze memory issues. Fixes: nodejs#2079 PR-URL: nodejs#4463 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: nodejs#1009 PR-URL: nodejs#4598 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

This uses libuv's mkdtemp function to provide a way to create a temporary folder, using a prefix as the path. The prefix is appended six random characters. The callback function will receive the name of the folder that was created. Usage example: fs.mkdtemp('/tmp/foo-', function(err, folder) { console.log(folder); // Prints: /tmp/foo-Tedi42 }); The fs.mkdtempSync version is also provided. Usage example: console.log(fs.mkdtemp('/tmp/foo-')); // Prints: tmp/foo-Tedi42 This pull request also includes the relevant documentation changes and tests. PR-URL: nodejs#5333 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: nodejs#9587 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net>

- Replace require() vars with const. - Replace assert.equal() with assert.strictEqual(). - Add common.mustCall() to the setTimeout() callback. PR-URL: nodejs#9995 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Asserts that an error should be thrown when an invalid signal is passed to process.kill(). PR-URL: nodejs#10026 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently, there are a number of popups that get displayed when running the tests asking to accept incoming network connections. Rules can be added manually to the socket firewall on Mac OS X but getting this right might not be obvious and quite a lot of time can be wasted trying to get the rules right. This script hopes to simplify things a little so that it can be re-run when needed. The script should be runnable from both the projects root directory and from the tools directory, for example: $ sudo ./tools/macosx-firewall.sh Fixes: nodejs#8911 PR-URL: nodejs#10114 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: nodejs#9864 Refs: nodejs#8683 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Ref: nodejs#10037 Ref: nodejs#10146 PR-URL: nodejs#10149 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Currently, two of the guides in the `/doc/guides` directory are actually guides for working on the Nodei.js project. Of those, one is linked from this page. This change adds a note to point people to the other. PR-URL: nodejs#10070 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Many of the tests use variables to track when callback functions are invoked or events are emitted. These variables are then asserted on process exit. This commit replaces this pattern in straightforward cases with common.mustCall(). This makes the tests easier to reason about, leads to a net reduction in lines of code, and uncovered a few bugs in tests. This commit also replaces some callbacks that should never be called with common.fail(). PR-URL: nodejs#7753 Reviewed-By: Wyatt Preul <wpreul@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Refactored `test-tls-server-verify.js` to replace uses of `var` with `const` and `let`. Also replaced uses of `assert.equal` with `assert.strictEqual`. PR-URL: nodejs#10076 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Use asssert.strictEqual to disallow coersion. PR-URL: nodejs#10071 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

* var -> const/let * assert.equal() -> assert.strictEqual() PR-URL: nodejs#10061 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Updating tests to use `common.fixturesDir` whenever possible/reasonable. Left out things like tests for `path` and `require.resolve`. PR-URL: nodejs#6997 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

change equal to strictEqual and var to const PR-URL: nodejs#9941 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

* var -> const, let * assert.equal() -> assert.strictEqual() PR-URL: nodejs#9948 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The second argument to "assert.throws" is usually a validation RegExp or function for the thrown error. However, the function also accepts a string and in this case it is interpreted as a message for the AssertionError and not used for validation. It is common for people to forget this and pass a validation string by mistake. This new rule checks that we never pass a string literal as a second argument to "assert.throws". Additionally, there is an option to enforce the function to be called with at least two arguments. It is currently off because we have many tests that do not comply with this rule. PR-URL: nodejs#10089 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Extend the assert-throws-arguments custom ESLint rule to also check for the use of template literals as a second argument to assert.throws. PR-URL: nodejs#10301 Ref: nodejs#10282 (comment) Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Italo A. Casas <me@italoacasas.com>

The assert.throws() calls in test-event-emitter-max-listeners.js should include a constructor or RegExp as a second argument. PR-URL: nodejs#9987 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

- Updated assert.equal to assert.strictEqual - Updated 'var' to 'const' - Using template literals PR-URL: nodejs#10036 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

Requiring a file from a directory that contains an invalid package.json file should throw an error. PR-URL: nodejs#10044 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* assert.equal() -> assert.strictEqual() * replace template string with a string; no variable substitution or concatenation or anything like that PR-URL: nodejs#9803 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

We have a tacit rule that for multiline statements, the operator should be placed before the linebreak. This commit commit fixes the few violations of this rule in the code base. This allows us to enable the corresponding ESLint rule. PR-URL: nodejs#10178 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Adds the `operator-linebreak` rule to our ESLint config. PR-URL: nodejs#10178 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

const and let instead var assert.strictEqual instead assert.equal PR-URL: nodejs#8668 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

addaleax · 2017-01-29T06:12:28Z

bump @sxa555

This test cases tries to use the V6 "shell:true" option on child_process.spawn that isn't in V4, therefore the test failes when there is no "echo.exe" on Windows systems. This fix implements the same functionality (cmd/c echo) that the shell:true option would have given.

sxa · 2017-01-31T14:00:47Z

Rebased

gibfahn · 2017-01-31T14:09:03Z

test/parallel/test-child-process-flush-stdio.js

-const p = cp.spawn('echo', [], opts);
+// executable like on *nix. The V4 API does not have the "shell" option to
+// spawn that we use in V4 and later so we can't use that here.
+var p;


Can you use let p here so this doesn't cause conflicts when/if #10685 is backported?

gibfahn · 2017-01-31T14:16:19Z

test/parallel/test-child-process-flush-stdio.js

@@ -19,7 +23,12 @@ p.stdout.read();

 function spawnWithReadable() {
  const buffer = [];
-  const p = cp.spawn('echo', ['123'], opts);
+  var p;


MylesBorins · 2017-02-06T19:12:52Z

@sxa555 this still needs a rebase

MylesBorins · 2017-03-03T01:50:25Z

ping @sxa555

MylesBorins · 2017-03-11T19:45:42Z

Since this has not been rebased and we are not likely to do another patch before v4 goes into maintenance I'm going to close this

nodejs-github-bot added test Issues and PRs related to the tests. v4.x labels Dec 29, 2016

mscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. labels Dec 29, 2016

sxa changed the title ~~test: fix test-child-process-flush-stdio on windows~~ test: fix test-child-process-flush-stdio on windows (Node V4 only) Dec 29, 2016

MylesBorins self-assigned this Dec 29, 2016

bzoz approved these changes Dec 30, 2016

View reviewed changes

mhdawson approved these changes Jan 3, 2017

View reviewed changes

gibfahn approved these changes Jan 3, 2017

View reviewed changes

jasnell approved these changes Jan 3, 2017

View reviewed changes

MylesBorins force-pushed the v4.x-staging branch from 5dee225 to 4357591 Compare January 4, 2017 00:04

ofrobots and others added 13 commits January 12, 2017 17:29

deps: update patch level in V8

20bee0f

PR-URL: nodejs#10668 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

test: stream readableListening internal state

3bd7ab1

PR-URL: nodejs#9864 Refs: nodejs#8683 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

addaleax and others added 18 commits January 22, 2017 22:47

test: refactor test-tls-server-verify

f4e76eb

Refactored `test-tls-server-verify.js` to replace uses of `var` with `const` and `let`. Also replaced uses of `assert.equal` with `assert.strictEqual`. PR-URL: nodejs#10076 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

test: refactor test-repl-mode.js

beffa82

* var -> const/let * assert.equal() -> assert.strictEqual() PR-URL: nodejs#10061 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

test: test: refactor test-sync-fileread

657d3f3

change equal to strictEqual and var to const PR-URL: nodejs#9941 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

test: add second argument to assert.throws()

87463bf

The assert.throws() calls in test-event-emitter-max-listeners.js should include a constructor or RegExp as a second argument. PR-URL: nodejs#9987 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

sxa force-pushed the v4-flushstdio-winfix branch from f954985 to c55ad84 Compare January 31, 2017 13:51

gibfahn suggested changes Jan 31, 2017

View reviewed changes

MylesBorins force-pushed the v4.x-staging branch from f5c57c7 to 735119c Compare February 1, 2017 01:04

MylesBorins force-pushed the v4.x-staging branch from bba3dcf to 6fa3c73 Compare February 22, 2017 01:39

jasnell force-pushed the v4.x-staging branch from eae55e5 to 3b11d89 Compare March 3, 2017 14:45

MylesBorins closed this Mar 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix test-child-process-flush-stdio on windows (Node V4 only) #10523

test: fix test-child-process-flush-stdio on windows (Node V4 only) #10523

sxa commented Dec 29, 2016 •

edited

Loading

sam-github commented Dec 29, 2016

MylesBorins commented Dec 29, 2016

sam-github commented Dec 29, 2016

sxa commented Dec 29, 2016

mhdawson left a comment

jasnell commented Jan 6, 2017

addaleax commented Jan 29, 2017

sxa commented Jan 31, 2017

gibfahn Jan 31, 2017

gibfahn Jan 31, 2017

MylesBorins commented Feb 6, 2017

MylesBorins commented Mar 3, 2017

MylesBorins commented Mar 11, 2017

test: fix test-child-process-flush-stdio on windows (Node V4 only) #10523

test: fix test-child-process-flush-stdio on windows (Node V4 only) #10523

Conversation

sxa commented Dec 29, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

sam-github commented Dec 29, 2016

MylesBorins commented Dec 29, 2016

sam-github commented Dec 29, 2016

sxa commented Dec 29, 2016

mhdawson left a comment

Choose a reason for hiding this comment

jasnell commented Jan 6, 2017

addaleax commented Jan 29, 2017

sxa commented Jan 31, 2017

gibfahn Jan 31, 2017

Choose a reason for hiding this comment

gibfahn Jan 31, 2017

Choose a reason for hiding this comment

MylesBorins commented Feb 6, 2017

MylesBorins commented Mar 3, 2017

MylesBorins commented Mar 11, 2017

sxa commented Dec 29, 2016 •

edited

Loading