assert: provide extra context in case of failed error validation checks #28263

BridgeAR · 2019-06-17T09:48:47Z

Please have a look at the individual commit messages for a detailed description.

I mark this as semver-minor since it adds a wrapper to the actual triggered error and as such it's something new.

// cc @nodejs/assert

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

This improves `assert.throws()` and `assert.rejects()` in case error classes are used as validation for the expected error. In case of a failure it will now throw an `AssertionError` instead of the received error if the check fails. That error has the received error as actual property and the expected property is set to the expected error class. The error message should help users to identify the problem faster than before by providing extra context what went wrong.

This makes sure the `generatedMessage` property is always set as expected. This was not the case some `assert.throws` and `assert.rejects` calls.

This makes sure that validation function used by `assert.throws` and `assert.rejects` always throw validatin errors instead of rethrowing the received error. That should improve the debugging experience for developers since they have a better context where the error is coming from and they also get to know what triggered it.

This refactors some code for less duplication.

This adds information about the actual thrown error to the AssertionError's message property. It also improves the logged error instances error name by using the constructors name, if available.

This makes sure that using `assert.throws()` or `assert.rejects()` in combination with Error classes log appropriate error messages in case the expected and received constructor name are identical but not part of the same prototype chain.

This updates two outdated examples to the current implementation.

This makes sure the current validation function example reflects the current implementation.

doc/api/assert.md

This makes sure that `AssertionError` links to the correct place in the assert documentation.

BridgeAR · 2019-06-20T16:02:34Z

@Trott I just updated the PR and incorporated your feedback :)

nodejs-github-bot · 2019-06-20T16:07:48Z

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

BridgeAR · 2019-06-22T16:58:22Z

@nodejs/assert PTAL

Trott

LGTM but worth a CITGM run out of extra caution, I think.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1890/

BridgeAR · 2019-06-26T01:49:38Z

The failures in CITGM are related to esm. I can't see anything related to this PR.

Trott · 2019-07-06T01:48:57Z

@nodejs/collaborators Theoretically, this can land, but it would be nice to have a few other eyes on it. Anyone up for reviewing it?

boneskull · 2019-07-06T19:33:14Z

I’m a bit confused. before this change, throws would throw the error which was thrown if the function does not throw with the expected class of error? and now it throws assertionerror?

Trott · 2019-07-07T02:04:42Z

I’m a bit confused. before this change, throws would throw the error which was thrown if the function does not throw with the expected class of error? and now it throws assertionerror?

Yes, that's correct.

boneskull · 2019-07-07T02:44:30Z

If that’s the case, this seems like a breaking change.

Notable changes: * assert: * If the validation function passed to `assert.throws()` or `assert.rejects()` returns a value other than `true`, an assertion error will be thrown instead of the original error to highlight the programming mistake (Ruben Bridgewater). #28263 * If a constructor function is passed to validate the instance of errors thrown in `assert.throws()` or `assert.reject()`, an assertion error will be thrown instead of the original error (Ruben Bridgewater). #28263 * build: * Node.js releases are now built with default full-icu support. This means that all locales supported by ICU are now included and Intl-related APIs may return different values than before (Richard Lau). #29887 * The minimum Xcode version supported for macOS was increased to 10. It is still possible to build Node.js with Xcode 8 but this may no longer be the case in a future v13.x release (Michael Dawson). #29622 * child_process: * `ChildProcess._channel` (DEP0129) is now a Runtime deprecation (cjihrig). #27949 * console: * The output `console.timeEnd()` and `console.timeLog()` will now automatically select a suitable time unit instead of always using milliseconds (Xavier Stouder). #29251 * deps: * The V8 engine was updated to version 7.8. This includes performance improvements to object destructuring, memory usage and WebAssembly startup time (Myles Borins). #29694) * domain: * The domain's error handler is now executed with the active domain set to the domain's parent to prevent inner recursion (Julien Gilli). #26211 * fs: * The undocumented method `FSWatcher.prototype.start()` was removed (Lucas Holmquist). #29905 * Calling the `open()` method on a `ReadStream` or `WriteStream` now emits a runtime deprecation warning. The methods are supposed to be internal and should not be called by user code (Robert Nagy). #29061 * `fs.read/write`, `fs.readSync/writeSync` and `fd.read/write` now accept any safe integer as their `offset` parameter. The value of `offset` is also no longer coerced, so a valid type must be passed to the functions (Zach Bjornson). #26572 * http: * Aborted requests no longer emit the `end` or `error` events after `aborted` (Robert Nagy). #27984 #20077 * Data will no longer be emitted after a socket error (Robert Nagy). #28711 * The legacy HTTP parser (previously available under the `--http-parser=legacy` flag) was removed (Anna Henningsen). #29589 * The `host` option for HTTP requests is now validated to be a string value (Giorgos Ntemiris). #29568 * The `request.connection` and `response.connection` properties are now runtime deprecated. The equivalent `request.socket` and `response.socket` should be used instead (Robert Nagy). #29015 * http, http2: * The default server timeout was removed (Ali Ijaz Sheikh). #27558 * Brought 425 status code name into accordance with RFC 8470. The name changed from "Unordered Collection" to "Too Early" (Sergei Osipov). #29880 * lib: * The `error.errno` property will now always be a number. To get the string value, use `error.code` instead (Joyee Cheung). #28140 * module: * `module.createRequireFromPath()` is deprecated. Use `module.createRequire()` instead (cjihrig). #27951 * src: * Changing the value of `process.env.TZ` will now clear the tz cache. This affects the default time zone used by methods such as `Date.prototype.toString` (Ben Noordhuis). #20026 * stream: * The timing and behavior of streams was consolidated for a number of edge cases. Please look at the individual commits below for more information. PR-URL: #29504

