GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — test_runner: avoid reading process state directly in run() by murataslan1 · Pull Request #62377 · nodejs/node

Conversation

murataslan1 commented Mar 21, 2026

Copy link

Copy Markdown

Refs: #53867

Summary

The run() function is exposed as a public API via node:test, but it accessed process.argv, process.execArgv, process.cwd(), and process.env directly. This meant programmatic API users could not fully control the test runner behavior.

PR #54705 added cwd as an option but other process state references remained (as noted by @cjihrig).

Changes

Capture process.argv, process.cwd(), process.env, and process.execArgv in the CLI entry point (lib/internal/main/test_runner.js) and pass them explicitly to run() as options
Add processExecArgv option for V8 flag propagation to child processes
Replace process.env fallback in runTestFile() with opts.env
Replace process.argv usage in getRunArgs() with globPatterns
Replace process.env.NODE_TEST_CONTEXT check with env option
Maintain backwards compatibility: when options are not provided, run() falls back to process state as before

Test plan

Existing test runner tests should pass without changes
Programmatic run() calls now respect explicitly provided env, cwd, processExecArgv options instead of reading from process globals

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          test_runner: avoid reading process state directly in run()

e1746a9

Refs: nodejs#53867

The run() function is exposed as a public API via node:test module.
Previously, it accessed process.argv, process.execArgv, process.cwd(),
and process.env directly, which meant programmatic API users could not
fully control the test runner behavior.

This change:
- Captures process.argv, process.cwd(), process.env, and
  process.execArgv in the CLI entry point (main/test_runner.js) and
  passes them explicitly to run() as options
- Adds a processExecArgv option for V8 flag propagation
- Replaces process.env fallback in runTestFile() with opts.env
- Replaces process.argv usage in getRunArgs() with globPatterns
- Replaces process.env.NODE_TEST_CONTEXT check with env option
- Maintains backwards compatibility: when options are not provided,
  run() falls back to process state as before

Copilot AI review requested due to automatic review settings

March 21, 2026 23:34

nodejs-github-bot commented Mar 21, 2026

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/test_runner

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added needs-ci

PRs that need a full CI run.

test_runner Issues and PRs related to the test runner subsystem. labels

Mar 21, 2026

Copilot started reviewing on behalf of murataslan1

March 21, 2026 23:35

View session

Copilot AI reviewed

Mar 21, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Node.js test runner so the public node:test run() API can be controlled programmatically without implicitly reading from process globals (argv/cwd/env/execArgv), while keeping backwards-compatible fallbacks.

Changes:

Capture process.argv, process.cwd(), process.env, and process.execArgv in the CLI entry point and pass them into run() as options.
Add/propagate processExecArgv for forwarding V8 flags to child processes.
Replace remaining direct reads of process.argv/process.env in runner internals with option-based values.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
lib/internal/test_runner/runner.js	Adds option plumbing/defaults for process state, uses passed-in argv/env/execArgv equivalents when spawning child test processes.
lib/internal/main/test_runner.js	Captures process state at the CLI entry point and passes it down via `run()` options.

Comments suppressed due to low confidence (1)

lib/internal/test_runner/runner.js:787

run() now treats any defined env as user-provided, and rejects it when isolation === 'none'. Because the CLI entry point always passes options.env = process.env, node --test --test-isolation=none will now throw. Consider adjusting the "user provided" detection (e.g., distinguish internal captured env from an explicit API override) or relaxing the isolation='none' restriction for the default/inherited env case.

  if (userProvidedEnv) {
    validateObject(env);

    if (isolation === 'none') {
      throw new ERR_INVALID_ARG_VALUE('options.env', env, 'is not supported with isolation=\'none\'');
    }
  }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/test_runner/runner.js Outdated

Comment on lines 170 to 172

		processExecArgv,
		globPatterns,
		rerunFailuresFilePath,

Copilot AI Mar 21, 2026

Copy link

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

globPatterns is forwarded into runArgs via ArrayPrototypePushApply() (so it must be an array of strings). Currently run() only ensures it's an array; consider validating its element types (e.g., string array) to avoid child-spawn argument errors caused by non-string entries.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Copilot uses AI. Check for mistakes.

