doc: correct concurrency wording in test() documentation

[azadgupta1]

This PR updates the test runner documentation for the concurrency option.

• Replaces “application thread” with “test process”
• Clarifies that concurrency is achieved using child processes, not threads
• Changes “parallel” to “concurrent” for accuracy

As discussed in #60721.

Requesting reviews from @Ethan-Arrowood and @cjihrig.

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[aduh95]

• Clarifies that concurrency is achieved using child processes, not threads

I don't think it clarifies, specifying concurrency only affects the current thread, IMO talking about "process" muddies the water.

[JakobJingleheimer]

It's also not always correct: isolation none runs the tests in the main process (instead of a dedicated test process).

"Thread" is indeed wrong though, and that affects shared globals, so maybe that should be corrected. I think there may be a circumstance where it uses threads instead of processes though, but that may not be the case (anymore?). I'd have to check.

[aduh95]

Is there any case where test({ concurrency: true }, …) would result in creating multiple threads / processes? I don't think that's a thing, I feel like we're talking about different things.

[JakobJingleheimer]

Is there any case where test({ concurrency: true }, …) would result in creating multiple threads / processes?

Processes: no, never (each test file gets its own process though)

Threads: I believe it uses Promise.allSettled when concurrency: true (when concurrency: number, I think it uses threads)

[aduh95]

Using Promise.allSettled would make more sense, because that runs in the same thread. Test files written in JS are no different for other JS files: if making a JS file multi-threaded was as easy as setting a boolean value, we would not reserve it only to tests.

[Ethan-Arrowood]

I think its just a queue of promises

node/lib/internal/test_runner/test.js

Lines 989 to 1000 in 0b6ae6d

	// If there is enough available concurrency to run the test now, then do
	// it. Otherwise, return a Promise to the caller and mark the test as
	// pending for later execution.
	this.parent.unfinishedSubtests.add(this);
	this.reporter.enqueue(this.nesting, this.loc, this.name, this.reportedType);
	if (this.root.harness.buildPromise || !this.parent.hasConcurrency()) {
	const deferred = PromiseWithResolvers();
	deferred.test = this;
	this.parent.addPendingSubtest(deferred);
	return deferred.promise;
	}

[Ethan-Arrowood]

And to be clear we are explicitly talking about the concurrency option for a suite or test function. Such as suite('foo', { concurrency: number | boolean }, () => {}); or test('bar', { concurrency: number | boolean }, () => {});

The concurrency option for run() or the CLI args does use processes.

[JakobJingleheimer]

And to be clear we are explicitly talking about the concurrency option for a suite or test function.

I know 🙂

It would probably be easiest to find the original PR that introduced it (there was specific discussion around Boolean vs number, so the answer will be there).

[aduh95]

IMO if we think talking about "thread" is bad because there's only one thread, it makes no sense to replace it with "process", for the same reason. Maybe we can find an alternative wording, e.g.:

Suggested change

	then that many tests would run concurrently within the test process.
	then that many tests would run asynchronously (they are still managed by the single-threaded event loop).

[Ethan-Arrowood]

IMO if we think talking about "thread" is bad because there's only one thread, it makes no sense to replace it with "process", for the same reason.

I better understand your reasoning now. I think your proposed wording is much clearer.

[azadgupta1]

Thanks for the feedback!

I see your point. I want to make the wording correct, but I'm not fully sure what the best replacement would be. Could you suggest the wording you think fits here? I’ll update the PR accordingly.

[Ethan-Arrowood]

this lgtm and I believe is a more accurate wording for how the test runner actually works.

[aduh95]

Adding an explicit -1 since this has an approval.

I disagree with using concurrently within the test process, if anything that makes it sound like there would be threads involved, at least the current wording is clearer that this isn't the case. I have a suggestion in #60773 (comment), I'm open to alternatives to unblock this

[aduh95]

We should probably also get rid of the other mentions of "parallel" (non blocking, can also happen in a follow up PR)

Suggested change

	If `true`, it would run all subtests in parallel.
	If `true`, it would run all subtests asynchronously.

or

Suggested change

	If `true`, it would run all subtests in parallel.
	If `true`, it would run all subtests at once.

[Ethan-Arrowood]

I prefer "asynchronously"

[Ethan-Arrowood]

lgtm! ty all for quick iteration here.

[nodejs-github-bot]

Landed in 774e564