Uh oh!
There was an error while loading. Please reload this page.
assert: add warning about assert.doesNotReject - #19462
Conversation
The usefulness of `assert.doesNotReject` is very limited and this warns against the usage.
BridgeAR
commented
Mar 20, 2018
| Please note: Using `assert.doesNotReject()` is actually not useful because there | ||
| is little benefit by catching a rejection and then rejecting it again. Instead, | ||
| consider adding a comment next to the specific code path that should not reject | ||
| and keep error messages as expressive as possible. |
There was a problem hiding this comment.
Suggestions: Get rid of Please note: and keep the first sentence concise:
There is little benefit to using `assert.doesNotReject()`. Instead, consider adding a comment...There was a problem hiding this comment.
In this specific case I would rather keep the Please note as I really want users to read this. That is also why it is placed prominently. And the text is currently similar to the one used in doesNotThrow, so I thought I just keep them aligned.
|
BridgeAR
commented
Mar 20, 2018
@apapirovski I am not certain what the benefit of catching the rejection and rejecting it again is. Can you please elaborate? Your example is going to reject the same error that it caught without modifying it. |
apapirovski
commented
Mar 20, 2018
@BridgeAR B'ah. I forgot it was all handled async anyway. I stopped following the development of that feature at some point. (In my mind we were throwing on nextTick.) |
BridgeAR
commented
Mar 24, 2018
Landed in 5d6d1fe |
The usefulness of `assert.doesNotReject` is very limited and this warns against the usage. PR-URL: nodejs#19462 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos
commented
Apr 2, 2018
Should land in v9.x only if #18023 makes it. |
The usefulness of `assert.doesNotReject` is very limited and this warns against the usage. PR-URL: nodejs#19462 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
The usefulness of
assert.doesNotRejectis very limited and thiswarns against the usage.
This keeps it in line with
assert.doesNotThrow.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes