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

removed 3rd arg from assert.throws() and changed assert.strict to ass… #22016

Closed
wants to merge 3 commits into from
Closed

removed 3rd arg from assert.throws() and changed assert.strict to ass… #22016

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a31ed2
removed 3rd arg from assert.throws() and changed assert.strict to ass…
bhavayAnand9 Jul 27, 2018
45f5442
Update test-child-process-execsync.js
bhavayAnand9 Jul 28, 2018
b073a4a
Update test-child-process-execsync.js
bhavayAnand9 Jul 29, 2018
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
4 changes: 1 addition & 3 deletions test/sequential/test-child-process-execsync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,10 @@ let caught = false;
assert.throws(
function() { execSync('exit -1', { shell: 'bad_shell' }); },
/spawnSync bad_shell ENOENT/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the trailing comma here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed trailing commas.

'execSync did not throw the expected exception!'
);
assert.throws(
function() { execFileSync('exit -1', { shell: 'bad_shell' }); },
/spawnSync bad_shell ENOENT/,
'execFileSync did not throw the expected exception!'
);

let cmd, ret;
Expand All @@ -56,7 +54,7 @@ try {
} finally {
assert.strictEqual(ret, undefined,
`should not have a return value, received ${ret}`);
assert.strictEqual(caught, true, 'execSync should throw');
Copy link
Member

Choose a reason for hiding this comment

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

This should not be changed. 1. caught should be a boolean and ok does a loose equal check. 2. The second argument would now be the error message and the third argument would be ignored.

Copy link
Member

@Trott Trott Jul 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, this should definitely be changed, but not the way it is in this PR right now.

There are at least two options.

Since caught is initialized to false and only ever changed to true inside of code that is in this file, strictEqual() is unnecessary. So this should be fine:

assert.ok(caught, 'execSync should throw');

Alternatively, if you want to insist on strictEqual() for the type-checking aspect, that's fine but then the string literal third argument should be removed (and possibly moved to a comment above the call) because you care about whether the test failed because caught was false or caught was the string "true" instead of the boolean true, etc.

// execSync should throw
assert.strictEqual(caught, true);

I think either of these options are better than what's there now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed assert.ok(caught, true, 'execSync should throw');
to assert.ok(caught, 'execSync should throw');

assert.ok(caught, true, 'execSync should throw');
const end = Date.now() - start;
assert(end < SLEEP);
assert(err.status > 128 || err.signal);
Expand Down