This makes sure the `generatedMessage` property is always set as expected. This was not the case some `assert.throws` and `assert.rejects` calls. PR-URL: nodejs#28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This refactors some code for less duplication. PR-URL: nodejs#28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This updates two outdated examples to the current implementation. PR-URL: nodejs#28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This makes sure that `AssertionError` links to the correct place in the assert documentation. PR-URL: nodejs#28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This makes sure the `generatedMessage` property is always set as expected. This was not the case some `assert.throws` and `assert.rejects` calls. Backport-PR-URL: #31431 PR-URL: #28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This refactors some code for less duplication. Backport-PR-URL: #31431 PR-URL: #28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This updates two outdated examples to the current implementation. Backport-PR-URL: #31431 PR-URL: #28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This makes sure that `AssertionError` links to the correct place in the assert documentation. Backport-PR-URL: #31431 PR-URL: #28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This makes sure the `generatedMessage` property is always set as expected. This was not the case some `assert.throws` and `assert.rejects` calls. Backport-PR-URL: #31431 PR-URL: #28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This refactors some code for less duplication. Backport-PR-URL: #31431 PR-URL: #28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This updates two outdated examples to the current implementation. Backport-PR-URL: #31431 PR-URL: #28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This makes sure that `AssertionError` links to the correct place in the assert documentation. Backport-PR-URL: #31431 PR-URL: #28263 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

…rejects() returns a value other than true, an assertion error will be thrown instead of the original error to highlight the programming mistake refs: nodejs/node#28263

… errors thrown in assert.throws() or assert.reject(), an assertion error will be thrown instead of the original error refs: nodejs/node#28263

…rejects() returns a value other than true, an assertion error will be thrown instead of the original error to highlight the programming mistake refs: nodejs/node#28263

… errors thrown in assert.throws() or assert.reject(), an assertion error will be thrown instead of the original error refs: nodejs/node#28263

…rejects() returns a value other than true, an assertion error will be thrown instead of the original error to highlight the programming mistake refs: nodejs/node#28263

… errors thrown in assert.throws() or assert.reject(), an assertion error will be thrown instead of the original error refs: nodejs/node#28263

BridgeAR added 6 commits June 15, 2019 14:25

assert: fix generatedMessage property

f90cfca

This makes sure the `generatedMessage` property is always set as expected. This was not the case some `assert.throws` and `assert.rejects` calls.

assert: DRY .throws code

1563d9d

This refactors some code for less duplication.

assert: add more information to AssertionErrors

9d7d103

This adds information about the actual thrown error to the AssertionError's message property. It also improves the logged error instances error name by using the constructors name, if available.

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 17, 2019

BridgeAR requested a review from Trott June 17, 2019 09:48

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jun 17, 2019

BridgeAR added 2 commits June 17, 2019 16:35

doc: update assert.throws() examples

e096890

This updates two outdated examples to the current implementation.

doc: update assert.throws example

fa0f3f1

This makes sure the current validation function example reflects the current implementation.

Trott reviewed Jun 17, 2019

View reviewed changes

doc/api/assert.md Outdated Show resolved Hide resolved

Trott reviewed Jun 17, 2019

View reviewed changes

doc/api/assert.md Outdated Show resolved Hide resolved

Trott reviewed Jun 17, 2019

View reviewed changes

doc/api/assert.md Outdated Show resolved Hide resolved

BridgeAR added 2 commits June 20, 2019 17:13

doc: make AssertionError a link

683e311

This makes sure that `AssertionError` links to the correct place in the assert documentation.

fixup: doc: update assert.throws example

9832d3d

Trott approved these changes Jun 22, 2019

View reviewed changes

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Jun 26, 2019

benjamingr approved these changes Jul 6, 2019

View reviewed changes

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 7, 2019

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 7, 2019

BridgeAR deleted the improve-assert-throws-rejects-errors branch January 20, 2020 12:05

BridgeAR mentioned this pull request Jan 22, 2020

[v12.x] multiple util and assert backports #31431

Closed

4 tasks

BridgeAR added the backport-open-v12.x label Jan 22, 2020

MylesBorins added backported-to-v12.x and removed backport-open-v12.x labels Jan 30, 2020

targos mentioned this pull request Jan 30, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

twada mentioned this pull request Apr 18, 2020

Sync with behavior changes in Node13 twada/rejected-or-not#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: provide extra context in case of failed error validation checks #28263

assert: provide extra context in case of failed error validation checks #28263

BridgeAR commented Jun 17, 2019 •

edited

BridgeAR commented Jun 20, 2019

nodejs-github-bot commented Jun 20, 2019

BridgeAR commented Jun 22, 2019

Trott left a comment

BridgeAR commented Jun 26, 2019

Trott commented Jul 6, 2019

boneskull commented Jul 6, 2019

Trott commented Jul 7, 2019

boneskull commented Jul 7, 2019

assert: provide extra context in case of failed error validation checks #28263

assert: provide extra context in case of failed error validation checks #28263

Conversation

BridgeAR commented Jun 17, 2019 • edited

Checklist

BridgeAR commented Jun 20, 2019

nodejs-github-bot commented Jun 20, 2019

BridgeAR commented Jun 22, 2019

Trott left a comment

Choose a reason for hiding this comment

BridgeAR commented Jun 26, 2019

Trott commented Jul 6, 2019

boneskull commented Jul 6, 2019

Trott commented Jul 7, 2019

boneskull commented Jul 7, 2019

BridgeAR commented Jun 17, 2019 •

edited