Skip to content

test: common.expectsError() is subtly broken #13682

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

Jump to bottom
Closed
refack opened this issue Jun 14, 2017 · 0 comments
Closed

test: common.expectsError() is subtly broken #13682

refack opened this issue Jun 14, 2017 · 0 comments
Labels
test Issues and PRs related to the tests. wip Issues and PRs that are still a work in progress.

Comments

@refack
Copy link
Contributor

refack commented Jun 14, 2017

  • Version: *
  • Platform: *
  • Subsystem: test

Ref: #13623 (comment)
as reported by @cjihrig:

common.expectsError() is subtly broken. It relies on instanceof to check the error type. So a TypeError is an instance of Error, which is why this passed. The simplest fix might be to check error.constructor inside of common.expectsError(). But that will require updating a bunch of tests because we're now using custom errors all over the place.
@refack refack self-assigned this Jun 14, 2017
@refack refack added the test Issues and PRs related to the tests. label Jun 14, 2017
This was referenced Jun 14, 2017
Closed
Closed
@refack refack added the wip Issues and PRs that are still a work in progress. label Jun 15, 2017
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 6, 2018
@refack @BridgeAR
test: fix common.expectsError
29d99c1 
The function should strictly test for the error class and only accept
the correct one. Any other error class should fail.

PR-URL: nodejs#13686
Fixes: nodejs#13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 6, 2018
@refack @BridgeAR
test: add more asserts to test-internal-errors
15016f2 
PR-URL: nodejs#13686
Fixes: nodejs#13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Jan 8, 2018
@refack @MylesBorins
test: fix common.expectsError
d7e536d 
The function should strictly test for the error class and only accept
the correct one. Any other error class should fail.

PR-URL: #13686
Fixes: #13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Jan 8, 2018
@refack @MylesBorins
test: add more asserts to test-internal-errors
015c146 
PR-URL: #13686
Fixes: #13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
@refack @MylesBorins
test: fix common.expectsError
95ff8f0 
The function should strictly test for the error class and only accept
the correct one. Any other error class should fail.

PR-URL: #13686
Fixes: #13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Jan 9, 2018
@refack @MylesBorins
test: add more asserts to test-internal-errors
fffb127 
PR-URL: #13686
Fixes: #13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit to targos/node that referenced this issue Mar 24, 2018
@refack @targos
test: add more asserts to test-internal-errors
b59f421 
PR-URL: nodejs#13686
Fixes: nodejs#13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit to targos/node that referenced this issue Mar 24, 2018
@refack @targos
test: fix common.expectsError
fd94671 
The function should strictly test for the error class and only accept
the correct one. Any other error class should fail.

PR-URL: nodejs#13686
Fixes: nodejs#13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit to targos/node that referenced this issue Mar 24, 2018
@BridgeAR @targos
test: fix wrong error classes passed in as type
8e5fd45 
PR-URL: nodejs#13686
Fixes: nodejs#13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue Mar 30, 2018
@refack @targos
test: add more asserts to test-internal-errors
cc68bc2 
Backport-PR-URL: #19579
PR-URL: #13686
Fixes: #13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue Mar 30, 2018
@refack @targos
test: fix common.expectsError
44b12c1 
The function should strictly test for the error class and only accept
the correct one. Any other error class should fail.

Backport-PR-URL: #19579
PR-URL: #13686
Fixes: #13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue Mar 30, 2018
@BridgeAR @targos
test: fix wrong error classes passed in as type
ca97be5 
Backport-PR-URL: #19579
PR-URL: #13686
Fixes: #13682
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@refack refack removed their assignment Oct 12, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
test Issues and PRs related to the tests. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix common.expectsError type checking refack/node
1 participant
@refack