Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: improve assert.throws #17585

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 10, 2017

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

@BridgeAR 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 nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Dec 10, 2017
@@ -772,7 +772,7 @@ argument, then `error` is assumed to be omitted and the string will be used for
<!-- eslint-disable no-restricted-syntax -->
```js
// THIS IS A MISTAKE! DO NOT DO THIS!
assert.throws(myFunction, 'missing foo', 'did not throw with expected message');
assert.throws(myFunction, 'missing foo'); // Did not throw with expected message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with doc comment addressed.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@BridgeAR
Copy link
Member Author

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

@BridgeAR
Copy link
Member Author

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.

```

Due to the confusing notation it is recommended not to use a string as second
argument at all. This might otherwise lead to difficult to spot errors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

```

Due to the confusing notation it is recommended not to use a string as second
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😆

// If you originally intended to match for the error message do this instead:
assert.throws(throwingMatched, /^Error: Matched$/);
// Does not throw because the error messages match.
assert.throws(throwingMismatch, /^Error: Matched$/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Trott
Copy link
Member

Trott commented Dec 10, 2017

@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.

@Trott
Copy link
Member

Trott commented Dec 10, 2017

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

@BridgeAR
Copy link
Member Author

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

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.

@BridgeAR 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
Copy link
Member Author

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2017
assert.throws(notThrowing, 'Second');
// AssertionError [ERR_ASSERTION]: Missing expected exception: Second

// If you originally intended to match for the error message do this instead:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few grammar notes

```

Due to the confusing notation it is recommended not to use a string as second
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -740,12 +740,15 @@ assert.throws(

Validate error message using [`RegExp`][]:

Using a regular expression runs `.toString` on the error object and will
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a comma after object

@@ -767,17 +770,41 @@ assert.throws(

Note that `error` can not be a string. If a string is provided as the second
argument, then `error` is assumed to be omitted and the string will be used for
`message` instead. This can lead to easy-to-miss mistakes:
`message` instead. This can lead to easy-to-miss mistakes. Please read the
example below carefully if you intend to use a string as second argument:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a second argument

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -767,17 +770,41 @@ assert.throws(

Note that `error` can not be a string. If a string is provided as the second
argument, then `error` is assumed to be omitted and the string will be used for
`message` instead. This can lead to easy-to-miss mistakes:
`message` instead. This can lead to easy-to-miss mistakes. Please read the
example below carefully if you intend to use a string as second argument:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@BridgeAR
Copy link
Member Author

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.

@BridgeAR
Copy link
Member Author

@maclover7
Copy link
Contributor

@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

@BridgeAR
Copy link
Member Author

@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.

@BridgeAR
Copy link
Member Author

PTAL

@MylesBorins
Copy link
Contributor

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

@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
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>
@BridgeAR BridgeAR mentioned this pull request Mar 8, 2018
4 tasks
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 8, 2018

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
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>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
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>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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>
@MylesBorins
Copy link
Contributor

Requested backport to 8.x in #19230

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018
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>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
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>
@BethGriggs BethGriggs mentioned this pull request Nov 12, 2018
@SimenB SimenB mentioned this pull request Dec 2, 2018
4 tasks
@BridgeAR BridgeAR deleted the improve-assert-throws branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.