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.

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: improve n-api array func coverage #12890

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
7 participants
@mhdawson
Copy link
Member

commented May 8, 2017

  • add coverage for napi_has_element
  • add coverage for napi_create_array_with_length
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows [commit guidelines]
Affected core subsystem(s)

test, n-api

test: improve n-api array func coverage
- add coverage for napi_has_element
- add coverage for napi_create_array_with_length
@mhdawson

This comment has been minimized.

Copy link
Member Author

commented May 8, 2017

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.

@@ -19,19 +19,26 @@ const array = [
]
];

assert.strictEqual(test_array.Test(array, array.length + 1),
assert.strictEqual(test_array.TestGetElement(array, array.length + 1),
'Index out of bound!');

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye May 8, 2017

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented May 8, 2017

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

I think the reasoning here was that the public API would consistently use size_t for all length parameters, but implementations should return errors for any lengths which are beyond the supported maximum.

@mhdawson

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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(
() => {

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

Does this arrow function fit on a single line?

This comment has been minimized.

Copy link
@mhdawson

mhdawson May 9, 2017

Author Member

I was keeping it consistent with what I saw most often in tests. For example: test/parallel/test-punycode.js.

() => {
test_array.TestGetElement(array, array.length + 1);
},
/Index out of bounds!/

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

Are you able to make this more strict? We've been moving toward adding ^, $, and the error type to the regular expression.

This comment has been minimized.

Copy link
@mhdawson

mhdawson May 9, 2017

Author Member

Oops missed that one, will add.

This comment has been minimized.

Copy link
@mhdawson

mhdawson May 9, 2017

Author Member

Also fixed up the other instance which was there from the existing version of the test.

});


assert.deepStrictEqual(test_array.New(array), array);

assert(test_array.TestHasElement(array, 0));
assert.strictEqual(false, test_array.TestHasElement(array, array.length + 1));

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

Can you switch the argument order to strictEqual().

This comment has been minimized.

Copy link
@mhdawson

mhdawson May 9, 2017

Author Member

will do.


assert(test_array.NewWithLength(0) instanceof Array);
assert(test_array.NewWithLength(1) instanceof Array);
assert(test_array.NewWithLength(2 ^ 32 - 1) instanceof Array);

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

Are you attempting to do exponentiation here?

This comment has been minimized.

Copy link
@jasnell

jasnell May 9, 2017

Member

This would need to be (2 ** 32 - 1)

This comment has been minimized.

Copy link
@mhdawson

mhdawson May 9, 2017

Author Member

hmm, maybe coding at 2am in your hotel room is not always the best idea. Will fix.

@mhdawson

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

@cjihrig pushed commit to address comments.

@mhdawson

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

@mhdawson

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

CI good landing.

@mhdawson

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

Landed as 654afa2

@mhdawson mhdawson closed this May 11, 2017

mhdawson added a commit that referenced this pull request May 11, 2017

test: improve n-api array func coverage
- add coverage for napi_has_element
- add coverage for napi_create_array_with_length

PR-URL: #12890
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

anchnk added a commit to anchnk/node that referenced this pull request May 19, 2017

test: improve n-api array func coverage
- add coverage for napi_has_element
- add coverage for napi_create_array_with_length

PR-URL: nodejs#12890
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@mhdawson mhdawson deleted the mhdawson:array-coverage branch Jun 28, 2017

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 12, 2018

test: improve n-api array func coverage
- add coverage for napi_has_element
- add coverage for napi_create_array_with_length

PR-URL: nodejs#12890
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018

test: improve n-api array func coverage
- add coverage for napi_has_element
- add coverage for napi_create_array_with_length

PR-URL: nodejs#12890
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018

test: improve n-api array func coverage
- add coverage for napi_has_element
- add coverage for napi_create_array_with_length

PR-URL: nodejs#12890
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Apr 16, 2018

test: improve n-api array func coverage
- 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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Apr 16, 2018

Merged

v6.14.2 proposal #19996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.