assert: improve assert.throws by BridgeAR · Pull Request #17585 · nodejs/node · GitHub

Throw a TypeError in case a error message is provided in the second
argument and a third argument is present as well.
This is clearly a mistake and should not be done.

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

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

Affected core subsystem(s)

assert

The text above this example needs to be re-written now as well as the example on line 778 as well. Rather than moving the third argument to a comment, perhaps just remove it entirely. The user should still be cautioned against using a string for the expected error message because a string will be interpreted as the message to provide if the function doesn't throw rather than something to validate the thrown error.

I am not sure I can follow. The situation was only improved in case three arguments are passed to throws. Otherwise the problem continues to exist. So the mitigation is only very tiny as the typical wrong use case is probably assert.throws(fn, "error msg").

I thought a few times about how to improve the two args situation but as far as I see it there is nothing that can be done to improve it. I actually also thought about deprecating using a string as second argument but that would be very inconsistent and weird as well.

Refining my comment a bit: The current version of the example code (assert.throws(fn, str, str)) is obviously an error. The version above in this PR (assert.throws(fn, str)) is less obviously an error because maybe "missing foo" is not an expected error message but rather the error message we want to provide if the function doesn't throw as expected. It's not clear to me if the best way to clarify is to edit the text preceding the block or if it is to edit missing foo to say something else.

LGTM with doc comment addressed.

Agree with @Trott abotu the doc being a little confusing now.

I updated the example and tried to be as detailed and precise as possible.

While thinking about this further: is this actually a semver-patch instead of semver-major? Because the use case when a error is now thrown is clearly a mistake and did not work as the user intended.

Nit: difficult to spot -> difficult-to-spot

Should it be as the second instead of as second? That seems more natural to me.

Hm, for me as second sounds better but I am probably not the best one to ask in this case. Are there any other votes? 😆

Am I missing something subtle? It looks like line 794 and line 796 are the same?

The called function is named similar but different. I clarified it now by choosing a different name.

@BridgeAR I'm OK with patch for this but would be interested in some other @nodejs/tsc opinions. Technically, anything that introduces a throw in a situation where it previously was not throwing is supposed to be treated as a breaking change, but I'm not sure that really applies in a situation like this where the code that doesn't throw is kinda broken anyway.

Left some nits on the new doc changes, but I really like the direction you went with it. 👍

I updated it and also added a independent commit to clarify how the RegExp actually works.

LGTM

I would release this as minor. Mainly to give it a bit more time to bake for LTS.
I'm fine with patch as well.

CI https://ci.nodejs.org/job/node-test-commit-light/18/

Please void using informal pronouns like you in the docs :-)

I rephrased that but I am not really happy with what I came up with
Please read the example below carefully if using a string as the second argument gets considered:
and
If it was intended to match for the error message do this instead:

Few grammar notes

Comma after notation
as a second argument
✂️ at all
✂️ otherwise

Needs a comma after object

as a second argument

I would think it should be more like as the second argument since there can only be one

I addressed the comments but I am not that happy with all the changes. So it would be nice to get another look at the sentences that contained a you before.

New CI https://ci.nodejs.org/job/node-test-commit-light/30/

@BridgeAR I think this should be using node-test-pull-request not node-test-commit-light, in order to run the tests on all platforms. node-test-commit-light is usually only meant to be used on documentation only PRs

@maclover7 I intentionally used that as it should only verify that the rebase was fine. assert has no special behavior on any platform and it should always be sufficient to only run linux-one in that case.

It takes ages to run a full CI for changes that absolutely behave the same on all platforms and there is no point in doing that out of my perspective.

PTAL

This does not land cleanly on v9.x, would someone be willing to backport?

Throw a TypeError in case a error message is provided in the second argument and a third argument is present as well. This is clearly a mistake and should not be done. PR-URL: nodejs#17585 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

It was not clear why the error name is actually also tested for when using a regular expression. This is now clarified. PR-URL: nodejs#17585 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Backport opened in #19230

Throw a TypeError in case a error message is provided in the second argument and a third argument is present as well. This is clearly a mistake and should not be done. Backport-PR-URL: #19230 PR-URL: #17585 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

It was not clear why the error name is actually also tested for when using a regular expression. This is now clarified. Backport-PR-URL: #19230 PR-URL: #17585 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Throw a TypeError in case a error message is provided in the second argument and a third argument is present as well. This is clearly a mistake and should not be done. Backport-PR-URL: #19230 PR-URL: #17585 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

It was not clear why the error name is actually also tested for when using a regular expression. This is now clarified. Backport-PR-URL: #19230 PR-URL: #17585 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Requested backport to 8.x in #19230

Throw a TypeError in case a error message is provided in the second argument and a third argument is present as well. This is clearly a mistake and should not be done. PR-URL: nodejs#17585 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Throw a TypeError in case a error message is provided in the second argument and a third argument is present as well. This is clearly a mistake and should not be done. Backport-PR-URL: #23223 PR-URL: #17585 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 10, 2017

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Dec 10, 2017

Trott reviewed Dec 10, 2017

View reviewed changes

Trott approved these changes Dec 10, 2017

View reviewed changes

benjamingr approved these changes Dec 10, 2017

View reviewed changes

Trott reviewed Dec 10, 2017

View reviewed changes

mcollina approved these changes Dec 11, 2017

View reviewed changes

BridgeAR added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 12, 2017

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 12, 2017

jasnell reviewed Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

maclover7 reviewed Dec 13, 2017

View reviewed changes

gireeshpunathil approved these changes Dec 13, 2017

View reviewed changes

evanlucas approved these changes Dec 13, 2017

View reviewed changes

BridgeAR force-pushed the improve-assert-throws branch from 5847dc0 to fcf02ed Compare December 15, 2017 23:17

BridgeAR closed this Dec 19, 2017

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

TimothyGu removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 13, 2018

BridgeAR mentioned this pull request Mar 8, 2018

[9.x backport] assert & util PRs #19230

Closed

4 tasks

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

targos added backported-to-v9.x and removed backport-requested-v9.x labels Mar 24, 2018

MylesBorins added the backport-requested-v8.x label Jul 31, 2018

MylesBorins added backported-to-v8.x and removed backport-requested-v8.x labels Oct 31, 2018

BethGriggs mentioned this pull request Nov 12, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

SimenB mentioned this pull request Dec 2, 2018

[v8.x] backport some assert commits #23223

Closed

4 tasks

BridgeAR deleted the improve-assert-throws branch April 1, 2019 23:37

assert: improve assert.throws by BridgeAR · Pull Request #17585 · nodejs/node · GitHub

Uh oh!

Uh oh!

assert: improve assert.throws - #17585

assert: improve assert.throws#17585BridgeAR wants to merge 2 commits intonodejs:masternodejs/node:masterfrom BridgeAR:improve-assert-throwsBridgeAR/node:improve-assert-throwsCopy head branch name to clipboard

Conversation

BridgeAR commented Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 10, 2017

Uh oh!

BridgeAR commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 10, 2017

Uh oh!

Trott commented Dec 10, 2017

Uh oh!

BridgeAR commented Dec 11, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 11, 2017

Uh oh!

BridgeAR commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

maclover7 commented Dec 15, 2017

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

BridgeAR commented Dec 16, 2017

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

BridgeAR commented Mar 8, 2018

assert: improve assert.throws#17585
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:improve-assert-throws

BridgeAR commented Dec 10, 2017 •
edited
Loading