test: add test case for send command error case - #31133
Conversation
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
Sorry, something went wrong.
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
Sorry, something went wrong.
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
Sorry, something went wrong.
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
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>
|
With #31159 landed, is this PR obsolete? @BridgeAR @antsmartian |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
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>
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>
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>
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>
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