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

test: refactor common.expectsError #31092

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Dec 25, 2019

This completely refactors the expectsError behavior: so far it's
almost identical to assert.throws(fn, object) in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions type in case the type property
was set. This pattern is now completely removed and assert.throws()
should be used instead.

The main intent for common.expectsError() is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilities that assert.throws() accepts. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

The custom prefer-common-expectserror eslint rule comes obsolete with
this change and common us used significantly less frequent.

Refs: #30186 (comment)

Most changes are trivial and it should be possible to skim through
most of them.
Files that have been changed more "significantly":

  • test/parallel/test-crypto.js
  • test/common/index.js
  • test/common/README.md
  • test/parallel/test-eslint-prefer-common-expectserror.js (removed)
  • test/parallel/test-http-invalid-path-chars.js
  • test/parallel/test-net-better-error-messages-port-hostname.js
  • test/parallel/test-net-connect-immediate-finish.js
  • test/parallel/test-stream-writable-change-default-encoding.js
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 completely refactors the `expectsError` behavior: so far it's
almost identical to `assert.throws(fn, object)` in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions `type` in case `type` was passed
used in the validation object. This pattern is now completely removed
and `assert.throws()` should be used instead.

The main intent for `common.expectsError()` is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilites that `assert.throws()` accepts as well. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

This has the side effect that `common` is used significantly less
frequent.
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Dec 26, 2019
@nodejs-github-bot

This comment has been minimized.

BridgeAR added a commit that referenced this pull request Dec 31, 2019
This completely refactors the `expectsError` behavior: so far it's
almost identical to `assert.throws(fn, object)` in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions `type` in case `type` was passed
used in the validation object. This pattern is now completely removed
and `assert.throws()` should be used instead.

The main intent for `common.expectsError()` is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilites that `assert.throws()` accepts as well. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

This has the side effect that `common` is used significantly less
frequent.

PR-URL: #31092
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 31, 2019

Landed in e038d6a 🎉

@BridgeAR BridgeAR closed this Dec 31, 2019
cjihrig added a commit to cjihrig/node-1 that referenced this pull request Dec 31, 2019
This custom ESLint rule is unused since
nodejs#31092. This commit
removes it.
@cjihrig cjihrig mentioned this pull request Dec 31, 2019
2 of 2 tasks complete
Trott added a commit that referenced this pull request Jan 2, 2020
This custom ESLint rule is unused since
#31092. This commit
removes it.

PR-URL: #31147
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Jan 3, 2020
This completely refactors the `expectsError` behavior: so far it's
almost identical to `assert.throws(fn, object)` in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions `type` in case `type` was passed
used in the validation object. This pattern is now completely removed
and `assert.throws()` should be used instead.

The main intent for `common.expectsError()` is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilites that `assert.throws()` accepts as well. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

This has the side effect that `common` is used significantly less
frequent.

PR-URL: #31092
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR added a commit that referenced this pull request Jan 3, 2020
This custom ESLint rule is unused since
#31092. This commit
removes it.

PR-URL: #31147
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
@targos

This comment has been minimized.

Copy link
Member

targos commented Jan 14, 2020

This needs a backport or other previous PRs to be backported in order to land on v12.x-staging.

@BridgeAR BridgeAR deleted the BridgeAR:2019-12-25-refactor-common-expects-error branch Jan 20, 2020
sxa555 added a commit to sxa555/node that referenced this pull request Jan 21, 2020
This completely refactors the `expectsError` behavior: so far it's
almost identical to `assert.throws(fn, object)` in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions `type` in case `type` was passed
used in the validation object. This pattern is now completely removed
and `assert.throws()` should be used instead.

The main intent for `common.expectsError()` is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilites that `assert.throws()` accepts as well. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

This has the side effect that `common` is used significantly less
frequent.

PR-URL: nodejs#31092
sxa555 added a commit to sxa555/node that referenced this pull request Jan 22, 2020
This completely refactors the `expectsError` behavior: so far it's
almost identical to `assert.throws(fn, object)` in case it was used
with a function as first argument. It had a magical property check
that allowed to verify a functions `type` in case `type` was passed
used in the validation object. This pattern is now completely removed
and `assert.throws()` should be used instead.

The main intent for `common.expectsError()` is to verify error cases
for callback based APIs. This is now more flexible by accepting all
validation possibilites that `assert.throws()` accepts as well. No
magical properties exist anymore. This reduces surprising behavior
for developers who are not used to the Node.js core code base.

This has the side effect that `common` is used significantly less
frequent.

PR-URL: nodejs#31092
Backport-PR-URL: nodejs#31449
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.