n-api: Reference and external tests #12551

jasongin · 2017-04-20T20:27:15Z

Add a test project to addons-napi that covers the N-API reference and external APIs
Fix a bug in napi_typeof() that was found by the new tests

This improves the code coverage of N-API by a few percent. See also #12219

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)

- Add a test project to addons-napi that covers the N-API reference and external APIs - Fix a bug in napi_typeof that was found by the new tests

jasongin · 2017-04-20T20:27:32Z

@mhdawson

addaleax

LGTM if the CI is happy

addaleax · 2017-04-20T23:47:41Z

test/addons-napi/test_reference/test.js

+assert.strictEqual(0, test_reference.finalizeCount);
+
+// External value without a finalizer
+let value = test_reference.createExternal();


Can you turn these groups into { … } blocks and use const where that works? That makes it a bit easier to see how different parts of a test (don’t) interact :)

const won't work because value must later be set to null in order to allow it to be garbage-collected.

... unless the scope of the { } blocks is narrowed to just around where each value is live. But then I think that would actually hurt readability because the blocks would not correspond to the logical test cases.

Also I think allowing value to go out of scope to enable GC is less obvious than assigning null.

I agree with @addaleax. Unless necessary for the purposes of the test, we've been moving toward using block scopes in the tests for better isolation.

@jasongin Yea, let seems fine here. You can still do both assigning null and using block scopes, if you think that’s more readable (I would say it is)

OK, I'm adding block scopes around each test case, keeping the null assignment within.

Fixed in caf86ed

cjihrig · 2017-04-25T14:33:14Z

test/addons-napi/test_reference/test.js

+const common = require('../../common');
+const assert = require('assert');
+
+const test_reference = require(`./build/${common.buildType}/test_reference`);


Can you stick to camelCase in JavaScript code.

The underscore matches the name of the module. I was trying to follow precedent in node.js JavaScript code, often with the child_process module, for example at https://github.com/nodejs/node/blob/master/test/common.js#L28

cjihrig · 2017-04-25T14:35:19Z

test/addons-napi/test_reference/test.js

+assert.strictEqual(0, test_reference.finalizeCount);
+
+// External value without a finalizer
+let value = test_reference.createExternal();


I agree with @addaleax. Unless necessary for the purposes of the test, we've been moving toward using block scopes in the tests for better isolation.

cjihrig · 2017-04-25T14:37:57Z

test/addons-napi/test_reference/test.js

+// This test script uses external values with finalizer callbacks
+// in order to track when values get garbage-collected. Each invocation
+// of a finalizer callback increments the finalizeCount property.
+assert.strictEqual(0, test_reference.finalizeCount);


Can you change the order of the arguments to actual, expected in strictEqual() throughout the test.

Yes, good catch. I'm used to some other testing frameworks that use the opposite order.

Fixed in caf86ed

- Make test cases more isolated with block scoping and reset finalize count - Fix order of actual, expected in asserts

mhdawson

LGTM, subject to fixing up Colin's comment. I've seen lots of issues with tests relying on triggering a gc in the Java world, but provided this passes consistently well worth running.

jasongin · 2017-04-27T16:51:35Z

I've responded to all the feedback. @cjihrig did you want to take another look?

cjihrig

I still think the JS code should camelCase, but ¯\_(ツ)_/¯

mhdawson · 2017-04-27T19:09:46Z

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

mhdawson · 2017-04-27T20:30:23Z

CI good landing

mhdawson · 2017-04-27T20:34:54Z

Landed as 4271254

- Add a test project to addons-napi that covers the N-API reference and external APIs - Fix a bug in napi_typeof that was found by the new tests PR-URL: #12551 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

targos · 2017-04-28T09:12:46Z

The new test fails on the canary branch:

$ ./node --expose-gc --napi-modules test/addons-napi/test_reference/test.js

assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: [External] === undefined
    at Object.<anonymous> (/home/mzasso/git/nodejs/node-canary/test/addons-napi/test_reference/test.js:44:10)
    at Module._compile (module.js:582:30)
    at Object.Module._extensions..js (module.js:593:10)
    at Module.load (module.js:516:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:618:10)
    at startup (bootstrap_node.js:144:16)
    at bootstrap_node.js:548:3

jasongin · 2017-04-28T19:12:19Z

Apparently the GC behavior related to weak-references changed in the newer version of V8 that's in the canary branch. @mhdawson's concern above about tests relying on triggering GC has been validated.

I found that this test case can be fixed in the canary branch by triggering the GC after a setImmediate() callback (just before the failed assertion), though I don't understand why that's required now. Oddly the other test cases also using weak references didn't have any problem. So to avoid refactoring to deal with the one asynchronous test case, that one can be moved to the end.

targos · 2017-05-05T09:17:46Z

@jasongin I'm trying to add this setImmediate callback but still get the same error:

