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: remove unused deprecation code #19317

Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 13, 2018

Currently there are two tests that specify a third argument, a
deprecation code string, when calling common.expectWarning. The
function only takes two arguments and this third argument is not used.

This commit removes the deprecation code.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Currently there are two tests that specify a third argument, a
deprecation code string, when calling common.expectWarning. The
function only takes two arguments and this third argument is not used.

This commit removes the deprecation code.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 13, 2018
@danbev
Copy link
Contributor Author

danbev commented Mar 13, 2018

@Trott
Copy link
Member

Trott commented Mar 13, 2018

No objection to this change, but going forward:

  • Should it take a third argument?

  • And/or shouldn't tests generally be checking the deprecation code anyway and not the message?

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2018
@danbev
Copy link
Contributor Author

danbev commented Mar 13, 2018

Should it take a third argument?

I think that would make sense. I can take a look but might not be until later this week or possibly next week.

@danbev
Copy link
Contributor Author

danbev commented Mar 15, 2018

Landed in 3ad7c1a.

@danbev danbev closed this Mar 15, 2018
danbev added a commit that referenced this pull request Mar 15, 2018
Currently there are two tests that specify a third argument, a
deprecation code string, when calling common.expectWarning. The
function only takes two arguments and this third argument is not used.

This commit removes the deprecation code.

PR-URL: #19317
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Jackson Tian <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@danbev danbev deleted the remove-unused-arguments-expectWarning branch March 15, 2018 06:28
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2018
@BridgeAR
Copy link
Member

BridgeAR commented May 1, 2018

This relies on a semver-major. I updated the labels accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.