| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Can you add a test for this? |
Sorry, something went wrong.
There was a problem hiding this comment.
Any reason not to be generous and permit null here? I believe our linter permits == null which evaluates to true for both null and undefined. In fact, there are two other places in this file already that use that.
Sorry, something went wrong.
There was a problem hiding this comment.
Adjusted that case.
Sorry, something went wrong.
|
Here's what the test file could look like. 'use strict';
const common = require('../common');
const assert = require('assert');
const { spawn } = require('child_process');
const tmpdir = require('../common/tmpdir');
const command = common.isWindows ? 'cd' : 'pwd';
const subprocess = spawn(command, undefined, { cwd: tmpdir.path });
subprocess.stdout.on('data', common.mustCall((data) => {
const actual = data.toString().trim().toLowerCase();
const expected = tmpdir.path.trim().toLowerCase();
assert.strictEqual(actual, expected);
})); |
Sorry, something went wrong.
|
Updated PR. |
Sorry, something went wrong.
There was a problem hiding this comment.
New tests should not include the copyright boilerplate.
Sorry, something went wrong.
There was a problem hiding this comment.
+
Sorry, something went wrong.
There was a problem hiding this comment.
Use subprocess.stdout.setEncoding('utf8') so you get strings instead of buffers and accumulate the strings.
toString-ing a buffer is unsound because the output may be split over multiple buffers (i.e., you can get multiple 'data' events and multi-byte sequences may be split across buffers.)
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch, overlooked that
Sorry, something went wrong.
|
PR has been updated according to comments. |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM as-is or with the comment I suggested adding.
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
See comment.
Sorry, something went wrong.
There was a problem hiding this comment.
There is already a test for this behaviour, could you expand it instead of adding another test file? See test-child-process-spawn-typeerror.js (and yes, it's name doesn't well reflect is purpose anymore). I assume this change effects, or SHOULD effect, every child_process API that accepts an argv argument, showing that they have identical handling of the argv option, which is what the test I mentioned is set up for.
Sorry, something went wrong.
There was a problem hiding this comment.
@sam-github Nit or requirement to land?
Sorry, something went wrong.
There was a problem hiding this comment.
I'd rather it lands with these changes. Particularly with API consistency.
Sorry, something went wrong.
There was a problem hiding this comment.
That's a little bit controversial.
This file contains lots of unrelated checks, like https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-spawn-typeerror.js#L115. Why do we test execFile function within spawn test file, shouldn't it be placed in test-child-process-exec-file.js or something like that ?
And we have already separate test file test-child-process-spawn-argv0.js for testing argv0 param, so what the problem with args ?
In my opinion, *-spawn-typeerror.js should contain tests just about incorrect types, and just for spawn method.
Sorry, something went wrong.
There was a problem hiding this comment.
In addition, that fix effects only spawn and spawnSync methods.
Sorry, something went wrong.
There was a problem hiding this comment.
I also favor smaller, more-focused test files, especially for child_process tests in parallel.
Sorry, something went wrong.
There was a problem hiding this comment.
I consider that tests that are intended to ensure multiple APIs have behave identically, when they should, are best contained within a single file.
At this time, I can't tell from the tests whether all the child_process APIs do, in fact, have identical arg handling. Also, I can't tell if this PR makes them more (or less) consistent. Its possible that it fixes an inconsistency. Its possible it makes them inconsistent. I don't know. Ideally, the test structure would make that very clear. Every PR doesn't have to be ideal. I'm removing my review.
Sorry, something went wrong.
There was a problem hiding this comment.
Added same test for spawnSync method.
Sorry, something went wrong.
Sorry, something went wrong.
|
Looks like there are relevant test failures on Windows: 15:40:38 not ok 64 parallel/test-child-process-spawn-args
15:40:38 ---
15:40:38 duration_ms: 0.251
15:40:38 severity: fail
15:40:38 exitcode: 1
15:40:38 stack: |-
15:40:38 events.js:174
15:40:38 throw er; // Unhandled 'error' event
15:40:38 ^
15:40:38
15:40:38 Error: spawn cd ENOENT
15:40:38 at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
15:40:38 at onErrorNT (internal/child_process.js:422:16)
15:40:38 at internalTickCallback (internal/process/next_tick.js:72:19)
15:40:38 at process._tickCallback (internal/process/next_tick.js:47:5)
15:40:38 at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
15:40:38 at executeUserCode (internal/bootstrap/node.js:317:15)
15:40:38 at startExecution (internal/bootstrap/node.js:251:3)
15:40:38 at startup (internal/bootstrap/node.js:202:3)
15:40:38 at internal/bootstrap/node.js:682:1
15:40:38 Emitted 'error' event at:
15:40:38 at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
15:40:38 at onErrorNT (internal/child_process.js:422:16)
15:40:38 [... lines matching original stack trace ...]
15:40:38 at internal/bootstrap/node.js:682:1
15:40:38 ...15:45:14 ok 65 parallel/test-child-process-spawn-argv0
15:45:14 ---
15:45:14 duration_ms: 0.596
15:45:14 ...
15:45:15 not ok 66 parallel/test-child-process-spawnsync-args
15:45:15 ---
15:45:15 duration_ms: 0.201
15:45:15 severity: fail
15:45:15 exitcode: 1
15:45:15 stack: |-
15:45:15 assert.js:86
15:45:15 throw new AssertionError(obj);
15:45:15 ^
15:45:15
15:45:15 AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
15:45:15 + actual - expected
15:45:15
15:45:15 + null
15:45:15 - Buffer [Uint8Array] []
15:45:15 at testCases.map (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-spawnsync-args.js:33:10)
15:45:15 at Array.map (<anonymous>)
15:45:15 at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-spawnsync-args.js:26:27)
15:45:15 at Module._compile (internal/modules/cjs/loader.js:718:30)
15:45:15 at Object.Module._extensions..js (internal/modules/cjs/loader.js:729:10)
15:45:15 at Module.load (internal/modules/cjs/loader.js:617:32)
15:45:15 at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
15:45:15 at Function.Module._load (internal/modules/cjs/loader.js:552:3)
15:45:15 at Function.Module.runMain (internal/modules/cjs/loader.js:771:12)
15:45:15 at executeUserCode (internal/bootstrap/node.js:317:15)
15:45:15 ... |
Sorry, something went wrong.
|
Fixed tests for Windows based systems |
Sorry, something went wrong.
Sorry, something went wrong.
|
Still seems to be failing in relevant ways on Windows: 14:32:33 not ok 64 parallel/test-child-process-spawn-args
14:32:33 ---
14:32:33 duration_ms: 0.250
14:32:33 severity: fail
14:32:33 exitcode: 1
14:32:33 stack: |-
14:32:33 events.js:174
14:32:33 throw er; // Unhandled 'error' event
14:32:33 ^
14:32:33
14:32:33 Error: spawn cd ENOENT
14:32:33 at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
14:32:33 at onErrorNT (internal/child_process.js:422:16)
14:32:33 at internalTickCallback (internal/process/next_tick.js:72:19)
14:32:33 at process._tickCallback (internal/process/next_tick.js:47:5)
14:32:33 at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
14:32:33 at executeUserCode (internal/bootstrap/node.js:318:15)
14:32:33 at startExecution (internal/bootstrap/node.js:252:3)
14:32:33 at startup (internal/bootstrap/node.js:203:3)
14:32:33 at internal/bootstrap/node.js:693:1
14:32:33 Emitted 'error' event at:
14:32:33 at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
14:32:33 at onErrorNT (internal/child_process.js:422:16)
14:32:33 [... lines matching original stack trace ...]
14:32:33 at internal/bootstrap/node.js:693:1
14:32:33 ...Do you have access to a Windows machine? If not, maybe someone from @nodejs/platform-windows can help. |
Sorry, something went wrong.
spawn method ignores 3-d argument 'options' in case the second one 'args' equals to 'undefined'. Fixes: nodejs#24912
|
@Trott no, I don't but that typo could be the case, let's check one more time |
Sorry, something went wrong.
Sorry, something went wrong.
spawn method ignores 3-d argument 'options' in case the second one 'args' equals to 'undefined'. Fixes: nodejs#24912 PR-URL: nodejs#24913 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
|
The tests added in this PR fail for me locally: $ ./node test/parallel/test-child-process-spawn-args.js
events.js:174
throw er; // Unhandled 'error' event
^
Error: spawn pwd ENOENT
at Process.ChildProcess._handle.onexit (internal/child_process.js:246:19)
at onErrorNT (internal/child_process.js:422:16)
at internalTickCallback (internal/process/next_tick.js:72:19)
at process._tickCallback (internal/process/next_tick.js:47:5)
at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)
at executeUserCode (internal/bootstrap/node.js:318:15)
at startExecution (internal/bootstrap/node.js:252:3)
at startup (internal/bootstrap/node.js:203:3)
at internal/bootstrap/node.js:693:1
Emitted 'error' event at:
at Process.ChildProcess._handle.onexit (internal/child_process.js:252:12)
at onErrorNT (internal/child_process.js:422:16)
[... lines matching original stack trace ...]
at internal/bootstrap/node.js:693:1
Am I missing something? |
Sorry, something went wrong.
python tools/test.py -J --mode=release test/parallel/test-child-process-spawn-args.jsDetails: https://github.com/nodejs/node/blob/master/BUILDING.md#running-tests |
Sorry, something went wrong.
|
@targos The test is missing a tmpdir.refresh() call. It doesn't (usually) affect things when run with the Python script because it assigns a tmpdir that is typically leftover from some other test, but when running from the command line, you may be less lucky. I'll open a PR to fix. |
Sorry, something went wrong.
Without `tmpdir.refresh()`, the test fails in some situations. This was missed because using `test.py` will almost always result in a leftover tmpdir lying around that makes the `refresh()` not needed. Refs: nodejs#24913 (comment)
|
FWIW I initially saw the tests fail with make test -j8 |
Sorry, something went wrong.
Without `tmpdir.refresh()`, the test fails in some situations. This was missed because using `test.py` will almost always result in a leftover tmpdir lying around that makes the `refresh()` not needed. Refs: nodejs#24913 (comment) PR-URL: nodejs#25098 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Without `tmpdir.refresh()`, the test fails in some situations. This was missed because using `test.py` will almost always result in a leftover tmpdir lying around that makes the `refresh()` not needed. Refs: #24913 (comment) PR-URL: #25098 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
spawn method ignores 3-d argument 'options' in case the second one 'args' equals to 'undefined'. Fixes: nodejs#24912 PR-URL: nodejs#24913 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Without `tmpdir.refresh()`, the test fails in some situations. This was missed because using `test.py` will almost always result in a leftover tmpdir lying around that makes the `refresh()` not needed. Refs: nodejs#24913 (comment) PR-URL: nodejs#25098 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Without `tmpdir.refresh()`, the test fails in some situations. This was missed because using `test.py` will almost always result in a leftover tmpdir lying around that makes the `refresh()` not needed. Refs: #24913 (comment) PR-URL: #25098 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
| Back | FazBrowse Home | New Git URL |
spawn method ignores 3-d argument 'options' in case
the second one 'args' equals to 'undefined'.
Fixes: #24912
Checklist