diff --git a/test/addons-napi/test_reference/test.js b/test/addons-napi/test_reference/test.js
index ddfec58..645b852 100644
--- a/test/addons-napi/test_reference/test.js
+++ b/test/addons-napi/test_reference/test.js
@@ -34,19 +34,6 @@ assert.strictEqual(test_reference.finalizeCount, 0);
 }

 {
-  // Weak reference
-  let value = test_reference.createExternalWithFinalize();
-  assert.strictEqual(test_reference.finalizeCount, 0);
-  test_reference.createReference(value, 0);
-  assert.strictEqual(test_reference.referenceValue, value);
-  value = null;
-  global.gc(); // Value should be GC'd because there is only a weak ref
-  assert.strictEqual(test_reference.referenceValue, undefined);
-  assert.strictEqual(test_reference.finalizeCount, 1);
-  test_reference.deleteReference();
-}
-
-{
   // Strong reference
   let value = test_reference.createExternalWithFinalize();
   assert.strictEqual(test_reference.finalizeCount, 0);
@@ -85,3 +72,18 @@ assert.strictEqual(test_reference.finalizeCount, 0);
   global.gc(); // Value was already GC'd
   assert.strictEqual(test_reference.finalizeCount, 1);
 }
+
+{
+  // Weak reference
+  let value = test_reference.createExternalWithFinalize();
+  assert.strictEqual(test_reference.finalizeCount, 0);
+  test_reference.createReference(value, 0);
+  assert.strictEqual(test_reference.referenceValue, value);
+  value = null;
+  global.gc(); // Value should be GC'd because there is only a weak ref
+  setImmediate(common.mustCall(() => {
+    assert.strictEqual(test_reference.referenceValue, undefined);
+    assert.strictEqual(test_reference.finalizeCount, 1);
+    test_reference.deleteReference();
+  }));
+}

jasongin · 2017-05-05T16:18:23Z

@targos Try moving the gc() call into the setImmediate() callback.

PR-URL: #12864 Ref: #12551 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#12864 Ref: nodejs#12551 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

- Add a test project to addons-napi that covers the N-API reference and external APIs - Fix a bug in napi_typeof that was found by the new tests PR-URL: nodejs#12551 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#12864 Ref: nodejs#12551 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

- Add a test project to addons-napi that covers the N-API reference and external APIs - Fix a bug in napi_typeof that was found by the new tests Backport-PR-URL: #19447 PR-URL: #12551 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Backport-PR-URL: #19447 PR-URL: #12864 Ref: #12551 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

n-api: Reference and external tests

801c146

- Add a test project to addons-napi that covers the N-API reference and external APIs - Fix a bug in napi_typeof that was found by the new tests

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Apr 20, 2017

addaleax approved these changes Apr 20, 2017

View reviewed changes

cjihrig requested changes Apr 25, 2017

View reviewed changes

Updates for review feedback

caf86ed

- Make test cases more isolated with block scoping and reset finalize count - Fix order of actual, expected in asserts

mhdawson approved these changes Apr 25, 2017

View reviewed changes

cjihrig approved these changes Apr 27, 2017

View reviewed changes

mhdawson closed this Apr 27, 2017

jasongin deleted the reftests branch April 28, 2017 16:18

targos mentioned this pull request May 5, 2017

test: fix napi test_reference for recent V8 #12864

Closed

2 tasks

targos added a commit that referenced this pull request May 7, 2017

test: fix napi test_reference for recent V8

0a734fe

PR-URL: #12864 Ref: #12551 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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

test: fix napi test_reference for recent V8

ffa7a1d

PR-URL: nodejs#12864 Ref: nodejs#12551 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added dont-land-on-v4.x labels Jun 18, 2017

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

test: fix napi test_reference for recent V8

ce03977

Backport-PR-URL: #19447 PR-URL: #12864 Ref: #12551 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: Reference and external tests #12551

n-api: Reference and external tests #12551

jasongin commented Apr 20, 2017

jasongin commented Apr 20, 2017

addaleax left a comment

addaleax Apr 20, 2017

jasongin Apr 21, 2017

jasongin Apr 21, 2017

cjihrig Apr 25, 2017

addaleax Apr 25, 2017

jasongin Apr 25, 2017

jasongin Apr 25, 2017

cjihrig Apr 25, 2017

jasongin Apr 25, 2017

cjihrig Apr 25, 2017

cjihrig Apr 25, 2017

jasongin Apr 25, 2017

jasongin Apr 25, 2017

mhdawson left a comment •

edited

Loading

jasongin commented Apr 27, 2017

cjihrig left a comment •

edited

Loading

mhdawson commented Apr 27, 2017

mhdawson commented Apr 27, 2017

mhdawson commented Apr 27, 2017

targos commented Apr 28, 2017

jasongin commented Apr 28, 2017

targos commented May 5, 2017

jasongin commented May 5, 2017

n-api: Reference and external tests #12551

n-api: Reference and external tests #12551

Conversation

jasongin commented Apr 20, 2017

Checklist

Affected core subsystem(s)

jasongin commented Apr 20, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment • edited Loading

Choose a reason for hiding this comment

jasongin commented Apr 27, 2017

cjihrig left a comment • edited Loading

Choose a reason for hiding this comment

mhdawson commented Apr 27, 2017

mhdawson commented Apr 27, 2017

mhdawson commented Apr 27, 2017

targos commented Apr 28, 2017

jasongin commented Apr 28, 2017

targos commented May 5, 2017

jasongin commented May 5, 2017

mhdawson left a comment •

edited

Loading

cjihrig left a comment •

edited

Loading