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.expectWarning()` #25251

Closed
wants to merge 4 commits into from

Conversation

@BridgeAR
Copy link
Member

commented Dec 28, 2018

The current API is somewhat confusing at times and simpler usage is
possible. This overloads the arguments further to accept objects
with deprecation codes as property keys. It also adds documentation
for the different possible styles.

Besides that it is now going to validate for the code being present
in case of deprecations but not for other cases. The former validation
was not consistent as it only validated some cases and accepted
undefined instead of common.NoWarnCode. This check is removed due to
the lack of consistency.

This also verifies that the warning order is identical to the order
in which they are triggered.

I also removed common.noWarnCode since it's not really required after
making sure all deprecation warnings have a code.

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 added 2 commits Dec 28, 2018
test: rework `common.expectWarning()`
The current API is somewhat confusing at times and simpler usage is
possible. This overloads the arguments further to accept objects
with deprecation codes as property keys. It also adds documentation
for the different possible styles.

Besides that it is now going to validate for the code being present
in case of deprecations but not for other cases. The former validation
was not consistent as it only validated some cases and accepted
undefined instead of `common.NoWarnCode`. This check is removed due to
the lack of consistency.

This also verifies that the warning order is identical to the order
in which they are triggered.
test: remove common.noWarnCode
This property was just sugar for `undefined` and did not check much.
Now that we properly check that a DeprecationWarning requires a code
we do not have to check that a code is always present.

@BridgeAR BridgeAR force-pushed the BridgeAR:rework-expect-warning branch from 17620af to b84f495 Dec 28, 2018

@danbev
danbev approved these changes Jan 7, 2019
@danbev

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

@nodejs/testing this could use another LG

@Trott
Copy link
Member

left a comment

Code changes seem good to me, but I left a comment about the doc update. (The argument types specified do not correspond to the examples provided.)

BridgeAR added 2 commits Jan 9, 2019
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

@jasnell
jasnell approved these changes Jan 9, 2019
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2019

Landed in baa4b9b 🎉

Thanks for the reviews.

@BridgeAR BridgeAR closed this Jan 10, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 10, 2019
test: refactor `common.expectWarning()`
The current API is somewhat confusing at times and simpler usage is
possible. This overloads the arguments further to accept objects
with deprecation codes as property keys. It also adds documentation
for the different possible styles.

Besides that it is now going to validate for the code being present
in case of deprecations but not for other cases. The former validation
was not consistent as it only validated some cases and accepted
undefined instead of `common.noWarnCode`. This check is removed due to
the lack of consistency. `common.noWarnCode` is completely removed
due to just being sugar for `undefined`.

This also verifies that the warning order is identical to the order
in which they are triggered.

PR-URL: nodejs#25251
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Jan 14, 2019
test: refactor `common.expectWarning()`
The current API is somewhat confusing at times and simpler usage is
possible. This overloads the arguments further to accept objects
with deprecation codes as property keys. It also adds documentation
for the different possible styles.

Besides that it is now going to validate for the code being present
in case of deprecations but not for other cases. The former validation
was not consistent as it only validated some cases and accepted
undefined instead of `common.noWarnCode`. This check is removed due to
the lack of consistency. `common.noWarnCode` is completely removed
due to just being sugar for `undefined`.

This also verifies that the warning order is identical to the order
in which they are triggered.

PR-URL: #25251
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
test: refactor `common.expectWarning()`
The current API is somewhat confusing at times and simpler usage is
possible. This overloads the arguments further to accept objects
with deprecation codes as property keys. It also adds documentation
for the different possible styles.

Besides that it is now going to validate for the code being present
in case of deprecations but not for other cases. The former validation
was not consistent as it only validated some cases and accepted
undefined instead of `common.noWarnCode`. This check is removed due to
the lack of consistency. `common.noWarnCode` is completely removed
due to just being sugar for `undefined`.

This also verifies that the warning order is identical to the order
in which they are triggered.

PR-URL: nodejs#25251
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.