assert: detect faulty throws usage #19867

BridgeAR · 2018-04-07T13:06:12Z

This is a attempt in fixing the ambiguous behavior of the assert.throws message. It is the best way to detect it out of my perspective. This ambiguity is the only real problem with the current API. It would of course be nicer if this would have always tested for the error message but changing that is not possible because quite some code will rely on the current behavior.

~~The PR currently relies on #19865 to prevent conflicts after landing that PR. I am going to remove that commit as soon as that lands.~~ Done.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

BridgeAR · 2018-04-07T13:06:36Z

CI https://ci.nodejs.org/job/node-test-pull-request/14121/

vsemozhetbyt · 2018-04-07T13:35:34Z

doc/api/errors.md

@@ -599,6 +599,14 @@ A special type of error that can be triggered whenever Node.js detects an
 exceptional logic violation that should never occur. These are raised typically
 by the `assert` module.

+<a id="ERR_AMBIGUOUS_ARGUMENT"></a>
+### ERR_AMBIGUOUS_ARGUMENT


Should it go before the ERR_ARG_NOT_ITERABLE ABC-wise?

vsemozhetbyt · 2018-04-07T13:36:00Z

doc/api/errors.md

+
+This is triggered by the `assert` module in case e.g.,
+`assert.throws(fn, message)` is used in a way that the message is the thrown
+error message. This is a ambiguous because the message is not verifying the


a ambiguous -> ambiguous

Trott · 2018-04-08T03:06:31Z

I'd be interested to have feedback from ecosystem testing framework maintainers on this. Not sure how to solicit that feedback, though. I guess a CITGM run would be a first step though.

BridgeAR · 2018-04-08T15:50:50Z

Comments addressed.

CI: https://ci.nodejs.org/job/node-test-pull-request/14127/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1357/

BridgeAR · 2018-04-09T02:43:07Z

@Trott it seems like the CITGM came out green.

Trott · 2018-04-09T04:08:59Z

@nodejs/testing (because: assert)
@nodejs/tsc (because: semver-major)

targos · 2018-04-09T04:34:42Z

The koa failure in CITGM is related to this change

BridgeAR · 2018-04-09T21:46:59Z

@targos thanks, I missed that (Note to myself: always run master to compare...). I am looking into it.

BridgeAR · 2018-04-09T21:50:20Z

It seems like this PR detected bugs in the koa tests :-). The message was used wrongly there and that resulted in the errors.

One of the biggest downsides to the `assert.throws` API is that it does not check for the error message in case that is used as second argument. It will instead be used in case no error is thrown. This improves the situation by checking the actual error message against the provided one and throws an error in case they are identical. It is very unlikely that the user wants to use that error message as information instead of checking against that message.

BridgeAR · 2018-04-10T02:08:23Z

Rebased due to conflicts.

New CI https://ci.nodejs.org/job/node-test-pull-request/14167/

BridgeAR · 2018-04-10T02:46:53Z

Ok, the tests in koa are fixed and I also opened a PR for coffeescript as that also came up when I looked closer.

BridgeAR · 2018-04-10T16:24:38Z

@nodejs/testing PTAL. This PR already detected two modules that wrongly used the error argument and I would like to get this in.

BridgeAR · 2018-04-10T16:31:45Z

Fixed a edge case that I just saw by looking at this again.

New CI https://ci.nodejs.org/job/node-test-pull-request/14181/

mcollina

LGTM

Trott · 2018-04-12T21:24:24Z

doc/api/errors.md

+This is triggered by the `assert` module in case e.g.,
+`assert.throws(fn, message)` is used in a way that the message is the thrown
+error message. This is ambiguous because the message is not verifying the error
+message and will only be thrown in case no error is thrown.


Non-blocking but if someone can help tighten this text up a bit, that would be good. @nodejs/documentation It communicates what it needs to, but there is room to make it a bit more clear I think. Doesn't have to happen in this PR (or at all). It could always happen at a later date.

BridgeAR · 2018-04-13T17:57:29Z

Landed in a25f567

One of the biggest downsides to the `assert.throws` API is that it does not check for the error message in case that is used as second argument. It will instead be used in case no error is thrown. This improves the situation by checking the actual error message against the provided one and throws an error in case they are identical. It is very unlikely that the user wants to use that error message as information instead of checking against that message. PR-URL: nodejs#19867 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

One of the biggest downsides to the `assert.throws` API is that it does not check for the error message in case that is used as second argument. It will instead be used in case no error is thrown. This improves the situation by checking the actual error message against the provided one and throws an error in case they are identical. It is very unlikely that the user wants to use that error message as information instead of checking against that message. PR-URL: #19867 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

The previous use of strings instead of regular expressions caused the assertion to treat the message as an additional output string should the test function fail to throw. Switching to RegExp causes the assertion to instead validate the error message itself. See nodejs/node#19867

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 7, 2018

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Apr 7, 2018

vsemozhetbyt reviewed Apr 7, 2018

View reviewed changes

BridgeAR force-pushed the improve-wrong-throws-usage branch from 47e3255 to 566f6dd Compare April 10, 2018 02:07

BridgeAR requested a review from a team April 10, 2018 02:08

This was referenced Apr 10, 2018

Fix tests koajs/koa#1170

Merged

Fix: a test relied on faulty behavior jashkenas/coffeescript#5028

Merged

fixup: fix edge case

0f0b79e

targos approved these changes Apr 12, 2018

View reviewed changes

BridgeAR requested a review from a team April 12, 2018 13:28

mcollina approved these changes Apr 12, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2018

BridgeAR requested a review from a team April 12, 2018 14:53

jasnell approved these changes Apr 12, 2018

View reviewed changes

Trott reviewed Apr 12, 2018

View reviewed changes

BridgeAR closed this Apr 13, 2018

sharwell mentioned this pull request Nov 25, 2018

Fix failure to validate exception messages tunnelvisionlabs/antlr4ts#379

Merged

BridgeAR deleted the improve-wrong-throws-usage branch April 1, 2019 23:40

Uh oh!

assert: detect faulty throws usage #19867

assert: detect faulty throws usage #19867

Uh oh!

Conversation

BridgeAR commented Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

BridgeAR commented Apr 7, 2018

Uh oh!

vsemozhetbyt Apr 7, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 7, 2018

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 8, 2018

Uh oh!

BridgeAR commented Apr 8, 2018

Uh oh!

BridgeAR commented Apr 9, 2018

Uh oh!

Trott commented Apr 9, 2018

Uh oh!

targos commented Apr 9, 2018

Uh oh!

BridgeAR commented Apr 9, 2018

Uh oh!

BridgeAR commented Apr 9, 2018

Uh oh!

BridgeAR commented Apr 10, 2018

Uh oh!

BridgeAR commented Apr 10, 2018

Uh oh!

BridgeAR commented Apr 10, 2018

Uh oh!

BridgeAR commented Apr 10, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Trott Apr 12, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 13, 2018

Uh oh!

Uh oh!

BridgeAR commented Apr 7, 2018 •

edited

Loading