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

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

Closed
wants to merge 21 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Mar 8, 2018

This is a backport of all assert PRs to 9.x that are requested at the moment (as far as I know) plus one util PR. I felt this is much easier because as soon as a couple conflicts were resolved most other commits were easy to backport on top as well.

#17576
#17002
#17582
#17703
#17585
#17584
#17903
#18029 (@bnoordhuis this landed cleanly after my former commits)
#17615
#18595
#18248
#18611

I also included 8e6e1c9 even though it landed in a semver-major PR. But that PR on it's own is not semver-major.

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 and others added 21 commits March 8, 2018 14:28
1) Separate all loose and strict functions.
2) Stronger outline the used comparison rules in (not)deepStrictEqual
3) Fix SameValue comparison info

PR-URL: nodejs#17002
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Requireing the strict version will allow to use `assert.equal`,
`assert.deepEqual` and there negated counterparts to be used with
strict comparison instead of using e.g. `assert.strictEqual`.

The API is identical to the regular assert export and only differs
in the way that all functions use strict compairson.

PR-URL: nodejs#17002
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.

PR-URL: nodejs#17582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.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.

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>
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>
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: nodejs#17582

PR-URL: nodejs#17903
Refs: nodejs#17582
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
And make `assert.doesNotThrow()` handle it as well.

PR-URL: nodejs#18029
Fixes: nodejs#18027
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Using a custom inspect function on the inspected object is
deprecated. Remove the reference from the option description
to make sure the user will read about the deprecation in the
more detailed description.

PR-URL: nodejs#17576
Reviewed-By: Anna Henningsen <anna@addaleax.net>
util.inspect can actually receive any property and the description
was wrong so far. This fixes it by renaming the argument to
value and also updating the description.

PR-URL: nodejs#17576
Reviewed-By: Anna Henningsen <anna@addaleax.net>
The current default formatting is not ideal and this improves
the situation by formatting the output more intuitiv.

1) All object keys are now indented by 2 characters instead of
   sometimes 2 and sometimes 3 characters.
2) Each object key will now use an individual line instead of
   sharing a line potentially with multiple object keys.
3) Long strings will now be split into multiple lines in case
   they exceed the "lineBreak" option length (including the
   current indentation).
4) Opening braces are now directly behind a object property
   instead of using a new line.
5) Switch inspect "base" order. In case the compact option is set
   to `false`, inspect will now print
     "[Function: foo] {\n  property: 'data'\n}"
   instead of
     "{ [Function: foo]\n  property: 'data'\n}".

PR-URL: nodejs#17576
Reviewed-By: Anna Henningsen <anna@addaleax.net>
From now on all error messages produced by `assert` in strict mode
will produce a error diff.

PR-URL: nodejs#17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Right now it is very difficult to determine if a terminal supports
colors or not. This function adds this functionality by detecting
environment variables and checking process.

PR-URL: nodejs#17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Destructure the necessary Error classes from internal/errors.
This improves the readability of the error creation.

PR-URL: nodejs#18247
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

PR-URL: nodejs#18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.

PR-URL: nodejs#18248
Fixes: nodejs#18228
Refs: nodejs#18228
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.

PR-URL: nodejs#18611
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.

PR-URL: nodejs#18611
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@nodejs-github-bot 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. tty Issues and PRs related to the tty subsystem. util Issues and PRs related to the built-in util module. v9.x labels Mar 8, 2018
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.

Backport-PR-URL: #19230
PR-URL: #18248
Fixes: #18228
Refs: #18228
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.

Backport-PR-URL: #19230
PR-URL: #18611
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.

Backport-PR-URL: #19230
PR-URL: #18611
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

landed in 16ab3b5...bae5de1

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Requireing the strict version will allow to use `assert.equal`,
`assert.deepEqual` and there negated counterparts to be used with
strict comparison instead of using e.g. `assert.strictEqual`.

