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: add mustCall() to child-process test #13605

Closed
wants to merge 1 commit into from
Closed

test: add mustCall() to child-process test #13605

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 11, 2017

Add common.mustCall() check to test-child-process-stdio-big-write-end.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test child_process

@Trott
test: add mustCall() to child-process test
2166ced 
Add common.mustCall() check to test-child-process-stdio-big-write-end.
@Trott Trott added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Jun 11, 2017
refack
refack reviewed Jun 11, 2017
View reviewed changes
test/parallel/test-child-process-stdio-big-write-end.js
@@ -43,10 +43,10 @@ function parent() {
child.stdout.on('data', function(c) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a mustCallAtLeast?
(I see that the value of n is asserted, but this will give a one-step-closer-to-root-cause assertion error)

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly, the assertion on n will occur before any assertion by mustCallAtLeast().

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.

refack
refack approved these changes Jun 11, 2017
View reviewed changes
test/parallel/test-child-process-stdio-big-write-end.js
@@ -43,10 +43,10 @@ function parent() {
child.stdout.on('data', function(c) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.

test/parallel/test-child-process-stdio-big-write-end.js
@@ -43,10 +43,10 @@ function parent() {
child.stdout.on('data', function(c) {
n += c;
});
child.stdout.on('end', function() {
child.stdout.on('end', common.mustCall(function() {
assert.strictEqual(+n, sent);
console.log('ok');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should remove (maybe replace with comment // end of test)

test/parallel/test-child-process-stdio-big-write-end.js
@@ -43,10 +43,10 @@ function parent() {
child.stdout.on('data', function(c) {
n += c;
});
child.stdout.on('end', function() {
child.stdout.on('end', common.mustCall(function() {
assert.strictEqual(+n, sent);
Copy link
Contributor

Choose a reason for hiding this comment

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

the + is cheating the strict
either put on the right side send + '\n' ('\n' is because data comes from console.log)
or replace the 'data' handler with:

  let n = 0;
  child.stdout.setEncoding('ascii');
  child.stdout.on('data', function(d) {
    const c = Number(d);
    if (!c) assert.fail(c);
    n += c;
  });

@Trott
Copy link
Member Author

Trott commented Jun 13, 2017

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

jasnell pushed a commit that referenced this pull request Jun 13, 2017
@Trott @jasnell
test: add mustCall() to child-process test
f308b4d 
Add common.mustCall() check to test-child-process-stdio-big-write-end.

PR-URL: #13605
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 13, 2017

Landed in f308b4d

@jasnell jasnell closed this Jun 13, 2017
addaleax pushed a commit that referenced this pull request Jun 17, 2017
@Trott @addaleax
test: add mustCall() to child-process test
1a8404c 
Add common.mustCall() check to test-child-process-stdio-big-write-end.

PR-URL: #13605
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 17, 2017
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@Trott @addaleax
test: add mustCall() to child-process test
7874360 
Add common.mustCall() check to test-child-process-stdio-big-write-end.

PR-URL: #13605
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@Trott @MylesBorins
test: add mustCall() to child-process test
6f44563 
Add common.mustCall() check to test-child-process-stdio-big-write-end.

PR-URL: #13605
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@Trott @MylesBorins
test: add mustCall() to child-process test
9356e96 
Add common.mustCall() check to test-child-process-stdio-big-write-end.

PR-URL: #13605
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
@beevelop beevelop mentioned this pull request Sep 6, 2017
Closed
@Trott Trott deleted the moar-mustCall 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

@hiroppy hiroppy hiroppy approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca 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.
8 participants
@Trott @jasnell @refack @lpinca @cjihrig @tniessen @hiroppy @MylesBorins