test: improve n-api array func coverage#12890
Uh oh!
There was an error while loading. Please reload this page.
Conversation
- add coverage for napi_has_element - add coverage for napi_create_array_with_length
nodejs-github-bot added node-api 
The other thing I noticed while adding these tests is that we should probably change size_t for the array length in napi_create_array_with_length to a uint32_t since I think that's the maximum allowed, however I've deferred doing that to a later PR. |
test/addons-napi/test_array/test.js Outdated
| assert.strictEqual(test_array.Test(array,array.length+1), | ||
| assert.strictEqual(test_array.TestGetElement(array,array.length+1), | ||
| 'Index out of bound!'); |
There was a problem hiding this comment.
Totally unrelated to this PR, but just wanted to say that this implementation doesn't look correct to me. "Index out of bound!" should be thrown as an Error. As it is, "Index out of bound!" could be a value in the Array.
jasongin commented May 8, 2017
I think the reasoning here was that the public API would consistently use |
mhdawson commented May 9, 2017
@jasongin based on that explanation we can just leave as is then, although I'll probably check at some point later if that is consistent with how size_t is used elsewhere, particularly when the bit size can be used to help indicate the maximum range. |
mhdawson commented May 9, 2017
@thefourtheye fixed up that part of the test. |
| assert.strictEqual(test_array.Test(array,array.length+1), | ||
| 'Index out of bound!'); | ||
| assert.throws( | ||
| ()=>{ |
There was a problem hiding this comment.
Does this arrow function fit on a single line?
There was a problem hiding this comment.
I was keeping it consistent with what I saw most often in tests. For example: test/parallel/test-punycode.js.
test/addons-napi/test_array/test.js Outdated
| ()=>{ | ||
| test_array.TestGetElement(array,array.length+1); | ||
| }, | ||
| /Indexoutofbounds!/ |
There was a problem hiding this comment.
Are you able to make this more strict? We've been moving toward adding ^, $, and the error type to the regular expression.
There was a problem hiding this comment.
Oops missed that one, will add.
There was a problem hiding this comment.
Also fixed up the other instance which was there from the existing version of the test.
test/addons-napi/test_array/test.js Outdated
| assert.deepStrictEqual(test_array.New(array),array); | ||
| assert(test_array.TestHasElement(array,0)); | ||
| assert.strictEqual(false,test_array.TestHasElement(array,array.length+1)); |
There was a problem hiding this comment.
Can you switch the argument order to strictEqual().
test/addons-napi/test_array/test.js Outdated
| assert(test_array.NewWithLength(0)instanceofArray); | ||
| assert(test_array.NewWithLength(1)instanceofArray); | ||
| assert(test_array.NewWithLength(2^32-1)instanceofArray); |
There was a problem hiding this comment.
Are you attempting to do exponentiation here?
There was a problem hiding this comment.
hmm, maybe coding at 2am in your hotel room is not always the best idea. Will fix.
mhdawson commented May 9, 2017
@cjihrig pushed commit to address comments. |
mhdawson commented May 10, 2017
mhdawson commented May 11, 2017
CI good landing. |
mhdawson commented May 11, 2017
Landed as 654afa2 |
- add coverage for napi_has_element - add coverage for napi_create_array_with_length PR-URL: #12890 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
- add coverage for napi_has_element - add coverage for napi_create_array_with_length PR-URL: nodejs#12890 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
- add coverage for napi_has_element - add coverage for napi_create_array_with_length PR-URL: nodejs#12890 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
- add coverage for napi_has_element - add coverage for napi_create_array_with_length Backport-PR-URL: #19447 PR-URL: #12890 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test, n-api