test: add coverage for error apis#12729
Conversation
Add coverage for N-API functions related to throwing and creating errors. A number of these are currently showing as not having any coverage in the nightly code coverage reports.
nodejs-github-bot
added
node-api
test/addons-napi/test_error/test.js
Outdated
|
|
||
| error = test_error.createTypeError(); | ||
| assert.ok(error instanceof TypeError, | ||
| 'expected error to be an instance of TypeeError'); |
| 'Non-error primitive correctly classed by napi_is_error'); | ||
|
|
||
| try { | ||
| test_error.throwExistingError(); |
There was a problem hiding this comment.
If throwExistingError() and similar calls below don't actually throw an error, then this test would incorrectly pass, right?
There was a problem hiding this comment.
Right. I'd suggest using assert.throws() to verify expected exceptions. It will make cleaner code anyway.
There was a problem hiding this comment.
Agreed. assert.throws() is a much better choice here.
| NAPI_CALL(env, napi_create_string_utf8(env, "existing error", -1, &message)); | ||
| NAPI_CALL(env, napi_create_error(env, message, &error)); | ||
| NAPI_CALL(env, napi_throw(env, error)); | ||
| return nullptr; |
There was a problem hiding this comment.
Just noting for the future: it would be excellent if these n-api thrown errors followed the same conventions as the internal/errors module. That is, include a code property and the [{code}]: in the name.
There was a problem hiding this comment.
The napi_create_error() and napi_throw_error() and similar APIs are intended to be used by native add-on modules to return and/or throw errors to JavaScript. So, it would be up to the native add-on code to supply meaningful values for the code property. But, N-API could encourage conformance to that convention by providing APIs such as napi_create_error_with_code(), napi_throw_error_with_code(). I'll open a new issue to track that.
test/addons-napi/test_error/test.js
Outdated
|
|
||
| assert.throws(() => { | ||
| test_error.throwExistingError(); | ||
| }, /Error: existing error/); |
There was a problem hiding this comment.
We've been moving toward adding ^ and $ to make the regular expression stricter.
|
Thanks for the review, pushed commit to address comments: CI run: https://ci.nodejs.org/job/node-test-pull-request/7837/ |
|
CI good landing. |
|
landed as 94a120c |
Add coverage for N-API functions related to throwing and creating errors. A number of these are currently showing as not having any coverage in the nightly code coverage reports. PR-URL: #12729 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add coverage for N-API functions related to throwing and creating errors. A number of these are currently showing as not having any coverage in the nightly code coverage reports. PR-URL: nodejs#12729 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add coverage for N-API functions related to throwing and creating errors. A number of these are currently showing as not having any coverage in the nightly code coverage reports. PR-URL: nodejs#12729 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add coverage for N-API functions related to throwing and creating errors. A number of these are currently showing as not having any coverage in the nightly code coverage reports. Backport-PR-URL: #19447 PR-URL: #12729 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
7 participants
Add coverage for N-API functions related to
throwing and creating errors. A number of these
are currently showing as not having any
coverage in the nightly code coverage reports.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test, n-api