feat: port test_cannot_run_js to CTS - #28
Conversation
Sorry, something went wrong.
|
This fails on Node v20, we might want to just remove that from the test matrix as active support ended 22 Oct 2024? |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
We can use the napiVersion and skipTest primitives from #46 to skip this test when napi version < 10 |
Sorry, something went wrong.
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>
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
use the napiVersion and skipTest primitives for porting test_cannot_run_js
| if (napiVersion < 10) skipTest(); | ||
| const addon_v8 = loadAddon("test_pending_exception"); | ||
| const addon_new = loadAddon("test_cannot_run_js"); |
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
This test seems fairly simple to be split. I can see the point that we want to keep the test closely aligned with the original source. But given this is a fairly straightforward case to increase the test coverage, I would prefer updating the test.
The reason that in the original test that we don't guard the tests with versions strictly is that Node.js only test with the latest version. So this modification is dedicated to node-api-cts.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Summary
Ports test_cannot_run_js from node/test/js-native-api/ to the CTS.
Upstream source: https://github.com/nodejs/node/tree/main/test/js-native-api/test_cannot_run_js
Test plan
🤖 Generated with Claude Code