test: add coverage for napi_has_named_property#13178
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Add test to cover napi_has_named_property
nodejs-github-bot added node-api 
| // Extract the name of the property to check | ||
| charbuffer[128]; | ||
| size_tbuffer_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.
| charbuffer[128]; | ||
| size_tbuffer_size=128; | ||
| size_tcopied; | ||
| 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.
mhdawson commented May 24, 2017
Thanks for the comments, pushed commit to address. Will land once 48 hours has passed. |
mhdawson commented May 25, 2017
mhdawson commented May 25, 2017
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. |
mhdawson commented May 25, 2017
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. |
mhdawson commented May 25, 2017
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