FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: port 6_object_wrap test to CTS by kraenhansen · Pull Request #32 · nodejs/node-api-cts · GitHub

/ node-api-cts Public

feat: port 6_object_wrap test to CTS - #32

Open
kraenhansen wants to merge 5 commits into
nodejs:mainfrom
kraenhansen:feat/port-6-object-wrap
ConversationCommits (5)ChecksFiles changed
Open

feat: port 6_object_wrap test to CTS#32
kraenhansen wants to merge 5 commits into
nodejs:mainfrom
kraenhansen:feat/port-6-object-wrap

Conversation

kraenhansen commented Mar 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

Ports 6_object_wrap from the Node.js test suite into the CTS. This is the first test to exercise the experimental addon infrastructure (add_node_api_cts_experimental_addon()), serving as end-to-end validation of #31.

Three build targets:

  • myobject (stable) — object wrap with getters/setters, plusOne, multiply
  • myobject_basic_finalizer (experimental, NAPI_EXPERIMENTAL) — basic finalizer verification, guarded by experimentalFeatures.postFinalizer
  • nested_wrap (NAPI_VERSION=10) — nested napi_ref finalization

Three JS test files:

  • test.js — property descriptors, constructor/plain-function invocation, method chaining
  • test-basic-finalizer.js — experimental finalizer with gcUntil, behind feature guard
  • nested_wrap.js — nested wrap finalization with gcUntil

All C++ source files are copied verbatim from upstream.

Test plan

  • npm run addons:configure && npm run addons:build — all three targets compile
  • npm run node:test — all 58 tests pass (3 new)
  • C++ files diff clean against upstream

🤖 Generated with Claude Code

kraenhansen moved this from Need Triage to Has PR in Node-API Team Project Mar 8, 2026
kraenhansen marked this pull request as draft March 8, 2026 13:01

This comment was marked as outdated.

Sign in to view
kraenhansen force-pushed the feat/port-6-object-wrap branch from d57e9e9 to d833299 Compare March 28, 2026 19:33

kraenhansen commented Mar 28, 2026

Copy link
Copy Markdown
Member Author

CI fails on Node.js v20 — same root cause as #28. The nested_wrap addon is built with NAPI_VERSION=10 which isn't fully supported on v20 (see nodejs/node#55676).

Blocked on #37 (dropping Node v20 from CI).

kraenhansen moved this from Has PR to In Progress in Node-API Team Project Mar 28, 2026

bavulapati commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

We can use the napiVersion and skipTest primitives from #46 to skip this test when napi version < 10

This was referenced Apr 9, 2026
Closed
Merged
Merged
feat: port 6_object_wrap test to CTS
b3d558f 
Port all three build targets and test files from upstream:
- myobject (stable) — object wrap with getters/setters/methods
- myobject_basic_finalizer (experimental) — basic finalizer verification
- nested_wrap (NAPI_VERSION=10) — nested ref finalization

The experimental target (myobject_basic_finalizer) exercises the
add_node_api_cts_experimental_addon() CMake function for the first time,
serving as end-to-end validation of the experimental infrastructure.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kraenhansen force-pushed the feat/port-6-object-wrap branch from d833299 to b3d558f Compare April 25, 2026 20:35
bavulapati and others added 2 commits April 25, 2026 23:08
skip on napi version < 10
fa99289 
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
Merge pull request #2 from bavulapati/feat/port-test-6-object-wrap
dea425e 
use the napiVersion and skipTest primitives for porting test_6_object_wrap
kraenhansen marked this pull request as ready for review April 25, 2026 21:16
bavulapati approved these changes Apr 26, 2026
View reviewed changes

bavulapati left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Comment thread tests/js-native-api/6_object_wrap/test-basic-finalizer.js
assert.strictEqual(obj, null);
})();

await gcUntil('basic-finalizer', () => addon.getFinalizerCallCount() === 1);

bavulapati Apr 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's interesting to see why the original test is asserting the finalizerCallCount again!

assert.strictEqual(addon.getFinalizerCallCount(), 1);

kraenhansen Apr 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree - that's curious 🤔 Perhaps the git history tells a story? I'd be okay with deleting that in a follow-up PR once we start refactoring these tests.

bavulapati Apr 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we should keep it. It's to handle the case where the gcUntil loop finishes without satisfying the condition addon.getFinalizerCallCount() === 1.

bavulapati Apr 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

the assertion should fail in that case.

legendecas Jul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

gcUntil shouldn't finish without error when the predicate is not satisfied. That would be a bug in gcUntil. The defense here would be redundant.

bavulapati suggested changes Apr 26, 2026
View reviewed changes
Comment thread tests/js-native-api/6_object_wrap/test-basic-finalizer.js
assert.strictEqual(obj, null);
})();

await gcUntil('basic-finalizer', () => addon.getFinalizerCallCount() === 1);

bavulapati Apr 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we should keep it. It's to handle the case where the gcUntil loop finishes without satisfying the condition addon.getFinalizerCallCount() === 1.

bavulapati reviewed May 1, 2026
View reviewed changes
Comment thread tests/js-native-api/6_object_wrap/test-basic-finalizer.js
assert.strictEqual(obj, null);
})();

await gcUntil('basic-finalizer', () => addon.getFinalizerCallCount() === 1);

bavulapati May 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
await gcUntil('basic-finalizer', () => addon.getFinalizerCallCount() === 1);
await gcUntil('basic-finalizer', () => addon.getFinalizerCallCount() === 1);
assert.strictEqual(addon.getFinalizerCallCount(), 1);

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

legendecas legendecas approved these changes
+1 more reviewer
bavulapati bavulapati requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Node-API Team Project
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL