lib: handle throw undefined in assert.throws()#18029
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
And make `assert.doesNotThrow()` handle it as well. Fixes: nodejs#18027
nodejs-github-bot added the assert 
addaleax approved these changes Jan 7, 2018
Member
addaleax left a comment
addaleax left a comment There was a problem hiding this comment.
LGTM
Maybe start the commit message with assert:? :)
BridgeAR approved these changes Jan 7, 2018
Member
BridgeAR left a comment
BridgeAR left a comment There was a problem hiding this comment.
LGTM but since this was never documented and never worked before, we might want to add a entry in the changed history part in the documentation.
XadillaX approved these changes Jan 8, 2018
TimothyGu approved these changes Jan 8, 2018
| constassert=module.exports=ok; | ||
| constNO_EXCEPTION_SENTINEL={}; |
MemberAuthor
There was a problem hiding this comment.
It's merged now but for posterity: yes, but it doesn't add anything extra and it uses more memory than a plain empty object literal.
danbev approved these changes Jan 8, 2018
cjihrig approved these changes Jan 8, 2018
Contributor
cjihrig left a comment
cjihrig left a comment There was a problem hiding this comment.
LGTM. I like the idea of using a symbol for the sentinel.
jasnell approved these changes Jan 8, 2018
Member
jasnell left a comment
jasnell left a comment There was a problem hiding this comment.
LGTM but also prefer a Symbol for the sentinel
Member
jasnell commented Jan 9, 2018
Member
jasnell commented Jan 10, 2018
Landing this. Can switch the sentinel to a symbol in a separate PR later. |
jasnell pushed a commit that referenced this pull request Jan 10, 2018
And make `assert.doesNotThrow()` handle it as well. PR-URL: #18029Fixes: #18027 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Member
jasnell commented Jan 10, 2018
Landed in 71203f5 |
Contributor
evanlucas commented Jan 30, 2018
This is not cherry-picking cleanly to v9.x-staging. If you would like for it to land there, please submit a backport pr. Thanks! |
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
And make `assert.doesNotThrow()` handle it as well. PR-URL: nodejs#18029Fixes: nodejs#18027 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
4 tasks
Member
BridgeAR commented Mar 8, 2018
Backport opened in #19230 |
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
And make `assert.doesNotThrow()` handle it as well. Backport-PR-URL: #19230 PR-URL: #18029Fixes: #18027 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
And make `assert.doesNotThrow()` handle it as well. Backport-PR-URL: #19230 PR-URL: #18029Fixes: #18027 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Contributor
MylesBorins commented Jul 31, 2018
Requested backport to 8.x in #19230 |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
And make
assert.doesNotThrow()handle it as well.Fixes: #18027