The API is identical to the regular assert export and only differs
in the way that all functions use strict compairson.

Backport-PR-URL: #19230
PR-URL: #17002
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what stands for what.

Backport-PR-URL: #19230
PR-URL: #17582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

Backport-PR-URL: #19230
PR-URL: #17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.

Backport-PR-URL: #19230
PR-URL: #17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 pushed a commit that referenced this pull request Mar 20, 2018
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 pushed a commit that referenced this pull request Mar 20, 2018
Using `assert()` or `assert.ok()` resulted in a error since a
refactoring.

Refs: #17582

Backport-PR-URL: #19230
PR-URL: #17903
Refs: #17582
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
And make `assert.doesNotThrow()` handle it as well.

Backport-PR-URL: #19230
PR-URL: #18029
Fixes: #18027
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Using a custom inspect function on the inspected object is
deprecated. Remove the reference from the option description
to make sure the user will read about the deprecation in the
more detailed description.

Backport-PR-URL: #19230
PR-URL: #17576
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
util.inspect can actually receive any property and the description
was wrong so far. This fixes it by renaming the argument to
value and also updating the description.

Backport-PR-URL: #19230
PR-URL: #17576
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
The current default formatting is not ideal and this improves
the situation by formatting the output more intuitiv.

1) All object keys are now indented by 2 characters instead of
   sometimes 2 and sometimes 3 characters.
2) Each object key will now use an individual line instead of
   sharing a line potentially with multiple object keys.
3) Long strings will now be split into multiple lines in case
   they exceed the "lineBreak" option length (including the
   current indentation).
4) Opening braces are now directly behind a object property
   instead of using a new line.
5) Switch inspect "base" order. In case the compact option is set
   to `false`, inspect will now print
     "[Function: foo] {\n  property: 'data'\n}"
   instead of
     "{ [Function: foo]\n  property: 'data'\n}".

Backport-PR-URL: #19230
PR-URL: #17576
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
From now on all error messages produced by `assert` in strict mode
will produce a error diff.

Backport-PR-URL: #19230
PR-URL: #17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Right now it is very difficult to determine if a terminal supports
colors or not. This function adds this functionality by detecting
environment variables and checking process.

Backport-PR-URL: #19230
PR-URL: #17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Backport-PR-URL: #19230
PR-URL: #17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Destructure the necessary Error classes from internal/errors.
This improves the readability of the error creation.

Backport-PR-URL: #19230
PR-URL: #18247
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

Backport-PR-URL: #19230
PR-URL: #18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Right now it is not documentated that WeakMap entries are not
compared. This might result in some confusion. This adds a note
about the behavior in `assert.deepStrictEqual`. This documentation
is also references in `util.isDeepStrictEqual`, so we do not have
to document it again for that function as the underlying algorithm
is the same.

Backport-PR-URL: #19230
PR-URL: #18248
Fixes: #18228
Refs: #18228
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
In rare cirumstances it is possible to get a identical error diff.
In such a case the advances diffing runs into a infinite loop.
This fixes it by properly checking for extra entries.

Backport-PR-URL: #19230
PR-URL: #18611
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Right now it is possible to get an AssertionError from input that has
the customInspect function set to always return the same value.

That way the error message is actually misleading because the output
is going to look the same. This fixes it by deactivating the custom
inspect function.

Backport-PR-URL: #19230
PR-URL: #18611
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@bzoz bzoz mentioned this pull request Mar 28, 2018
2 tasks
@MylesBorins
Copy link
Contributor

Should this block be backported to v8.x????

/cc @nodejs/backporters @BridgeAR

@MylesBorins
Copy link
Contributor

if we do backport this should come with #18478

@BridgeAR BridgeAR deleted the 9.backport-assert-stuff branch April 1, 2019 23:39
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. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-minor PRs that contain new features and should be released in the next minor version. tty Issues and PRs related to the tty subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants