test: add test case for send command error case#31133
Closed
antsmartian wants to merge 1 commit intonodejs:masterfrom
Closed
test: add test case for send command error case#31133antsmartian wants to merge 1 commit intonodejs:masterfrom
antsmartian wants to merge 1 commit intonodejs:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
Collaborator
lpinca
reviewed
Dec 30, 2019
antsmartian
force-pushed
the
test-inspector-error
branch
from
7a736d9 to
b6a73c1
Compare
Collaborator
lpinca
approved these changes
Dec 31, 2019
Trott
approved these changes
Dec 31, 2019
Member
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
Would be good if we could test this without --expose-internal but that might be difficult? @nodejs/inspector
BridgeAR
requested changes
Jan 1, 2020
Member
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
I don't think this is the right thing to do.
From the API perspective the passed through callback is a user defined callback. It should therefore not catch the error from inside the callback and instead end up as an uncaught exception or be handled by the "user".
The only way to fix this seems to be to completely remove the outer try / catch in lib/internal/util/inspector.js. // cc @targos
BridgeAR
added a commit
to BridgeAR/node
that referenced
this pull request
Jan 1, 2020
The API caught errors from inside of the users passed through callback. This never caused any issues, since this API is only used internally. Otherwise it would have potentially hidden bugs in user code. Refs: nodejs#31133
4 tasks
Trott
pushed a commit
that referenced
this pull request
Jan 4, 2020
The API caught errors from inside of the users passed through callback. This never caused any issues, since this API is only used internally. Otherwise it would have potentially hidden bugs in user code. Refs: #31133 PR-URL: #31159 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Member
|
With #31159 landed, is this PR obsolete? @BridgeAR @antsmartian |
Member
|
@Trott I would say so. The only thing that could be tested is if throwing an error in the callback ends up as uncaught exception. Such a test would be unusual though. @antsmartian I hope it's fine that I close this for that reason. |
targos
pushed a commit
that referenced
this pull request
Jan 6, 2020
The API caught errors from inside of the users passed through callback. This never caused any issues, since this API is only used internally. Otherwise it would have potentially hidden bugs in user code. Refs: #31133 PR-URL: #31159 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos
pushed a commit
that referenced
this pull request
Jan 14, 2020
The API caught errors from inside of the users passed through callback. This never caused any issues, since this API is only used internally. Otherwise it would have potentially hidden bugs in user code. Refs: #31133 PR-URL: #31159 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos
pushed a commit
that referenced
this pull request
Jan 14, 2020
The API caught errors from inside of the users passed through callback. This never caused any issues, since this API is only used internally. Otherwise it would have potentially hidden bugs in user code. Refs: #31133 PR-URL: #31159 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs
pushed a commit
that referenced
this pull request
Feb 6, 2020
The API caught errors from inside of the users passed through callback. This never caused any issues, since this API is only used internally. Otherwise it would have potentially hidden bugs in user code. Refs: #31133 PR-URL: #31159 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added test case for a error scenario, which wasn't covered before: https://coverage.nodejs.org/coverage-cc3f2b386c6ee34f/lib/internal/util/inspector.js.html#L21
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes