n-api: Reference and external tests by jasongin · Pull Request #12551 · nodejs/node · GitHub

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

@mhdawson

LGTM if the CI is happy

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

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

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.

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

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.

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

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

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

CI good landing

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 <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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

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.

@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();
+  }));
+}

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

PR-URL: #12864 Ref: #12551 (comment) Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs#12864 Ref: nodejs#12551 (comment) Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

- 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 <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs#12864 Ref: nodejs#12551 (comment) Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

- 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 <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Backport-PR-URL: #19447 PR-URL: #12864 Ref: #12551 (comment) Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

+              // 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);

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

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

n-api: Reference and external tests by jasongin · Pull Request #12551 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: Reference and external tests - #12551

n-api: Reference and external tests#12551jasongin wants to merge 2 commits intonodejs:masternodejs/node:masterfrom jasongin:reftestsjasongin/nodejs:reftestsCopy head branch name to clipboard

Conversation

jasongin commented Apr 20, 2017

Checklist

Affected core subsystem(s)

Uh oh!

jasongin commented Apr 20, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasongin commented Apr 27, 2017

Uh oh!

cjihrig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Apr 27, 2017

Uh oh!

mhdawson commented Apr 27, 2017

Uh oh!

mhdawson commented Apr 27, 2017

Uh oh!

targos commented Apr 28, 2017

Uh oh!

jasongin commented Apr 28, 2017

Uh oh!

targos commented May 5, 2017

Uh oh!

jasongin commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

n-api: Reference and external tests#12551
jasongin wants to merge 2 commits into
nodejs:masterfrom
jasongin:reftests

mhdawson left a comment •
edited
Loading

cjihrig left a comment •
edited
Loading