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: add coverage for napi_typeof #13990

Closed
wants to merge 3 commits into from
Closed

Conversation

mhdawson
Copy link
Member

We had some, but not complete coverage indirectly through
other tests. Add test to validate it specifically and
covers cases that were not being covered.

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

We had some, but not complete coverage indirectly through
other tests.  Add test to validate it specifically and
covers cases that were not being covered.
@nodejs-github-bot nodejs-github-bot added node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Jun 29, 2017
@mhdawson mhdawson requested a review from jasongin June 30, 2017 13:39
@@ -34,3 +34,12 @@ assert.ok(test_general.testGetPrototype(baseObject) !==
// test version management functions
// expected version is currently 1
assert.strictEqual(test_general.testGetVersion(), 1);

[123, 'test string', function() {}, new Object(), true,
undefined, Symbol()].forEach((val) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small readability nit

[
  123,
  'test string',
  function() {},
  new Object(),
  true,
  undefined,
  Symbol()
].forEach( ... )


napi_value result;
if (object_type == napi_number) {
NAPI_CALL(env ,napi_create_number(env, 42, &result));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comma after env is misaligned.

napi_value args[1];
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));

napi_valuetype object_type;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something like argument_type would be more descriptive of what this variable is.


[123, 'test string', function() {}, new Object(), true,
undefined, Symbol()].forEach((val) => {
assert.strictEqual(typeof val, typeof (test_general.testNapiTypeof(val)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this test is specifically for napi_typeof(), I don't understand why test_general.testNapiTypeof() returns a value of that type instead of just a string representing the type. Then, this assertion could be:

assert.strictEqual(typeof val, test_general.testNapiTypeof(val));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would probably work as well. Just not what I can up with. I'll take a look as its probably an easy change.

@mhdawson
Copy link
Member Author

Pushed commit to address comments.

@@ -29,7 +29,7 @@ napi_value testGetVersion(napi_env env, napi_callback_info info) {
uint32_t version;
napi_value result;
NAPI_CALL(env, napi_get_version(env, &version));
NAPI_CALL(env ,napi_create_number(env, version, &result));
NAPI_CALL(env,napi_create_number(env, version, &result));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a space after the first comma.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

undefined,
Symbol()
].forEach((val) => {
assert.strictEqual(typeof val, test_general.testNapiTypeof(val));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. A tiny nit here and on line 52. I think the parameter order should be switched to reflect actual, expected

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that will update.

@mhdawson
Copy link
Member Author

mhdawson commented Jul 5, 2017

Pushed commit to address final comments

CI run: https://ci.nodejs.org/job/node-test-pull-request/8994/

@mhdawson
Copy link
Member Author

mhdawson commented Jul 5, 2017

Landed as 34cf8ad

@mhdawson mhdawson closed this Jul 5, 2017
mhdawson added a commit that referenced this pull request Jul 5, 2017
We had some, but not complete coverage indirectly through
other tests.  Add test to validate it specifically and
covers cases that were not being covered.

PR-URL: #13990
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Trott
Copy link
Member

Trott commented Jul 6, 2017

This test is failing consistently for me locally on macOS.

@Trott
Copy link
Member

Trott commented Jul 6, 2017

Running make test-addons-napi-clean fixed my issue. So, uh, yeah, never mind. Sorry for the noise.

addaleax pushed a commit that referenced this pull request Jul 11, 2017
We had some, but not complete coverage indirectly through
other tests.  Add test to validate it specifically and
covers cases that were not being covered.

PR-URL: #13990
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
We had some, but not complete coverage indirectly through
other tests.  Add test to validate it specifically and
covers cases that were not being covered.

PR-URL: #13990
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
We had some, but not complete coverage indirectly through
other tests.  Add test to validate it specifically and
covers cases that were not being covered.

PR-URL: #13990
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
We had some, but not complete coverage indirectly through
other tests.  Add test to validate it specifically and
covers cases that were not being covered.

PR-URL: nodejs#13990
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
We had some, but not complete coverage indirectly through
other tests.  Add test to validate it specifically and
covers cases that were not being covered.

Backport-PR-URL: #19447
PR-URL: #13990
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
@mhdawson mhdawson deleted the napi-cov30 branch September 30, 2019 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants