test: wrap assert.fail when passed to callback - #3453
Conversation
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
|
LGTM pending CI |
Sorry, something went wrong.
There was a problem hiding this comment.
find/replace error in this comment?
Sorry, something went wrong.
There was a problem hiding this comment.
fixed
Sorry, something went wrong.
|
LGTM pending CI Speaking of which: https://ci.nodejs.org/job/node-test-pull-request/547/ |
Sorry, something went wrong.
|
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 test/parallel/parallel.status.) 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 |
Sorry, something went wrong.
|
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, assert.fail(message) and assert.fail(val1, val2, operator). Currently the signature is assert.fail(val1, val2, message, operator) which is weird because message and { val1, val2, operator } are exclusive arguments, i.e. if message is falsy the rest are used, but if it is truthy the rest are ignored. Which leads to, logically, we should have two signatures instead of one. Here is the illogical output for what I'd imagine are common paradigms: > assert.fail()
AssertionError: undefined undefined undefined
at repl:1:8
at ...
> assert.fail('uh oh!')
AssertionError: 'uh oh!' undefined undefined
at repl:1:8
at ...
> assert.fail(1e2, 1e3, 'not equal', '!==')
AssertionError: not equal
at repl:1:8
at ...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. |
Sorry, something went wrong.
|
@jasnell is this ready to land? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
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.
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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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.