Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: add coverage for error apis #12729

Closed
wants to merge 3 commits into from
Closed

test: add coverage for error apis #12729

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86e5db7
test: add coverage for error apis
mhdawson Apr 28, 2017
694bcaa
squash: address first set of comments
mhdawson May 1, 2017
59d210d
squash: address next comments
mhdawson May 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 58 additions & 0 deletions test/addons-napi/test_error/test.js
View file
Open in desktop
Expand Up @@ -55,3 +55,61 @@ assert.strictEqual(test_error.checkError({}), false,
// Test that non-error primitive is correctly classed
assert.strictEqual(test_error.checkError('non-object'), false,
'Non-error primitive correctly classed by napi_is_error');

try {
test_error.throwExistingError();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If throwExistingError() and similar calls below don't actually throw an error, then this test would incorrectly pass, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I'd suggest using assert.throws() to verify expected exceptions. It will make cleaner code anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. assert.throws() is a much better choice here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, changing.

} catch (error) {
assert.ok(error instanceof Error,
'expected error to be an instance of Error');
assert.strictEqual(error.message,
'existing error',
'expected message to be "existing error"');
}

try {
test_error.throwError();
} catch (error) {
assert.ok(error instanceof Error,
'expected error to be an instance of Error');
assert.strictEqual(error.message,
'error',
'expected message to be "error"');
}

try {
test_error.throwRangeError();
} catch (error) {
assert.ok(error instanceof RangeError,
'expected error to be an instance of RangeError');
assert.strictEqual(error.message,
'range error',
'expected message to be "range error"');
}

try {
test_error.throwTypeError();
} catch (error) {
assert.ok(error instanceof TypeError,
'expected error to be an instance of TypeError');
assert.strictEqual(error.message,
'type error',
'expected message to be "type error"');
}

let error = test_error.createError();
assert.ok(error instanceof Error, 'expected error to be an instance of Error');
assert.strictEqual(error.message, 'error', 'expected message to be "error"');

error = test_error.createRangeError();
assert.ok(error instanceof RangeError,
'expected error to be an instance of RangeError');
assert.strictEqual(error.message,
'range error',
'expected message to be "range error"');

error = test_error.createTypeError();
assert.ok(error instanceof TypeError,
'expected error to be an instance of TypeeError');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeError

assert.strictEqual(error.message,
'type error',
'expected message to be "type error"');
55 changes: 55 additions & 0 deletions test/addons-napi/test_error/test_error.cc
View file
Open in desktop
Expand Up @@ -15,9 +15,64 @@ napi_value checkError(napi_env env, napi_callback_info info) {
return result;
}

napi_value throwExistingError(napi_env env, napi_callback_info info) {
napi_value message;
napi_value error;
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened nodejs/abi-stable-node#241

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell @cjihrig @jasongin pushed commit to address comments. There are lots of different ways of formatting the assert.throws throughout the existing tests. This seemed the most succinct for this case.

}

napi_value throwError(napi_env env, napi_callback_info info) {
NAPI_CALL(env, napi_throw_error(env, "error"));
return nullptr;
}

napi_value throwRangeError(napi_env env, napi_callback_info info) {
NAPI_CALL(env, napi_throw_range_error(env, "range error"));
return nullptr;
}

napi_value throwTypeError(napi_env env, napi_callback_info info) {
NAPI_CALL(env, napi_throw_type_error(env, "type error"));
return nullptr;
}

napi_value createError(napi_env env, napi_callback_info info) {
napi_value result;
napi_value message;
NAPI_CALL(env, napi_create_string_utf8(env, "error", -1, &message));
NAPI_CALL(env, napi_create_error(env, message, &result));
return result;
}

napi_value createRangeError(napi_env env, napi_callback_info info) {
napi_value result;
napi_value message;
NAPI_CALL(env, napi_create_string_utf8(env, "range error", -1, &message));
NAPI_CALL(env, napi_create_range_error(env, message, &result));
return result;
}

napi_value createTypeError(napi_env env, napi_callback_info info) {
napi_value result;
napi_value message;
NAPI_CALL(env, napi_create_string_utf8(env, "type error", -1, &message));
NAPI_CALL(env, napi_create_type_error(env, message, &result));
return result;
}

void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
napi_property_descriptor descriptors[] = {
DECLARE_NAPI_PROPERTY("checkError", checkError),
DECLARE_NAPI_PROPERTY("throwExistingError", throwExistingError),
DECLARE_NAPI_PROPERTY("throwError", throwError),
DECLARE_NAPI_PROPERTY("throwRangeError", throwRangeError),
DECLARE_NAPI_PROPERTY("throwTypeError", throwTypeError),
DECLARE_NAPI_PROPERTY("createError", createError),
DECLARE_NAPI_PROPERTY("createRangeError", createRangeError),
DECLARE_NAPI_PROPERTY("createTypeError", createTypeError),
};

NAPI_CALL_RETURN_VOID(env, napi_define_properties(
Expand Down