feat: add spawnTest harness global by bavulapati · Pull Request #54 · nodejs/node-api-cts · GitHub
Skip to content

feat: add spawnTest harness global#54

Open
bavulapati wants to merge 9 commits into
nodejs:mainfrom
bavulapati:feat/spawn-test-global
Open

feat: add spawnTest harness global#54
bavulapati wants to merge 9 commits into
nodejs:mainfrom
bavulapati:feat/spawn-test-global

Conversation

@bavulapati

@bavulapati bavulapati commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

This will be used in test_exception and test_finalizer

@bavulapati bavulapati force-pushed the feat/spawn-test-global branch 2 times, most recently from 9adf4db to 2795471 Compare April 16, 2026 16:24
bavulapati added a commit to bavulapati/node-api-cts that referenced this pull request Apr 17, 2026
@bavulapati
use pr nodejs#54
876f7b5 
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
@bavulapati bavulapati mentioned this pull request Apr 17, 2026
Draft
@kraenhansen

kraenhansen commented Apr 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

legendecas
legendecas reviewed Apr 27, 2026
View reviewed changes

@legendecas legendecas left a comment

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 spawn is highly coupled with a specific runtime, like CLI options. Could this be a more generic helper?

@bavulapati bavulapati force-pushed the feat/spawn-test-global branch from 2795471 to 6d80b42 Compare May 29, 2026 15:57
@bavulapati

bavulapati commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

I don't expect all implementors to have multi-process support where the harness is able to spawn up sub-processes. The emnapi and React Native implementors in particular won't be straight forward, if possible at all.

We could add another flag to skip the tests which needs this? 🤔 But that would entail a lower coverage on the implementors which doesn't support this "harness feature", which is not great. There's also the option of potentially refactoring the tests which needs this, into not needing sub-process spawning. Got any other ideas?

@kraenhansen Agreed that emnapi/WASM and React Native may not support spawning. I don't think we can fully refactor it away, though: test_exception needs to observe a process actually terminating on an uncaught exception, and test_finalizer is about at-exit behavior — both inherently whole-process. So a capability flag seems more realistic than refactoring: implementors that can't spawn expose spawnTest as unavailable, and tests that need it call skipTest() — per-test and explicit, rather than a blanket skip flag. Happy to spec that out if that direction sounds right.

@bavulapati

bavulapati commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I think spawn is highly coupled with a specific runtime, like CLI options. Could this be a more generic helper?

@legendecas Good catch — nodeFlags was the runtime coupling. After making --expose-gc a baseline (the harness's own gc.js requires it) it had no internal caller anyway, so I've dropped it in
63a6db3. The test-facing contract is now just spawnTest(filePath, { cwd }) → { status, signal, stdout, stderr }; the Node-specific child setup (--expose-gc, the --import chain) stays hidden inside the implementor's child_process.js. Test files never touch CLI flags.

@legendecas legendecas linked an issue Jun 5, 2026 that may be closed by this pull request
Support per-test flags in the Node implementor harness #60
Open
@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Jun 5, 2026
bavulapati added 3 commits June 5, 2026 21:57
@bavulapati
Merge branch 'main' into feat/spawn-test-global
012af85
@bavulapati
Merge remote-tracking branch 'upstream/main' into feat/spawn-test-global
5d3b3f2 
# Conflicts:
#	eslint.config.js
#	implementors/node/tests.ts
@bavulapati
refactor: make spawnTest result runtime-agnostic
518b3e2 
Replace the POSIX-specific signal field with an aborted boolean computed
by the Node implementor, moving the abort exit-code knowledge out of
portable tests and into child_process.js.
@bavulapati

bavulapati commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@legendecas I've made the test-facing contract runtime-agnostic:

  • the result is now { status, aborted, stdout, stderr } — the POSIX-specific signal is replaced by an aborted boolean
  • the abort exit-code/signal knowledge (POSIX 128+N and Windows NTSTATUS) now lives in implementors/node/child_process.js, not in portable test files
  • there are no longer any runtime CLI flags in the signature (--expose-gc is applied internally; nodeFlags was dropped earlier)

So a test only asks "did it exit cleanly / abort?" and every Node-specific detail stays inside the Node implementor. PTAL 🙏

@bavulapati bavulapati requested a review from legendecas June 12, 2026 16:21
@bavulapati
fix: use pathToFileURL for harness --import paths
8131d45 
A bare 'file://' + path is malformed on Windows (file://C:\...).
Also surface aborts in the failure reason regardless of exit code.
legendecas
legendecas reviewed Jun 19, 2026
View reviewed changes
Comment thread implementors/node/child_process.js Outdated
legendecas
legendecas reviewed Jun 19, 2026
View reviewed changes
Comment thread implementors/node/child_process.js
@kraenhansen kraenhansen mentioned this pull request Jun 23, 2026
Open
@kraenhansen

kraenhansen commented Jun 23, 2026

Copy link
Copy Markdown
Member

What's the expected implementation of spawnTest for an implementor which doesn't support spawning? As per harness/spawn-test.js, they need to provide an implementation.

I think we should add a capability flag already now and have that harness spawn test skipped on that as well.

@bavulapati

bavulapati commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

What's the expected implementation of spawnTest for an implementor which doesn't support spawning? As per harness/spawn-test.js, they need to provide an implementation.

I think we should add a capability flag already now and have that harness spawn test skipped on that as well.

Reused the existing runtimeFeatures for this.

legendecas
legendecas reviewed Jun 26, 2026
View reviewed changes
Comment thread implementors/node/tests.ts Outdated

@legendecas legendecas Jun 26, 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'm a bit concerned that this buffered stdout would lead to nothing being printed for hanging tests.

@bavulapati bavulapati Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the runner now passes stdout: 'inherit' to spawnTest, so the child's stdout streams straight to the terminal as it runs (visible for slow/hanging tests) instead of being buffered until exit. stderr is still captured to attach to the failure message.

spawnTest keeps 'pipe' as the default, since test callers assert on the returned stdout — the runner is the one consumer that opts into streaming.

@bavulapati bavulapati requested a review from legendecas June 26, 2026 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legendecas legendecas Awaiting requested review from legendecas

At least 1 approving review is required to merge this pull request.

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.

Support per-test flags in the Node implementor harness

3 participants

@bavulapati @kraenhansen @legendecas