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: fix test-child-process-stdout-flush-exit #1868

Merged
merged 1 commit into from
Jun 2, 2015
Merged

test: fix test-child-process-stdout-flush-exit #1868

merged 1 commit into from
Jun 2, 2015

Conversation

santigimeno
Copy link
Member

Make sure all the stdout data is available before
performing the assertions.

Before this change, I was getting this error from time to time:

assert.js:88
  throw new assert.AssertionError({
        ^
AssertionError: false == true
    at ChildProcess.<anonymous> (/usr/home/sgimeno/node/io.js/test/parallel/test-child-process-stdout-flush-exit.js:47:5)
    at emitTwo (events.js:87:13)
    at ChildProcess.emit (events.js:172:7)
    at maybeClose (internal/child_process.js:763:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:210:5)

Make sure all the stdout data is available before
performing the assertions.
assert(gotBye);
child.on('close', function() {
assert.equal(stdout.slice(0, 6), 'hello\n');
assert.equal(stdout.slice(stdout.length - 8), 'goodbye\n');
});
Copy link
Member

Choose a reason for hiding this comment

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

Can you wrap the callback in a common.mustCall(...) while you're here? LGTM otherwise.

@santigimeno
Copy link
Member Author

@bnoordhuis done. Thanks

@bnoordhuis
Copy link
Member

Cheers. Let's see what the CI thinks: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/751/

@mscdex mscdex added the test Issues and PRs related to the tests. label Jun 2, 2015
@santigimeno
Copy link
Member Author

Probably related to #944

});
child.on('close', common.mustCall(function() {
assert.equal(stdout.slice(0, 6), 'hello\n');
assert.equal(stdout.slice(stdout.length - 8), 'goodbye\n');
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 would be better, no?

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't really matter for primitives. I'll land it, CI is happy.

@bnoordhuis bnoordhuis merged commit ca05ab8 into nodejs:master Jun 2, 2015
@bnoordhuis
Copy link
Member

Landed in 311c3e4, thanks.

@Fishrock123
Copy link
Contributor

(LGTM)

bnoordhuis pushed a commit that referenced this pull request Jun 2, 2015
Make sure all the stdout data is available before
performing the assertions.

Fixes: #944
PR-URL: #1868
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@bnoordhuis
Copy link
Member

Updated in b926718.

andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Make sure all the stdout data is available before
performing the assertions.

Fixes: nodejs/node#944
PR-URL: nodejs/node#1868
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@rvagg rvagg mentioned this pull request Jun 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants