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.

Sign up for GitHub

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

Jump to bottom

test: add coverage for napi_typeof #13990

Closed
wants to merge 3 commits into from
Closed

test: add coverage for napi_typeof #13990

wants to merge 3 commits into from

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

@mhdawson
test: add coverage for napi_typeof
db99007 
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
jasnell
jasnell reviewed Jun 30, 2017
View reviewed changes
test/addons-napi/test_general/test.js Outdated
@@ -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( ... )

cjihrig
cjihrig reviewed Jun 30, 2017
View reviewed changes
test/addons-napi/test_general/test_general.c Outdated

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.

test/addons-napi/test_general/test_general.c Outdated
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.

test/addons-napi/test_general/test.js Outdated

[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.

cjihrig
cjihrig approved these changes Jun 30, 2017
View reviewed changes
test/addons-napi/test_general/test_general.c Outdated
@@ -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

cjihrig
cjihrig reviewed Jun 30, 2017
View reviewed changes
test/addons-napi/test_general/test.js Outdated
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
@mhdawson
test: add coverage for napi_typeof
34cf8ad 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member

Trott commented Jul 6, 2017 •
edited

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
@mhdawson @addaleax
test: add coverage for napi_typeof
eb0cddd 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@mhdawson @addaleax
test: add coverage for napi_typeof
2a91d59 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@mhdawson @Fishrock123
test: add coverage for napi_typeof
e479bff 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
@mhdawson
test: add coverage for napi_typeof
134d7a1 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@mhdawson @MylesBorins
test: add coverage for napi_typeof
7f126c2 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
@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
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@jasongin jasongin Awaiting requested review from jasongin

Assignees
No one assigned
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mhdawson @Trott @jasnell @cjihrig @MylesBorins @nodejs-github-bot