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

Improve parallel test child process silent #15938

Closed
wants to merge 3 commits into from
Closed

Improve parallel test child process silent #15938

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18feb2f
test: fix typo
osukaa Oct 6, 2017
4e72bec
test: fix stderr reference
osukaa Oct 6, 2017
dda7831
test: remove literal messages to only use assert default message
osukaa Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions test/parallel/test-child-process-silent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ if (process.argv[2] === 'pipe') {
stdoutData = true;
});
let stderrData = false;
parent.stdout.on('data', function() {
parent.stderr.on('data', function() {
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why this was changed?

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR the original seems wrong as there is already a listener for stdout above and this should actually work on stderr.

stderrData = true;
});

Expand All @@ -74,17 +74,17 @@ if (process.argv[2] === 'pipe') {
child.stdout.pipe(process.stdout, { end: false });

let childSending = false;
let childReciveing = false;
let childReceiving = false;
child.on('message', function(message) {
if (childSending === false) {
childSending = (message === 'message from child');
}

if (childReciveing === false) {
childReciveing = (message === 'got message from master');
if (childReceiving === false) {
childReceiving = (message === 'got message from master');
}

if (childReciveing === true) {
if (childReceiving === true) {
child.kill();
}
});
Expand All @@ -97,11 +97,11 @@ if (process.argv[2] === 'pipe') {
parent.kill();

// Check std(out|err) pipes
assert.ok(!stdoutData, 'The stdout socket was piped to parent');
assert.ok(!stderrData, 'The stderr socket was piped to parent');
assert.ok(!stdoutData);
assert.ok(!stderrData);

// Check message system
assert.ok(childSending, 'The child was able to send a message');
assert.ok(childReciveing, 'The child was able to receive a message');
assert.ok(childSending);
assert.ok(childReceiving);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of just removing this it would probably be best to either change it to assert.strictEqual or to include the value itself in the error message. But now it would not be known what the value is.

});
}