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

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

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jun 17, 2019

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.
@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 17, 2019
@BridgeAR BridgeAR requested a review from Trott June 17, 2019 09:48
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jun 17, 2019
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 Outdated Show resolved Hide resolved
doc/api/assert.md Outdated Show resolved Hide resolved
doc/api/assert.md Outdated Show resolved Hide resolved
This makes sure that `AssertionError` links to the correct place in
the assert documentation.
@BridgeAR
Copy link
Member Author

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

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member Author

@nodejs/assert PTAL

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

@BridgeAR 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
@Trott
Copy link
Member

Trott commented Jul 6, 2019

@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
Copy link
Contributor

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 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
Copy link
Member

Trott commented Jul 7, 2019

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.

@Trott 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
@boneskull
Copy link
Contributor

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

BethGriggs added a commit that referenced this pull request Oct 22, 2019
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
BethGriggs added a commit that referenced this pull request Oct 22, 2019
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
@BridgeAR BridgeAR deleted the improve-assert-throws-rejects-errors branch January 20, 2020 12:05
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 22, 2020
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 22, 2020
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 22, 2020
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 22, 2020
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>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
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>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
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>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
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>
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
twada added a commit to twada/rejected-or-not that referenced this pull request Apr 18, 2020
…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
twada added a commit to twada/rejected-or-not that referenced this pull request Apr 18, 2020
… errors thrown in assert.throws() or assert.reject(), an assertion error will be thrown instead of the original error

refs: nodejs/node#28263
twada added a commit to twada/rejected-or-not that referenced this pull request Apr 18, 2020
…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
twada added a commit to twada/rejected-or-not that referenced this pull request Apr 18, 2020
… errors thrown in assert.throws() or assert.reject(), an assertion error will be thrown instead of the original error

refs: nodejs/node#28263
twada added a commit to twada/rejected-or-not that referenced this pull request Apr 18, 2020
…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
twada added a commit to twada/rejected-or-not that referenced this pull request Apr 18, 2020
… errors thrown in assert.throws() or assert.reject(), an assertion error will be thrown instead of the original error

refs: nodejs/node#28263
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants