assert: .throws accept objects #17584

BridgeAR · 2017-12-10T00:59:42Z

From now on it is possible to use a validation object in throws
instead of the other possibilities.

Refs #17557

I personally am not sure if this is a cool thing or not but I thought I just put something together to get some votes.

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

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

Trott · 2017-12-10T05:34:20Z

doc/api/assert.md

Nit: a object -> an object

Trott · 2017-12-10T05:37:18Z

Thanks for doing this. I like it. This should make it possible to simplify (or even eliminate?) common.expectsError().

benjamingr

LGTM and +1

BridgeAR · 2017-12-10T20:07:06Z

I addressed the comment and improved some things further. Right now I still do not see how it will replace or simplify common.expectsError though.

The differences as I see them:
common.expectsError:

accepts special input e.g. a RegExp for some fields (e.g. message).
has a mandatory check on the code property.
can be used as callback function that also works like common.mustCall.
strictly verifies that the type property, if present, is also inherited by Error (or is Error).
verifies that the error is a instance of type (if type is present).
only checks a limited amount of fields no matter if there are more on the provided object.

So we could overcome some of these e.g.

(1) - We could special handle either especially the message field to also accept a RegExp or to always accept a RegExp in case the field to test contains a string.
(3) - by continuing to use common.expectsError in exactly that case or we could add that functionality to assert.throws as well.
(6) - is actually better here out of my perspective.

(4) and (5) are probably not really that important even though the replacement would only be to check for the name, but that would still be good enough out of my perspective. I do not see any solution for (2) though.

Trott · 2017-12-10T22:54:08Z

I'm not sure I like the mandatory code check in common.epxectsError() anyway, to be honest. It means that common.expectsError() can only be used on internal Node.js errors and not on errors from the JS engine. But this can be a later discussion. Even if all this change does is make it easier to switch between assert.throws() and common.expectsError(), that's still a big enough win for me. :-D

BridgeAR · 2017-12-12T21:14:54Z

Any further votes on if this is a good or bad idea? @nodejs/collaborators PTAL

ronkorving · 2017-12-14T09:18:19Z

lib/assert.js

name and message have already been tested for, so perhaps they should be explicitly not tested for again? I imagine that testing for them would be a very common scenario, in which case not running deepStrictEqual will probably run a bit faster. Thoughts?

I guess the performance will be exactly the same. deepStrictEqual will only do

// Already existing function equal(a, b) { if (a === b) return true } // This check is executed if (equal(a, b) !== true) throw new Error(msg)

mcollina

LGTM

BridgeAR · 2017-12-20T21:10:05Z

Rebased. New CI https://ci.nodejs.org/job/node-test-commit-light/43/
(Only the small one to verify the rebase as the full run did not show anything to look further into).

From now on it is possible to use a validation object in throws instead of the other possibilites.

BridgeAR · 2017-12-21T04:57:34Z

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

BridgeAR · 2017-12-21T06:03:26Z

Landed in 2d37491

From now on it is possible to use a validation object in throws instead of the other possibilites. PR-URL: nodejs#17584 Refs: nodejs#17557 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

MylesBorins · 2018-01-09T23:33:31Z

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

From now on it is possible to use a validation object in throws instead of the other possibilites. PR-URL: nodejs#17584 Refs: nodejs#17557 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

BridgeAR · 2018-03-08T16:23:33Z

Backport opened in #19230

From now on it is possible to use a validation object in throws instead of the other possibilites. Backport-PR-URL: #19230 PR-URL: #17584 Refs: #17557 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

Notable changes: * assert: - From now on all error messages produced by `assert` in strict mode will produce a error diff. (Ruben Bridgewater) #17615 - From now on it is possible to use a validation object in throws instead of the other possibilities. (Ruben Bridgewater) #17584 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * fs: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * tty: - Add getColorDepth function to determine if terminal supports colors (Ruben Bridgewater) #17615 * util: - add util.inspect compact option (Ruben Bridgewater) #17576 * **Added new collaborators** - [watson](https://github.com/watson) Thomas Watson PR-URL: #19428

MylesBorins · 2018-07-31T18:26:51Z

Requested backport to 8.x in #19230

From now on it is possible to use a validation object in throws instead of the other possibilites. PR-URL: nodejs#17584 Refs: nodejs#17557 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

From now on it is possible to use a validation object in throws instead of the other possibilites. Backport-PR-URL: #23223 PR-URL: #17584 Refs: #17557 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

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

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 10, 2017

Trott reviewed Dec 10, 2017

View reviewed changes

doc/api/assert.md Outdated

Copy link

Member

Trott Dec 10, 2017

Choose a reason for hiding this comment

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

Nit: a object -> an object

benjamingr approved these changes Dec 10, 2017

View reviewed changes

azu mentioned this pull request Dec 10, 2017

assert: .throws accept objects azu/node-browser-polyfill-gap#4

Open

lpinca approved these changes Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

ronkorving reviewed Dec 14, 2017

View reviewed changes

BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 15, 2017

mcollina approved these changes Dec 16, 2017

View reviewed changes

ronkorving approved these changes Dec 18, 2017

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 Dec 19, 2017

BridgeAR force-pushed the assert-throws-obj branch from 3759e60 to b986313 Compare December 20, 2017 21:08

assert: .throws accept objects

c0fc2f5

From now on it is possible to use a validation object in throws instead of the other possibilites.

BridgeAR force-pushed the assert-throws-obj branch from b986313 to c0fc2f5 Compare December 21, 2017 04:56

hiroppy approved these changes Dec 21, 2017

View reviewed changes

BridgeAR closed this Dec 21, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2017

MylesBorins added the backport-requested-v9.x label Jan 9, 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

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

Uh oh!

assert: .throws accept objects #17584

assert: .throws accept objects #17584

Uh oh!

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!

Trott Dec 10, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 10, 2017

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 10, 2017

Uh oh!

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

Uh oh!

Uh oh!

BridgeAR commented Dec 12, 2017

Uh oh!

ronkorving Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 20, 2017

Uh oh!

BridgeAR commented Dec 21, 2017

Uh oh!

BridgeAR commented Dec 21, 2017

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

BridgeAR commented Mar 8, 2018

Uh oh!

MylesBorins commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

BridgeAR commented Dec 10, 2017 •

edited

Loading

Trott commented Dec 10, 2017 •

edited

Loading