Skip to content

feat: port test_cannot_run_js to CTS#28

Open
kraenhansen wants to merge 3 commits intonodejs:mainfrom
kraenhansen:feat/port-test-cannot-run-js
Open

feat: port test_cannot_run_js to CTS#28
kraenhansen wants to merge 3 commits intonodejs:mainfrom
kraenhansen:feat/port-test-cannot-run-js

Conversation

@kraenhansen
Copy link
Copy Markdown
Member

@kraenhansen kraenhansen commented Mar 1, 2026

Summary

Ports test_cannot_run_js from node/test/js-native-api/ to the CTS.

  • Builds two addons from the same C source (test_cannot_run_js with NAPI_VERSION=10, test_pending_exception with NAPI_VERSION=9) to cover both code paths in the finalizer
  • Verifies that when a finalizer tries to access JS during environment shutdown, napi_get_named_property returns napi_cannot_run_js (v10+), napi_pending_exception (v9), or napi_ok if the event loop is still running

Upstream source: https://github.com/nodejs/node/tree/main/test/js-native-api/test_cannot_run_js

Test plan

  • npm run addons:configure && npm run addons:build — both addons compile cleanly
  • npm run node:test — all 56 tests pass

🤖 Generated with Claude Code

@kraenhansen
Copy link
Copy Markdown
Member Author

kraenhansen commented Mar 8, 2026

This fails on Node v20, we might want to just remove that from the test matrix as active support ended 22 Oct 2024?

@legendecas legendecas moved this from Need Triage to Todo in Node-API Team Project Mar 20, 2026
@legendecas legendecas moved this from Todo to In Progress in Node-API Team Project Mar 20, 2026
@kraenhansen kraenhansen force-pushed the feat/port-test-cannot-run-js branch from 91187a6 to a0f9aee Compare March 28, 2026 18:48
@kraenhansen kraenhansen marked this pull request as ready for review March 28, 2026 18:48
@kraenhansen
Copy link
Copy Markdown
Member Author

CI fails on Node.js v20 with SIGSEGV because Node-API version 10 is not fully supported on v20 — the fix landed in nodejs/node#55676 (v22.14.0+, never backported to v20).

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

@kraenhansen kraenhansen marked this pull request as draft March 28, 2026 19:53
@bavulapati
Copy link
Copy Markdown
Contributor

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

Builds two addons from the same C source with different NAPI_VERSION
defines (10 and 9) to verify that finalizers attempting to access JS
during shutdown receive napi_cannot_run_js or napi_pending_exception
(respectively), or napi_ok if the event loop is still running.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kraenhansen kraenhansen force-pushed the feat/port-test-cannot-run-js branch from a0f9aee to a590d2c Compare April 25, 2026 20:51
bavulapati and others added 2 commits April 25, 2026 23:12
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
use the napiVersion and skipTest primitives for porting test_cannot_run_js
@kraenhansen kraenhansen marked this pull request as ready for review April 25, 2026 21:25
@kraenhansen kraenhansen requested a review from legendecas April 25, 2026 21:25
Comment on lines +9 to +11
if (napiVersion < 10) skipTest();
const addon_v8 = loadAddon("test_pending_exception");
const addon_new = loadAddon("test_cannot_run_js");
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.

let's split this into two tests. so that we can only skip test_cannot_run_js when napiVersion < 10. We did the split similarly in #44

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.

I see we did that 🤔 My hope is that we can keep these tests very close (almost verbatim) to the original source as this makes it easier to automatically spot regressions in coverage.

And then have follow-up PRs with refactors, which are easy to revert in isolation.

I suggest that we create an issue to track splitting this or simply follow up with a PR splitting this once it merges. @legendecas would you be okay with that approach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants