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

BridgeAR · 2018-03-08T15:28:00Z

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

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

The stack frames from .throws and .doesNotThrow got included even though that was not intended. PR-URL: nodejs#17703 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

And make `assert.doesNotThrow()` handle it as well. PR-URL: nodejs#18029Fixes: nodejs#18027 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: nodejs#17615 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Destructure the necessary Error classes from internal/errors. This improves the readability of the error creation. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

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#18248Fixes: nodejs#18228 Refs: nodejs#18228 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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: #18248Fixes: #18228 Refs: #18228 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2018-03-20T11:36:54Z

landed in 16ab3b5...bae5de1

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ron Korving <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

And make `assert.doesNotThrow()` handle it as well. Backport-PR-URL: #19230 PR-URL: #18029Fixes: #18027 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

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: #18248Fixes: #18228 Refs: #18228 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2018-07-31T18:24:42Z

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

/cc @nodejs/backporters @BridgeAR

MylesBorins · 2018-08-07T17:32:03Z

if we do backport this should come with #18478

BridgeARand others added 21 commits March 8, 2018 14:28

assert: fix throws and doesNotThrow stack frames
9664cc8
The stack frames from .throws and .doesNotThrow got included even though that was not intended. PR-URL: nodejs#17703 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]>

test: refactor common.expectsError
0ed963b
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

util: rename util.inspect argument
f77f91d
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 <[email protected]>

assert: improve error messages
70ac318
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

tty: refactor to es6
073cf69
PR-URL: nodejs#17615 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

assert: use destructuring for errors
4fbed71
Destructure the necessary Error classes from internal/errors. This improves the readability of the error creation. PR-URL: nodejs#18247 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

assert: fix throws trace
6694f48
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

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

This was referenced Mar 8, 2018
util: add util.inspect compact option #17576
Closed
assert: add strict functionality export #17002
Closed
assert: use object argument in innerFail #17582
Closed
assert: fix .throws and .doesNotThrow stack frames #17703
Closed

MylesBorins closed this Mar 20, 2018

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
tty: refactor to es6
74f0d1a
Backport-PR-URL: #19230 PR-URL: #17615 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

bzoz mentioned this pull request Mar 28, 2018
test: fix test-tty-get-color-depth #18478
Closed
2 tasks

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

BridgeAR deleted the 9.backport-assert-stuff branch April 1, 2019 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

BridgeAR commented Mar 8, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

MylesBorins commented Jul 31, 2018

Uh oh!

MylesBorins commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

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

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

Uh oh!

Conversation

BridgeAR commented Mar 8, 2018

Checklist

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

MylesBorins commented Jul 31, 2018

Uh oh!

MylesBorins commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants