test: wrap assert.fail when passed to callback#3453
Uh oh!
There was an error while loading. Please reload this page.
Conversation
MylesBorins commented Oct 20, 2015
/cc @jasnell |
jasnell commented Oct 20, 2015
LGTM |
cjihrig commented Oct 20, 2015
LGTM pending CI |
Trott commented Oct 20, 2015
LGTM pending CI Speaking of which: https://ci.nodejs.org/job/node-test-pull-request/547/ |
Trott commented Oct 20, 2015
CI results breakdown: freebsd101-32 failure is a known flaky test that #3430 intends to fix. Three tests on ppcbe-fedora20 and one test on ppcle-ubuntu1404 (or did I put that backwards?) are all known fail-every-time tests on those platforms. (Probably should mark them as flaky for those platforms in win2008r2 vs2015 is build step explosion. win10 vs2015 is one test failure that was untouched by this PR and then a build step explosion. ubuntu1404-32 is a failure in a test untouched by this PR. In short: LGTM |
brendanashworth commented Oct 21, 2015
I don't have a specific concern with either this or #3378, but my thought regarding this particular pattern is that it is already in use in both core and user land. Would it be worth implementing this as an official API? It would be, I guess, Here is the illogical output for what I'd imagine are common paradigms: >assert.fail() AssertionError: undefinedundefinedundefinedat repl:1:8at...>assert.fail('uh oh!') AssertionError: 'uh oh!'undefinedundefined at repl:1:8at...>assert.fail(1e2,1e3,'not equal','!==') AssertionError: notequal at repl:1:8at...Yes, I'm aware people want to lock this as a module but this I think is a good reason why it shouldn't be done. This is already expected for the most part. This shouldn't hold this up though, just tangential. |
MylesBorins commented Oct 22, 2015
@jasnell is this ready to land? |
Trott commented Oct 23, 2015
@thealphanerd if you could amend the commit message so that the body wraps at 72 chars as specified in CONTRIBUTING.md, that will save James or whoever lands this that little step. Might as well rebase against current master while you're at it just to suss out any conflicts that may have arisen with commits from the last 2 days. |
Currently there are many instances where assert.fail is directly passed to a callback for error handling. Unfortunately this will swallow the error as it is the third argument of assert.fail that sets the message not the first. This commit adds a new function to test/common.js that simply wraps assert.fail and calls it with the provided message. Tip of the hat to @Trott for pointing me in the direction of this.
Trott commented Oct 24, 2015
Landed in 28e9a02 |
Currently there are many instances where assert.fail is directly passed to a callback for error handling. Unfortunately this will swallow the error as it is the third argument of assert.fail that sets the message not the first. This commit adds a new function to test/common.js that simply wraps assert.fail and calls it with the provided message. Tip of the hat to @Trott for pointing me in the direction of this. PR-URL: #3453 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Currently there are many instances where assert.fail is directly passed to a callback for error handling. Unfortunately this will swallow the error as it is the third argument of assert.fail that sets the message not the first. This commit adds a new function to test/common.js that simply wraps assert.fail and calls it with the provided message. Tip of the hat to @Trott for pointing me in the direction of this. PR-URL: #3453 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
jasnell commented Oct 26, 2015
Landed in v4.x-staging in 635f743 |
Currently there are many instances where assert.fail is directly passed to a callback for error handling. Unfortunately this will swallow the error as it is the third argument of assert.fail that sets the message not the first. This commit adds a new function to test/common.js that simply wraps assert.fail and calls it with the provided message. Tip of the hat to @Trott for pointing me in the direction of this. PR-URL: #3453 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Currently there are many instances where assert.fail is directly passed to a callback for error handling. Unfortunately this will swallow the error as it is the third argument of assert.fail that sets the message not the first. This commit adds a new function to test/common.js that simply wraps assert.fail and calls it with the provided message. Tip of the hat to @Trott for pointing me in the direction of this. PR-URL: #3453 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
6 participants
Currently there are many instances where assert.fail is directly passed
to a callback for error handling. Unfortunately this will swallow the error
as it is the third argument of assert.fail that sets the message not the first.
This commit adds a new function to test/common.js that simply wraps assert.fail
and calls it with the provided message.
Tip of the hat to @Trott for pointing me in the direction of this.