test: add coverage for napi_typeof#13990
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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 added node-api 
| assert.strictEqual(test_general.testGetVersion(),1); | ||
| [123,'test string',function(){},newObject(),true, | ||
| undefined,Symbol()].forEach((val)=>{ |
There was a problem hiding this comment.
Small readability nit
[123,'test string',function(){},newObject(),true,undefined,Symbol()].forEach( ... )
| napi_valueresult; | ||
| if (object_type==napi_number){ | ||
| NAPI_CALL(env ,napi_create_number(env, 42, &result)); |
There was a problem hiding this comment.
The comma after env is misaligned.
| napi_valueargs[1]; | ||
| NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL)); | ||
| napi_valuetypeobject_type; |
There was a problem hiding this comment.
I think something like argument_type would be more descriptive of what this variable is.
| [123,'test string',function(){},newObject(),true, | ||
| undefined,Symbol()].forEach((val)=>{ | ||
| assert.strictEqual(typeofval,typeof(test_general.testNapiTypeof(val))); |
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
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 commented Jun 30, 2017
Pushed commit to address comments. |
| napi_valueresult; | ||
| 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)); |
There was a problem hiding this comment.
Missing a space after the first comma.
| undefined, | ||
| Symbol() | ||
| ].forEach((val)=>{ | ||
| assert.strictEqual(typeofval,test_general.testNapiTypeof(val)); |
There was a problem hiding this comment.
Oh. A tiny nit here and on line 52. I think the parameter order should be switched to reflect actual, expected
There was a problem hiding this comment.
Thanks for catching that will update.
mhdawson commented Jul 5, 2017
Pushed commit to address final comments CI run: https://ci.nodejs.org/job/node-test-pull-request/8994/ |
mhdawson commented Jul 5, 2017
Landed as 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
This test is failing consistently for me locally on macOS. |
Trott commented Jul 6, 2017
Running |
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]>
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]>
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]>
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]>
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]>
6 participants
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), orvcbuild test(Windows) passesAffected core subsystem(s)
test, n-api