-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: checks on napi factory wrap’s finalization #22612
Conversation
@nodejs/n-api PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure that we need this PR given that we already test for this in https://github.com/nodejs/node/blob/master/test/addons-napi/8_passing_wrapped/test.js.
Nevertheless, I don't have any strong objections to landing this. |
@legendecas I suppose for completeness you could instead, in https://github.com/nodejs/node/blob/master/test/addons-napi/8_passing_wrapped/test.js, change the storage of obj2 = null;
global.gc();
assert.strictEqual(addon.finalizeCount(), 2); That would round out that test nicely. |
@gabrielschulhof thanks for reviewing😁. amended. It's true that finalizer has been checked in 8_passing_wrapped, however it could be more reasonable to take more checks. |
Resumed CI as: https://ci.nodejs.org/job/node-test-pull-request/17016/ |
Resumed again as CI: https://ci.nodejs.org/job/node-test-pull-request/17017/ |
Another new CI: https://ci.nodejs.org/job/node-test-pull-request/17145/ |
Resumed CI as: https://ci.nodejs.org/job/node-test-pull-request/17152/ |
Landed in 1cee085. |
Expect closing #22396
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes