Skip to content

test: make test-process-argv-0 robust #2541

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

Closed
Trott wants to merge 2 commits into from
Closed

test: make test-process-argv-0 robust #2541

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae77774
test: make test-process-argv-0 robust
Trott Aug 25, 2015
9044490
test: use process exit event rather than child exit event
Trott Aug 25, 2015
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
Next Next commit
test: make test-process-argv-0 robust 
Remove use of STDERR to avoid test flakiness on CentOS.

Fixes: #2477
  • Loading branch information
@Trott
Trott committed Aug 25, 2015
commit ae77774da9838a6b4bf66eb4922a2d719ae47812
10 changes: 0 additions & 10 deletions test/parallel/test-process-argv-0.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,18 @@
'use strict';
var util = require('util');
var path = require('path');
var assert = require('assert');
var spawn = require('child_process').spawn;
var common = require('../common');

console.error('argv=%j', process.argv);
console.error('exec=%j', process.execPath);

if (process.argv[2] !== 'child') {
var child = spawn(process.execPath, [__filename, 'child'], {
cwd: path.dirname(process.execPath)
});

var childArgv0 = '';
var childErr = '';
child.stdout.on('data', function(chunk) {
childArgv0 += chunk;
});
child.stderr.on('data', function(chunk) {
childErr += chunk;
});
child.on('exit', function() {
Copy link
Copy Markdown
Contributor

@thefourtheye thefourtheye Aug 25, 2015

Choose a reason for hiding this comment

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

common.mustCall

Copy link
Copy Markdown
Member Author

@Trott Trott Aug 25, 2015

Choose a reason for hiding this comment

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

I don't see how common.mustCall() can be used to replace the assertion here. Can you explain a bit more what you mean?

Copy link
Copy Markdown
Contributor

@thefourtheye thefourtheye Aug 25, 2015

Choose a reason for hiding this comment

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

Actually, common.mustCall makes sure that the exit handler is called for sure. Because the value is asserted only inside it. If the handler is never called, the test will still pass.

Copy link
Copy Markdown
Member Author

@Trott Trott Aug 25, 2015

Choose a reason for hiding this comment

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

Ah, because exit may not fire on the child process if there's an error. Good catch.

Copy link
Copy Markdown
Member

@targos targos Aug 25, 2015

Choose a reason for hiding this comment

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

common.mustCall uses process.exit internally. It is not safer than what is done here

Copy link
Copy Markdown
Member Author

@Trott Trott Aug 25, 2015

Choose a reason for hiding this comment

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

I was actually just going to just change child.on('exit', ...) to process.on('exit', ...) rather than use common.mustCall() for two reasons:

  • I think it's clearer what is going on that way.
  • This test does not load common.js to begin with because it doesn't need it. Loading the entire thing just for one use of common.mustCall() seems like overkill when process.on('exit', ...) will work just fine.

You (@targos) may very well be correct that child.on('exit', ...) and process.on('exit', ...) have the exact same behavior. But, at least according to the docs, the exit event on a ChildProcess might not fire. I can't find any similar warning in the docs on the exit event for process. So I'm inclined to go with process. If the exit event might not fire, that might be an issue to file against the docs. The text describing beforeExit seems to imply that exit can be depended on to fire for process.

Copy link
Copy Markdown
Member

@targos targos Aug 26, 2015

Choose a reason for hiding this comment

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

My bad, I misread the line as process.on('exit', ...).

console.error('CHILD: %s', childErr.trim().split('\n').join('\nCHILD: '));
assert.equal(childArgv0, process.execPath);
});
}
Expand Down