-
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: cover napi_get/set/has_named_property() #26947
test: cover napi_get/set/has_named_property() #26947
Conversation
40b2a69
to
e4005e5
Compare
Add test coverage for these N-APIs. PR-URL: nodejs#26947
e4005e5
to
48b9bfc
Compare
|
||
NAPI_ASSERT(env, argc >= 2, "Wrong number of arguments"); | ||
|
||
napi_valuetype valuetype0; |
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.
Nit: Perhaps it would be more consistent to name the NAPI variables like value_type0
instead of valuetype0
, like is done for other variables like key_length
above. Or is this intentionally done for the NAPI types?
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.
LGTM
Landed in baa54a5. |
Add test coverage for these N-APIs. PR-URL: #26947 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Add test coverage for these N-APIs.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes