Skip to content

feat: port test_exception to CTS#43

Open
bavulapati wants to merge 1 commit into
nodejs:mainfrom
bavulapati:feat/port-test-exception
Open

feat: port test_exception to CTS#43
bavulapati wants to merge 1 commit into
nodejs:mainfrom
bavulapati:feat/port-test-exception

Conversation

@bavulapati

Copy link
Copy Markdown
Contributor

ports
test_exception from Node.js test suite to the CTS.

@bavulapati

Copy link
Copy Markdown
Contributor Author

@kraenhansen This turned out to be tricky, as this needs to verify an uncaughtException. Thus, forcing another level of process spawning to verify.

@kraenhansen

kraenhansen commented Mar 30, 2026

Copy link
Copy Markdown
Member

This one is tricky, as I don't expect implementors such as emnapi and React Native to support this from their harness implementation 🤔

@bavulapati

Copy link
Copy Markdown
Contributor Author

@kraenhansen A priority list would have helped. I'm just picking up from the list sequentially.

@bavulapati

Copy link
Copy Markdown
Contributor Author

depends on #54

@kraenhansen

Copy link
Copy Markdown
Member

@bavulapati similarly to this comment I suggest moving this into draft until the parent PR merge.

@kraenhansen
kraenhansen marked this pull request as draft April 26, 2026 08:30
@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Jun 5, 2026
@bavulapati
bavulapati force-pushed the feat/port-test-exception branch from 4dfac05 to 94d6582 Compare June 29, 2026 12:35
@bavulapati
bavulapati force-pushed the feat/port-test-exception branch from 94d6582 to fc17c54 Compare July 14, 2026 08:10
@bavulapati
bavulapati marked this pull request as ready for review July 14, 2026 08:12
@@ -0,0 +1,146 @@
'use strict';
// Flags: --expose-gc

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.

Not needed.

Suggested change
// Flags: --expose-gc

@@ -0,0 +1,9 @@
// An exception thrown from a C finalizer during GC surfaces as an uncaught
// exception on stderr; it runs in a spawned child so the crash doesn't take

@legendecas legendecas Jul 17, 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.

I think we should rewrite the test with onUncaughtException in the harness.

But it seems like this test duplicates with tests/js-native-api/test_reference/test_finalizer.js. Maybe the main difference is that the exception is thrown during addon init. So maybe we could keep the test with onUncaughtException.

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