Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: refactor switch #4870

Closed
wants to merge 1 commit into from
Closed

test: refactor switch #4870

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 26, 2016

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 Trott added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Jan 26, 2016
@Trott Trott closed this Jan 26, 2016
`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.

PR-URL: nodejs#4870
@Trott Trott reopened this Jan 26, 2016
@Trott Trott changed the title test: refactor unnecessary switch test: refactor switch Jan 26, 2016
@Trott
Copy link
Member Author

Trott commented Jan 26, 2016

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

@Trott
Copy link
Member Author

Trott commented Jan 26, 2016

@Trott
Copy link
Member Author

Trott commented Jan 27, 2016

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

@santigimeno
Copy link
Member

LGTM

@orangemocha
Copy link
Contributor

LGTM

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

@Trott
Copy link
Member Author

Trott commented Jan 27, 2016

@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
Copy link
Member Author

Trott commented Jan 27, 2016

@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
Copy link
Member Author

Trott commented Jan 27, 2016

Trott added a commit to Trott/io.js that referenced this pull request Jan 27, 2016
`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.

PR-URL: nodejs#4870
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@Trott
Copy link
Member Author

Trott commented Jan 27, 2016

Landed in 0351b2f

@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

Added the lts-watch-v4.x label.

rvagg pushed a commit that referenced this pull request Jan 28, 2016
`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.

PR-URL: #4870
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
rvagg pushed a commit that referenced this pull request Feb 8, 2016
`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.

PR-URL: #4870
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@MylesBorins
Copy link
Contributor

@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
Copy link
Member Author

Trott commented Feb 17, 2016

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

@MylesBorins
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
`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.

PR-URL: #4870
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@Trott
Copy link
Member Author

Trott commented Feb 17, 2016

@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
Copy link
Contributor

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
Copy link
Member

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

@santigimeno
Copy link
Member

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

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
`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.

PR-URL: #4870
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
`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.

PR-URL: #4870
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
`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.

PR-URL: nodejs#4870
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@Trott Trott deleted the refactor-switch branch January 13, 2022 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants