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.

Sign up for GitHub

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

Jump to bottom

test: refactor test-child-process-send-type-error.js #13904

Closed
wants to merge 1 commit into from
Closed

test: refactor test-child-process-send-type-error.js #13904

wants to merge 1 commit into from

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

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

cjihrig
cjihrig requested changes Jun 25, 2017
View reviewed changes
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

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

cjihrig
cjihrig approved these changes Jun 29, 2017
View reviewed changes
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.

test/parallel/test-child-process-send-type-error.js Outdated
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);?

refack
refack approved these changes Jun 29, 2017
View reviewed changes
test/parallel/test-child-process-send-type-error.js Outdated
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);?

@Trott
test: refactor test-child-process-send-type-error.js
c874808 
* 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
@Trott
test: refactor test-child-process-send-type-error
93683c6 
* 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
@Trott @addaleax
test: refactor test-child-process-send-type-error
2e860ff 
* 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
Merged
addaleax pushed a commit that referenced this pull request Jun 30, 2017
@Trott @addaleax
test: refactor test-child-process-send-type-error
77299d6 
* 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
@Trott @addaleax
test: refactor test-child-process-send-type-error
2307986 
* 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
@Trott @addaleax
test: refactor test-child-process-send-type-error
071ecb0 
* 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
@Trott @MylesBorins
test: refactor test-child-process-send-type-error
f9096a7 
* 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
@Trott @MylesBorins
test: refactor test-child-process-send-type-error
3cae68a 
* 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
@Trott @MylesBorins
test: refactor test-child-process-send-type-error
97ad485 
* 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
@Trott @MylesBorins
test: refactor test-child-process-send-type-error
bd8574c 
* 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
Merged
This was referenced Sep 5, 2017
Closed
Closed
Closed
This was referenced Sep 6, 2017
Closed
Closed
@Trott Trott deleted the refactor-test-child-process-send-type-error branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Trott @refack @jasnell @cjihrig @MylesBorins