Skip to content

test: remove unused deprecation code - #19317

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:remove-unused-arguments-expectWarning
Closed

danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:remove-unused-arguments-expectWarning

Conversation

@danbev

@danbev danbev commented Mar 13, 2018

Copy link
Copy Markdown
Contributor

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 Node.js core tests and test infrastructure. label Mar 13, 2018
@danbev

danbev commented Mar 13, 2018

Copy link
Copy Markdown
Contributor Author

@Trott

Trott commented Mar 13, 2018

Copy link
Copy Markdown
Member

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 with CI started, the required approvals, and no outstanding review comments. label Mar 13, 2018
@danbev

danbev commented Mar 13, 2018

Copy link
Copy Markdown
Contributor Author

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

danbev commented Mar 15, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

@danbev danbev mentioned this pull request Mar 20, 2018
3 tasks done
@tniessen tniessen removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 24, 2018
@BridgeAR

BridgeAR commented May 1, 2018

Copy link
Copy Markdown
Member

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 Node.js core tests and test infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.