Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test: change call to deprecated util.isError() #3084

Closed
wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 26, 2015

common.isError() is just the deprecated util.isError(). Replace with instanceof Error check.

@Trott Trott added the test Issues and PRs related to the tests. label Sep 26, 2015
@brendanashworth
Copy link
Contributor

LGTM

1 similar comment
@targos
Copy link
Member

targos commented Sep 27, 2015

LGTM

@Trott
Copy link
Member Author

Trott commented Sep 27, 2015

@ChALkeR
Copy link
Member

ChALkeR commented Sep 28, 2015

Maybe this test should use assert.throws()?

@Trott
Copy link
Member Author

Trott commented Sep 29, 2015

@ChALkeR Sure! How's it look now? /cc @targos @brendanashworth

@Trott
Copy link
Member Author

Trott commented Sep 29, 2015

CI with new changes: https://ci.nodejs.org/job/node-test-commit/694/

var exceptionCaught = false;

try {
const shouldThrow = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not the function declaration form?

Copy link
Member Author

Choose a reason for hiding this comment

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

There were no other functions defined in the file, so I did function expressions because it is what I tend to use. There is precedence for function expressions in test/parallel files. test-http-parser.js has 21 function expressions. Still, it was basically an arbitrary decision on my part and I can switch them to function declarations in this file if you or anyone else has a strong preference.

@Trott
Copy link
Member Author

Trott commented Oct 2, 2015

Can I get a confirmation LGTM for the additional change? @ChALkeR @targos @brendanashworth

@targos
Copy link
Member

targos commented Oct 2, 2015

LGTM

1 similar comment
@ChALkeR
Copy link
Member

ChALkeR commented Oct 2, 2015

LGTM

@Trott
Copy link
Member Author

Trott commented Oct 2, 2015

Landed in 5bbc6df

@Trott Trott closed this Oct 2, 2015
Trott added a commit that referenced this pull request Oct 2, 2015
common.Error() is just util.isError() which is deprecated. Use
assert.throws() instead.

PR-URL: #3084
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Trott added a commit that referenced this pull request Oct 28, 2015
common.Error() is just util.isError() which is deprecated. Use
assert.throws() instead.

PR-URL: #3084
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

Landed in v4.x-staging in 5bb6e0f

Trott added a commit that referenced this pull request Oct 29, 2015
common.Error() is just util.isError() which is deprecated. Use
assert.throws() instead.

PR-URL: #3084
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@Trott Trott deleted the rm-isError branch January 13, 2022 22:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants