test: add coverage for napi_has_named_property#13178
test: add coverage for napi_has_named_property#13178mhdawson wants to merge 2 commits intonodejs:masterfrom
Conversation
Add test to cover napi_has_named_property
nodejs-github-bot
added
node-api
|
|
||
| // Extract the name of the property to check | ||
| char buffer[128]; | ||
| size_t buffer_size = 128; |
There was a problem hiding this comment.
I mean, this is obviously correct, but I get all tingly seeing this not just use sizeof(buffer). 😄
There was a problem hiding this comment.
Not an excuse but I copied from the existing code in another file, using sizeof does seem better so I'll move it over to that.
| NAPI_CALL(env, | ||
| napi_get_value_string_utf8(env, args[1], buffer, buffer_size - 1, &copied)); | ||
|
|
||
| // do the check and create the boolean retutn value |
| assert.strictEqual(test_object.readonlyAccessor2, 2); | ||
| assert.throws(() => { test_object.readonlyAccessor2 = 3; }, TypeError); | ||
|
|
||
| assert.ok(test_object.hasNamedProperty(test_object, 'echo')); |
There was a problem hiding this comment.
It might be better to use strictEqual() here to make sure we're actually getting a Boolean back.
| char buffer[128]; | ||
| size_t buffer_size = 128; | ||
| size_t copied; | ||
| buffer[buffer_size - 1] = 0; |
There was a problem hiding this comment.
napi_get_value_string_utf8() will always null-terminate, so this line should not be necessary.
|
Thanks for the comments, pushed commit to address. Will land once 48 hours has passed. |
|
Re-run on linux-one https://ci.nodejs.org/job/node-test-commit-linuxone/6151/. Original failure looks like infra issues as opposed to anything else. |
|
Failures on arm are both unrelated (both are failures of test-npm-install). Possible an infra failure at the time they were running. Going to lan. |
|
Should have noted re-run on linuxone was fine as well. Landed as 1961900 |
Add test to cover napi_has_named_property PR-URL: #13178 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Add test to cover napi_has_named_property PR-URL: #13178 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Add test to cover napi_has_named_property PR-URL: nodejs#13178 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Add test to cover napi_has_named_property Backport-PR-URL: #19447 PR-URL: #13178 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
5 participants
Add test to cover napi_has_named_property
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test, n-api