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: .throws accept objects #17584

Closed
wants to merge 1 commit into
base: master
from

Conversation

@BridgeAR
Member

BridgeAR commented Dec 10, 2017

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

If specified, `error` can be a constructor, [`RegExp`][], or validation
function.
If specified, `error` can be a constructor, [`RegExp`][], a validation
function, or a object where each property will be tested for.

This comment has been minimized.

@Trott

Trott Dec 10, 2017

Member

Nit: a object -> an object

@Trott

This comment has been minimized.

Member

Trott commented Dec 10, 2017

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

@benjamingr

LGTM and +1

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Dec 10, 2017

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:

  1. accepts special input e.g. a RegExp for some fields (e.g. message).
  2. has a mandatory check on the code property.
  3. can be used as callback function that also works like common.mustCall.
  4. strictly verifies that the type property, if present, is also inherited by Error (or is Error).
  5. verifies that the error is a instance of type (if type is present).
  6. 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

This comment has been minimized.

Member

Trott commented Dec 10, 2017

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

@lpinca

lpinca approved these changes Dec 12, 2017

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Dec 12, 2017

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

createMsg(msg, 'message', actual, expected));
}
const keys = Object.keys(expected);
for (const key of keys) {

This comment has been minimized.

@ronkorving

ronkorving Dec 14, 2017

Contributor

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?

This comment has been minimized.

@BridgeAR

BridgeAR Dec 15, 2017

Member

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

This comment has been minimized.

Member

BridgeAR commented Dec 20, 2017

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

assert: .throws accept objects
From now on it is possible to use a validation object in throws
instead of the other possibilites.
@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Dec 21, 2017

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Dec 21, 2017

Landed in 2d37491

@BridgeAR BridgeAR closed this Dec 21, 2017

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 21, 2017

assert: .throws accept objects
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>

@addaleax addaleax removed the author ready label Dec 21, 2017

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jan 9, 2018

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

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

assert: .throws accept objects
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>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

assert: .throws accept objects
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 added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018

assert: .throws accept objects
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 BridgeAR referenced this pull request Mar 8, 2018

Closed

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

4 of 4 tasks complete
@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Mar 8, 2018

Backport opened in #19230

MylesBorins added a commit that referenced this pull request Mar 15, 2018

assert: .throws accept objects
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>

@targos targos referenced this pull request Mar 18, 2018

Merged

v9.9.0 proposal #19428

MylesBorins added a commit that referenced this pull request Mar 20, 2018

assert: .throws accept objects
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>

MylesBorins added a commit that referenced this pull request Mar 21, 2018

2018-03-21, Version 9.9.0 (Current)
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 added a commit that referenced this pull request Mar 21, 2018

2018-03-21, Version 9.9.0 (Current)
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

FallenRiteMonk added a commit to FallenRiteMonk/node that referenced this pull request Mar 23, 2018

2018-03-21, Version 9.9.0 (Current)
Notable changes:

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

PR-URL: nodejs#19428
@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jul 31, 2018

Requested backport to 8.x in #19230

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

assert: .throws accept objects
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 added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

assert: .throws accept objects
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 added a commit that referenced this pull request Oct 31, 2018

assert: .throws accept objects
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>

@BethGriggs BethGriggs referenced this pull request Nov 12, 2018

Open

v8.13.0 proposal #23974

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment