removed 3rd arg from assert.throws() and changed assert.strict to ass… - #22016
removed 3rd arg from assert.throws() and changed assert.strict to ass…#22016bhavayAnand9 wants to merge 3 commits into
Conversation
Sorry, something went wrong.
| } finally { | ||
| assert.strictEqual(ret, undefined, | ||
| `should not have a return value, received ${ret}`); | ||
| assert.strictEqual(caught, true, 'execSync should throw'); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Changed assert.ok(caught, true, 'execSync should throw');
to assert.ok(caught, 'execSync should throw');
Sorry, something went wrong.
| @@ -37,12 +37,10 @@ let caught = false; | |||
| assert.throws( | |||
| function() { execSync('exit -1', { shell: 'bad_shell' }); }, | |||
| /spawnSync bad_shell ENOENT/, | |||
There was a problem hiding this comment.
Can you remove the trailing comma here and below.
Sorry, something went wrong.
There was a problem hiding this comment.
Removed trailing commas.
Sorry, something went wrong.
cjihrig
left a comment
There was a problem hiding this comment.
LGTM once the comment regarding the strictEqual() change is addressed.
Sorry, something went wrong.
Sorry, something went wrong.
|
"Resumed" CI: https://ci.nodejs.org/job/node-test-pull-request/16076/ |
Sorry, something went wrong.
Improve assertions in test-child-process-execsync by removing unneeded third arguments and replacing equal checks with assert.ok() where appropriate. PR-URL: nodejs#22016 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
|
Thanks for the contribution! 🎉 (If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.) |
Sorry, something went wrong.
Improve assertions in test-child-process-execsync by removing unneeded third arguments and replacing equal checks with assert.ok() where appropriate. PR-URL: #22016 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
…ert.ok
Checklist