test: checks on napi factory wrap’s finalization#22612
test: checks on napi factory wrap’s finalization#22612legendecas wants to merge 1 commit intonodejs:masterfrom
Conversation
nodejs-github-bot
added
node-api
|
@nodejs/n-api PTAL |
gabrielschulhof
left a comment
gabrielschulhof
left a comment
There was a problem hiding this comment.
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. |
BridgeAR
added
the
author ready
|
Landed in 1cee085. |
Fixes: #22396 PR-URL: #22612 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
Fixes: #22396 PR-URL: #22612 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
Fixes: #22396 PR-URL: #22612 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
Fixes: #22396 PR-URL: #22612 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
5 participants
Expect closing #22396
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes