-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: added test for lib/internal/util/inspector.js #24997
test: added test for lib/internal/util/inspector.js #24997
Conversation
Looks like you forgot the
(This all can be fixed by whoever lands the PR, but if you can fix it in the commit and force push it, all the better to save someone else a bit of editing.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common.mustCall()
does not take a string argument. If you're intention here is to throw an error, use () => { throw new Error('foo'); }
or something like that instead.
test/sequential/test-inspector-send-inspector-command-exception.js
Outdated
Show resolved
Hide resolved
test/sequential/test-inspector-send-inspector-command-exception.js
Outdated
Show resolved
Hide resolved
I'd prefer something that used the public APIs to trigger the error, but this will do in its absence! |
And thanks for the pull request! |
Test for root/internal/inspector.js to make 100% test coverage. Test checks that onError on line 19 is executed when exception is thrown
Pass function that will throw an exception when called. Remove 1 from common.mustCall() because 1 is a default.
@Trott Thanks for the review. I made the changes that you requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some relevant failures in CI. It's because the inspector tests all need to be skipped if node
is compiled without certain things. Can you add a line that calls common.skipIfInspectorDisabled();
like you'll see in the other test-inspector-*
files?
@IlarionHalushka - can you pls address the review comments? |
Test for lib/internal/util/inspector.js to make 100% test coverage.
Test checks that onError on line 19 is executed when exception is thrown
make -j4 test
(UNIX), orvcbuild test
(Windows) passes