test: refactor switch

[Trott]

test-child-process-fork-net2.js has a switch statement with 6 cases.
Each case uses child.send(), passing an object for the callback.
child.send() ignores the callback because it is not a function.
Removing the unused argument.

[Trott]

OK, updated to just remove confusing unused object from function calls.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/1379/

[Trott]

One buildbot failure but otherwise looks good. /cc @nodejs/testing (looking for an LGTM or two)

[santigimeno]

LGTM

[orangemocha]

LGTM

Shouldn't send check the type of callback right away?

[Trott]

@orangemocha You'd think so, but it doesn't. Not sure if that's a feature or a bug. Here's the current relevant code from lib/internal/child_process.js:

  target.send = function(message, handle, callback) {
    if (typeof handle === 'function') {
      callback = handle;
      handle = undefined;
    }
    if (this.connected) {
      return this._send(message, handle, false, callback);
    }
    const ex = new Error('channel closed');
    if (typeof callback === 'function') {
      process.nextTick(callback, ex);
    } else {
      this.emit('error', ex);  // FIXME(bnoordhuis) Defer to next tick.
    }
    return false;
  };

So, if the connected property is not set, it will emit an error event if callback is not a function. That's as expected, sort of. I mean, I expect that behavior if I don't send a callback at all (i.e., callback is null or undefined). But I would expect a TypeError if callback is a string or a (non-null) object.

But if it is connected, then it passes callback (whether it is a function or not) on to _send(). And, if the message is successfully sent, it does this:

        if (typeof callback === 'function')
          callback(null);

It will call the callback on success if the callback is a function, but ignore it otherwise.

Again, not sure if this is a bug or a feature (or me misunderstanding how the code works--always a possibility).

[Trott]

@orangemocha I should add that if _send() encounters an error, it basically does the same thing as send(): Calls the callback with the error if the callback is a function, otherwise emits an error event. The arguably surprising part is that send() and _send() treat various non-function values (e.g., strings, objects) the same as values that would indicate no callback parameter at all. Least astonishment principle would seem to indicate an error (or at least a warning?) occur if you send a string or an object instead of a function for the callback parameter. I would expect a TypeError.

[Trott]

Green! \o/ https://ci.nodejs.org/job/node-test-commit/1938/

[Trott]

Landed in 0351b2f

[jasnell]

Added the lts-watch-v4.x label.

[MylesBorins]

@Trott this commit is breaking the unit test suite on v4.x-staging

Would you be open to backporting this and opening a new PR so we can test and what not

[Trott]

@thealphanerd Odd. This change should be a no-op. When you say "unit test suite", what is that?

[MylesBorins]

never mind.. there is a new flaky test on 4.x... I'll land this now

[MylesBorins]

Path: parallel/test-http-destroyed-socket-write2
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: false == true
    at test (/Users/thealphanerd/code/node-v4/test/parallel/test-http-destroyed-socket-write2.js:89:5)
    at Immediate.write [as _onImmediate] (/Users/thealphanerd/code/node-v4/test/parallel/test-http-destroyed-socket-write2.js:29:7)
    at processImmediate [as _immediateCallback] (timers.js:383:17)

Here's the error we are getting now

[Trott]

@thealphanerd I've not seen that before. (It's almost definitely unrelated to this PR.) Is that OS X? Are you running make test or doing something else?

[MylesBorins]

happening locally on osx when running make-test.

Once I have this RC ready to cut I'm going to stress test that one

[santigimeno]

I'm pretty sure I've seen that one on OS X too

[santigimeno]

Oh, I sent a PR some time ago for this: #4970