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: Make N-API weak-ref GC tests asynchronous #13121

Merged
merged 1 commit into from
May 30, 2017

Conversation

jasongin
Copy link
Member

One of the N-API weak-reference test cases already had to be made asynchronous to handle different behavior in a newer V8 version. (0a734fe) When porting N-API to Node-ChakraCore, we found more of the test cases needed similar treatment. (nodejs/node-chakracore#246) So to make thes tests more robust (and avoid having differences in the test code for Node-ChakraCore), I am refactoring the tests in this file to insert a setImmedate() callback before every call to gc() and assertions about the effects of the GC.

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

@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 May 19, 2017
@jasongin
Copy link
Member Author

@targos @mhdawson Please review.

value = null;
global.gc(); // Value should NOT be GC'd because there is a strong ref
assert.strictEqual(test_reference.finalizeCount, 0);
'External value without a finalizer',
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to make it an array of objects instead of doing the string check for the title ?
ex
{ title: 'External value without a finalizer', steps: [() => {...}, () => {...}, etc ]}

Copy link
Member Author

@jasongin jasongin May 19, 2017

Choose a reason for hiding this comment

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

That would work too, but would be more complicated to do the nested iteration recursively. I didn't see a need to over-design this just for these few tests. I can do it if you want. :)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking it would actually be easier/less code, but if you don't think that's the case I don't want to hold up getting this landed. I may take a look/PR later on when I have time if what I was thinking can simply, if not then I'll leave it be.

jasongin added a commit to jasongin/node-addon-api that referenced this pull request May 20, 2017
 - Make GC tests (arraybuffer, buffer) async, to account for different
   GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically
jasongin added a commit to jasongin/node-addon-api that referenced this pull request May 21, 2017
 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically
jasongin added a commit to jasongin/node-addon-api that referenced this pull request May 21, 2017
 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically.
 - Add missing entry for object tests in index.js.
 - Remove check for writable attribute on accessor property
   descriptors; it should not be there according to the JS spec.
 - Remove the explicit dependency on node-gyp in package.json.
   (NPM carries its own copy of node-gyp.)
title = tests[i];
runTests(i + 1, title, tests);
} else {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you replace the try...catch, throw, and console.error() call with a call to assert.doesNotThrow()?

Copy link
Member Author

@jasongin jasongin May 22, 2017

Choose a reason for hiding this comment

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

That could work, except assert.doesNotThrow() loses the stack trace of the original exception; it only displays the message. So I prefer not to use it except when checking for a very specific exceptional condition. In this case, the original exception stack trace is likely to point to a line number of another assertion in the file, so that's very helpful information.

jasongin added a commit to jasongin/node-addon-api that referenced this pull request May 23, 2017
 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically.
 - Add missing entry for object tests in index.js.
 - Remove check for writable attribute on accessor property
   descriptors; it should not be there according to the JS spec.
 - Remove the explicit dependency on node-gyp in package.json.
   (NPM carries its own copy of node-gyp.)
jasongin added a commit to nodejs/node-addon-api that referenced this pull request May 23, 2017
 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically.
 - Add missing entry for object tests in index.js.
 - Remove check for writable attribute on accessor property
   descriptors; it should not be there according to the JS spec.
 - Remove the explicit dependency on node-gyp in package.json.
   (NPM carries its own copy of node-gyp.)
 - Fix improper rethrow of an Error caught by reference that caused
   a double napi_ref delete, which failed in release builds of
   Node-ChakraCore.
@jasongin
Copy link
Member Author

@cjihrig @mhdawson Do you think changes are needed or is this good?

@jasongin
Copy link
Member Author

@cjihrig cjihrig dismissed their stale review May 24, 2017 18:10

Review dismissed

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

One of the N-API weak-reference test cases already had to be made
asynchronous to handle different behavior in a newer V8 version:
nodejs#12864
When porting N-API to Node-ChakraCore, we found more of the test
cases needed similar treatment:
nodejs/node-chakracore#246 So to make
thes tests more robust (and avoid having differences in the test code
for Node-ChakraCore), I am refactoring the tests in this file to
insert a `setImmedate()` callback before every call to `gc()` and
assertions about the effects of the GC.

PR-URL: nodejs#13121
Reviewed-By: Michael Dawson <[email protected]>
@jasongin
Copy link
Member Author

Landed as bb91879

@jasongin jasongin closed this May 30, 2017
@jasongin jasongin merged commit bb91879 into nodejs:master May 30, 2017
@jasongin jasongin deleted the reference_test_gc branch May 30, 2017 20:00
jasnell pushed a commit that referenced this pull request Jun 5, 2017
One of the N-API weak-reference test cases already had to be made
asynchronous to handle different behavior in a newer V8 version:
#12864
When porting N-API to Node-ChakraCore, we found more of the test
cases needed similar treatment:
nodejs/node-chakracore#246 So to make
thes tests more robust (and avoid having differences in the test code
for Node-ChakraCore), I am refactoring the tests in this file to
insert a `setImmedate()` callback before every call to `gc()` and
assertions about the effects of the GC.

PR-URL: #13121
Reviewed-By: Michael Dawson <[email protected]>
@jasnell jasnell mentioned this pull request Jun 5, 2017
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jun 15, 2017
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jun 15, 2017
Fixed by upstream PR nodejs/node#13121

Resolves nodejs#246

PR-URL: nodejs#301
Reviewed-By: Kunal Pathak <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jun 15, 2017
Fixed by upstream PR nodejs/node#13121

Resolves nodejs#246

PR-URL: nodejs#301
Reviewed-By: Kunal Pathak <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
One of the N-API weak-reference test cases already had to be made
asynchronous to handle different behavior in a newer V8 version:
nodejs#12864
When porting N-API to Node-ChakraCore, we found more of the test
cases needed similar treatment:
nodejs/node-chakracore#246 So to make
thes tests more robust (and avoid having differences in the test code
for Node-ChakraCore), I am refactoring the tests in this file to
insert a `setImmedate()` callback before every call to `gc()` and
assertions about the effects of the GC.

PR-URL: nodejs#13121
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
One of the N-API weak-reference test cases already had to be made
asynchronous to handle different behavior in a newer V8 version:
#12864
When porting N-API to Node-ChakraCore, we found more of the test
cases needed similar treatment:
nodejs/node-chakracore#246 So to make
thes tests more robust (and avoid having differences in the test code
for Node-ChakraCore), I am refactoring the tests in this file to
insert a `setImmedate()` callback before every call to `gc()` and
assertions about the effects of the GC.

Backport-PR-URL: #19447
PR-URL: #13121
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically.
 - Add missing entry for object tests in index.js.
 - Remove check for writable attribute on accessor property
   descriptors; it should not be there according to the JS spec.
 - Remove the explicit dependency on node-gyp in package.json.
   (NPM carries its own copy of node-gyp.)
 - Fix improper rethrow of an Error caught by reference that caused
   a double napi_ref delete, which failed in release builds of
   Node-ChakraCore.
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically.
 - Add missing entry for object tests in index.js.
 - Remove check for writable attribute on accessor property
   descriptors; it should not be there according to the JS spec.
 - Remove the explicit dependency on node-gyp in package.json.
   (NPM carries its own copy of node-gyp.)
 - Fix improper rethrow of an Error caught by reference that caused
   a double napi_ref delete, which failed in release builds of
   Node-ChakraCore.
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically.
 - Add missing entry for object tests in index.js.
 - Remove check for writable attribute on accessor property
   descriptors; it should not be there according to the JS spec.
 - Remove the explicit dependency on node-gyp in package.json.
   (NPM carries its own copy of node-gyp.)
 - Fix improper rethrow of an Error caught by reference that caused
   a double napi_ref delete, which failed in release builds of
   Node-ChakraCore.
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically.
 - Add missing entry for object tests in index.js.
 - Remove check for writable attribute on accessor property
   descriptors; it should not be there according to the JS spec.
 - Remove the explicit dependency on node-gyp in package.json.
   (NPM carries its own copy of node-gyp.)
 - Fix improper rethrow of an Error caught by reference that caused
   a double napi_ref delete, which failed in release builds of
   Node-ChakraCore.
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