removed 3rd arg from assert.throws() and changed assert.strict to ass… #22016
Conversation
| @@ -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'); | |||
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.
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.
There was a problem hiding this comment.
Changed assert.ok(caught, true, 'execSync should throw');
to assert.ok(caught, 'execSync should throw');
| @@ -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.
There was a problem hiding this comment.
Removed trailing commas.
Trott
added
the
author ready
|
"Resumed" CI: https://ci.nodejs.org/job/node-test-pull-request/16076/ |
|
Landed in ede279c |
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/.) |
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
make -j4 test(UNIX), orvcbuild test(Windows) passes