Feature/fix child process type checking

[sam-github]

#8392 introduced a test that doesn't fail when the code it is testing is deleted.

Also, behaviour of execFile and spawn have drifted apart again, unnecessarily, I think. This brings them into line again.

/to @cjihrig

[cjihrig]

Even before my changes, I think this test was a bit overkill. It intentionally ran a bad command and passed in bad arguments to make sure that the TypeError was thrown, and the invalid command error wasn't encountered. A simple assert.throws() like you've done would have been simpler. I'm fine with getting rid of this code.

Would you mind removing the the errors variable and the process.on('exit', ...)?

[sam-github]

Not at all, I will do.

[sam-github]

@tjfontaine @cjihrig PTAL

[cjihrig]

I think this could cause problems in execFile() since arguments[1] could be a function too. Maybe throw in a check for arguments[1] !== callback

[trevnorris]

Correct. The callback is not being allowed to bubble down. This'll need to be fixed, but more than just adding a check. The values need to be assigned in that specific case. e.g.:

} else if (typeof arguments[1] === 'function') {
  callback = arguments[1];
  args = [];
} else if (arguments[1] && typeof arguments[1] !== 'object') {

Though this does leave the possibility of the user passing 0 to arguments[1] and still not throwing. Instead I'd use the following:

} else if (arguments[1] && typeof arguments[1] === 'object') {
  args = [];
  options = util._extend(options, arguments[1]);
} else {
  throw new TypeError('Incorrect value of args option');
}

[sam-github]

I popped off the rename, 939c897, because it makes addressing comments hard without rebasing them away, I'll add it back after review.

[sam-github]

Addressed comments (and incorporated #8530).
PTAL.

[trevnorris]

Applied this to v0.10. Here are some of the following errors:

=== release test-child-process-exec-error ===                          
Path: simple/test-child-process-exec-error
assert.js:92
  throw new assert.AssertionError({
        ^
AssertionError: 0 == 1
    at process.<anonymous> (/var/projects/node/test/simple/test-child-process-exec-error.js:35:12)
    at process.emit (events.js:117:20)
Command: out/Release/node /var/projects/node/test/simple/test-child-process-exec-error.js
=== release test-child-process-exit-code ===                    
Path: simple/test-child-process-exit-code
assert.js:92
  throw new assert.AssertionError({
        ^
AssertionError: false == true
    at ChildProcess.<anonymous> (/var/projects/node/test/simple/test-child-process-exit-code.js:44:10)
    at ChildProcess.emit (events.js:98:17)
    at Process.ChildProcess._handle.onexit (child_process.js:814:12)
Command: out/Release/node /var/projects/node/test/simple/test-child-process-exit-code.js
=== release test-error-reporting ===                                  
Path: simple/test-error-reporting
assert.js:92
  throw new assert.AssertionError({
        ^
AssertionError: 3 == 0
    at process.<anonymous> (/var/projects/node/test/simple/test-error-reporting.js:75:10)
    at process.emit (events.js:117:20)
Command: out/Release/node /var/projects/node/test/simple/test-error-reporting.js

[sam-github]

Sorry, @trevnorris, those tests pass for me on Linux, what's your platform?

And what did you mean by "applied to v0.10"? This PR is off the current head of v0.10... did you mean you applied it to v0.12? It doesn't apply clean to that branch.

I could rebase it onto it, and re-PR, of course. There are 3 branches in play, its hard to know where to direct PRs. I could even do one PR to v0.10, and another to v0.12, so that fixing the conflicts would be my problem, but I'm not really sure how helpful that would be.

[sam-github]

@trevnorris ping

[trevnorris]

@sam-github Thanks for the ping. The mention of applying it to v0.10 was meant to say it was applied to the tip of v0.10, since the PR hadn't been rebased on the latest. There's no need to re-PR for just rebasing on latest v0.10. Just rebase and force push your branch.

I'm rerunning the tests and will update with the results. My platform is Linux 3.16.0-24-generic #32-Ubuntu

[trevnorris]

The tests are still failing. Here's a strange one. Apply the following patch:

diff --git a/test/simple/test-child-process-exec-error.js b/test/simple/test-child-process-exec-error.js
index 2afe114..1f9d6f5 100644
--- a/test/simple/test-child-process-exec-error.js
+++ b/test/simple/test-child-process-exec-error.js
@@ -29,9 +29,11 @@ function test(fun, code) {
   fun('does-not-exist', function(err) {
     assert.equal(err.code, code);
     errors++;
+    console.log('>>> fun executed', process.pid, errors);
   });

   process.on('exit', function() {
+    console.log('>>> process exiting', process.pid, errors);
     assert.equal(errors, 1);
   });
 }

And the output I get is:

>>> fun executed 22943 1
>>> process exiting 22943 0

assert.js:93
  throw new assert.AssertionError({
        ^
AssertionError: 0 == 1
    at process.<anonymous> (/var/projects/node/test/simple/test-child-process-exec-error.js:37:12)
    at process.emit (events.js:117:20)

But if I move the var error = 0 outside of function test() then the output is:

>>> fun executed 22989 1
>>> process exiting 22989 1
>>> process exiting 22989 1

Any ideas what's going on?

[sam-github]

@trevnorris yes, I've figured out the problem, I'll see if I can fix it. I think checking types of args may not be possible for functions that have 4 args, 3 of which are optional.

[sam-github]

@trevnorris fixed, and added regression tests. I wasn't dealing with unsigned/null as explicit "no option" place holders in the postitional list, and exec uses this feature when it calls execFile. Thanks for catching that.

[trevnorris]

Awesome work. I'm getting a timeout on test-cluster-dgram-1.js, but double checking that's not actually part of your code (not sure how it could be). Also there are a couple JS style issues (can see those w/ make jslint) but I can fix those up when I squash and land.

[trevnorris]

Thanks much. Landed in 13a992b1, 2ff29cc, e17c5a7, 70dafa7 and 8032a21.

[sam-github]

@trevnorris I'll make sure to run jshint more regularly, sorry about that.