Comment thread

lib/internal/main/test_runner.js Outdated

		// Capture process state explicitly so run() does not need to access it.
		options.globPatterns = ArrayPrototypeSlice(process.argv, 1);
		options.cwd = process.cwd();
		options.env = process.env;

Copilot AI Mar 21, 2026

Copy link

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

options.env is being set unconditionally, which makes run() think the caller explicitly provided an env option. With --test-isolation=none, run() currently rejects options.env, so this change will break node --test --test-isolation=none. Consider only setting/passing options.env in the CLI when isolation='process' (or update run() to treat this captured env as non-user-provided).

Suggested change

	options.env = process.env;
	if (options.isolation === 'process') {
	options.env = process.env;
	}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Copilot uses AI. Check for mistakes.

Comment thread

lib/internal/test_runner/runner.js

Comment on lines 654 to 657

		argv = [],
		cwd = process.cwd(),
		cwd,
		processExecArgv,
		rerunFailuresFilePath,

Copilot AI Mar 21, 2026

Copy link

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

processExecArgv is consumed by getRunArgs() as an array (it is filtered and iterated), but there is no validation for the options.processExecArgv type. If a public API caller passes a non-array value, this will fail at runtime in less obvious ways. Consider validating it (e.g., validateStringArray(processExecArgv, 'options.processExecArgv')) when it is explicitly provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Copilot uses AI. Check for mistakes.

avivkeller requested changes

Mar 21, 2026

View reviewed changes

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

avivkeller commented Mar 21, 2026

Copy link

Copy Markdown

Member

Blocking as this needs documentation and tests

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This comment was marked as off-topic.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          test_runner: address review feedback

acf2faa

- Remove processExecArgv option: process.execArgv is runtime state
  that should not be a public API option. Keep reading it directly
  in getRunArgs() as before.
- Fix env with isolation=none: only set options.env from CLI entry
  point when isolation is not 'none', preventing ERR_INVALID_ARG_VALUE
  when using --test-isolation=none.
- Add documentation for the env option in doc/api/test.md.
- Add tests for env option acceptance and env+isolation=none rejection.

murataslan1 commented Mar 22, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Author

Pushed fixes based on the feedback:

Dropped processExecArgv entirely — doesn't belong in the public API, reverted to reading process.execArgv directly
Fixed the env + isolation=none issue — only passing env from CLI when isolation is process-based
Added docs for the env option and two tests (env acceptance + env rejection with isolation=none)

Let me know if anything else needs changing!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          test_runner: validate globPatterns as string array

37fc25a

murataslan1 commented Apr 1, 2026

Copy link

Copy Markdown

Author

tightened globPatterns validation — now uses validateStringArray for consistency with argv/execArgv. the env+isolation=none edge case was already handled in the earlier commits (cli only sets env when isolation isn't none, and runner.js tracks userProvidedEnv separately).

think this should be good to go now, lmk if anything else needs attention

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina approved these changes

Jul 12, 2026

View reviewed changes

mcollina left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Jul 12, 2026

mcollina requested changes

Jul 12, 2026

View reviewed changes

Comment thread

doc/api/test.md

mcollina Jul 12, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this doc change needs a history entry like other changes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coderabbitai Bot mentioned this pull request

Jul 25, 2026

Switch build, deployment, and operations to Node.js and remove Deno hackers-pub/hackerspub#371

Merged

qodo-code-review Bot mentioned this pull request

Aug 31, 2026

test(api): capture Signature B's mechanism, and rule out every in-repo cause sayed710/rocky#23

Merged

Reviewers

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

mcollina

mcollina requested changes

avivkeller

avivkeller requested changes

+1 more reviewer

samueldreadfulwater3-hash

samueldreadfulwater3-hash left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci

PRs that need a full CI run.

request-ci

Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review.

test_runner

Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

murataslan1 commented Mar 21, 2026

Summary

Changes

Test plan

Uh oh!

nodejs-github-bot commented Mar 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

avivkeller commented Mar 21, 2026

Uh oh!

This comment was marked as off-topic.

Uh oh!

murataslan1 commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murataslan1 commented Apr 1, 2026

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

murataslan1 commented Mar 22, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.