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 test-child-process-send-type-error.js #13904

Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 25, 2017

  • Do not run tests in child process as their failure will not cause the
    test to fail. Only run the tests in the parent process where their
    failure means the test fails.
  • Use common.mustNotCall() to guarantee callback is not invoked.
  • Insert blank line per test writing guide.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test child_process

@Trott Trott added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Jun 25, 2017
@Trott
Copy link
Member Author

Trott commented Jun 25, 2017

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to run the test in the child process to make sure child.send() and process.send() behave the same.

Can I suggest adding an exit event handler in the parent to make sure the child process exited normally. That seems to fix the problem on my machine.

@Trott Trott force-pushed the refactor-test-child-process-send-type-error branch from e810393 to a575bbf Compare June 29, 2017 02:56
@Trott
Copy link
Member Author

Trott commented Jun 29, 2017

@cjihrig Reworked to address your comment. PTAL

@Trott
Copy link
Member Author

Trott commented Jun 29, 2017

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one suggestion.

target = cp.fork(__filename, ['child']);
target.on('exit', (code) => { assert.strictEqual(code, 0); });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.mustCall()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.strictEqual(signal, null);?

target = cp.fork(__filename, ['child']);
target.on('exit', (code) => { assert.strictEqual(code, 0); });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.strictEqual(signal, null);?

* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.
@Trott Trott force-pushed the refactor-test-child-process-send-type-error branch from a575bbf to c874808 Compare June 29, 2017 22:42
@Trott
Copy link
Member Author

Trott commented Jun 29, 2017

Addressed further nits. CI: https://ci.nodejs.org/job/node-test-pull-request/8880/

@Trott
Copy link
Member Author

Trott commented Jun 30, 2017

Landed in 93683c6

@Trott Trott closed this Jun 30, 2017
Trott added a commit to Trott/io.js that referenced this pull request Jun 30, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: nodejs#13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 30, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: #13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Jun 30, 2017
addaleax pushed a commit that referenced this pull request Jun 30, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: #13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: #13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: #13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: #13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: #13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 15, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: #13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
* Add exit listener to child process to check return code. Previously,
  child process faiilure would not cause the test to fail.
* Use common.mustNotCall() to guarantee callback is not invoked.
* Insert blank line per test writing guide.

PR-URL: #13904
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
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