test_runner: add cwd option to run by pmarchini · Pull Request #54705 · nodejs/node · GitHub

I'm opening this PR as a draft to have a place where we can discuss this implementation.

Some background context:

During #54225, we discussed the possibility of adding a new option to run.
This new option would be cwd.

This change could impact many other parts of the code, such as:
#54225 (comment)

I just pushed a new commit.

Nice. I realized that this new cwd option may introduce new bugs in at least three places:

With --experimental-test-isolation=none.

With code coverage reporting.

With test locations.

For this reason, we decided to work on this in a separate PR (#54225 (comment)).

Review requested:

@nodejs/test_runner

With --experimental-test-isolation=none.

We need to turn the cwd option into a URL and get the href.

With code coverage reporting.

If we make the cwd a part of the globalOptions object, the coverage code will have access to it.

With test locations.

Same thing. The test object there will have a reference to the cwd.

@cjihrig thanks for the support, I'm gonna take a look ASAP!

I don't think we should add this. The cwd option should be supported by the run() API only IMO.

Okay, makes sense! I was thinking about a way to set the entire test runner's cwd to allow this behavior to be configured via the CLI as well.

Btw, while working on this, I noticed an unexpected behavior related to isolation and watch mode.
I'm going to open a PR to ask for your feedback, @cjihrig

If the user wants to use the CLI, they could do something like cd path/to/tests && node --test

I agree

I would rather not do this. existsSync() specifically is a race condition. Just assume that the cwd is a directory and handle any errors. For example, the child process APIs, which also take a cwd option don't perform this type of validation.

I removed the validation and added a couple of tests to check that run handles an incorrect cwd without throwing errors

We should ensure that options.cwd always exists and remove the ?? process.cwd() fallback.

I would prefer to get the cwd from the rootTest here instead of adding another argument.

done

In the meantime, I've seen that in harness.js we have:

node/lib/internal/test_runner/harness.js

Lines 80 to 90 in c1afd2c

    
             harness.resetCounters(); 
        
             globalRoot = new Test({ 
        
               __proto__: null, 
        
               ...rootTestOptions, 
        
               harness, 
        
               name: '<root>', 
        
             }); 
        
             setupProcessState(globalRoot, globalOptions, harness); 
        
             globalRoot.startTime = hrtime(); 
        
             return globalRoot; 
        
           }

While the method definition is:

node/lib/internal/test_runner/harness.js

Lines 182 to 184 in c1afd2c

    
           function setupProcessState(root, globalOptions) { 
        
             const hook = createHook({

Can I remove harness from the setupProcessState method call, or should I do that in a separate PR?

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.42%. Comparing base (89a2f56) to head (a162f2d).
Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/runner.js	78.57%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54705      +/-   ##
==========================================
+ Coverage   88.39%   88.42%   +0.02%     
==========================================
  Files         652      652              
  Lines      186565   186576      +11     
  Branches    36046    36050       +4     
==========================================
+ Hits       164916   164981      +65     
+ Misses      14908    14866      -42     
+ Partials     6741     6729      -12

Files with missing lines	Coverage Δ
lib/internal/test_runner/coverage.js	64.69% <100.00%> (-0.06%)	⬇️
lib/internal/test_runner/harness.js	92.67% <100.00%> (+0.02%)	⬆️
lib/internal/test_runner/runner.js	89.42% <78.57%> (+2.38%)	⬆️

... and 45 files with indirect coverage changes

Hey @cjihrig, regarding these tests, I was working on a GitHub codespace yesterday, and I noticed that this test was flaky there.
The issue was with the interval: if the system is delayed for any reason beyond the timeout, a second run of "rename" (and similarly for "delete") is triggered, causing an error because the file has already been renamed and can no longer be found under its previous name.

I'll take a look at the other similar tests

That makes sense. Timers in tests should be avoided like the plague if possible.

Do we need to use an interval for operations like that? For example, would it work to use a setTimeout() and then reschedule the callback again once the timer fires?

Hey @cjihrig, I agree, I hadn’t thought about replacing it with setTimeout 😬
I’ll do it ASAP !

@cjihrig, following another fix I was working on for a related flaky test, test/parallel/test-runner-watch-mode-complex.mjs, and following your suggestions, I removed the setIntervals in an attempt to simplify the test's readability while hopefully preventing flakiness

Can this be simplified to:

Suggested change

      
              ...(cwd ? { cwd } : {}),
          
              ...(isolation ? { isolation } : {}),
          
              cwd,
          
              isolation,

Please use setTimeout() from node:timers/promises (or is this just the changes from your other PR?)

Absolutely! I worked on them at the same time.
I need to port what we did on the other test to this one 😁

This change could also reduce the flakiness of this test (at least I hope so)

done

I wonder if it would be more efficient to either:

Add cwd to the object returned in parseCommandLine(). Although then that function is not strictly only working with the command line.
Do globalOptions.cwd = process.cwd() here instead of creating an object in parseCommandLine(), spreading it here, and creating a new object containing all the same values as the original object.

I'm personally leaning toward option 2.

hey @cjihrig, I agree regarding option 2.
IMHO option 1 would make sense if we were adding a new option.

I'm going to take a look ASAP

lgtm

Landed in 1c7795e

The commit lands cleanly on v22.x-staging but test fails:

./node test/parallel/test-runner-no-isolation-different-cwd.mjs
before one: <root>
suite one
before two: <root>
suite two
beforeEach one: suite one - test
beforeEach two: suite one - test
suite one - test
afterEach one: suite one - test
afterEach two: suite one - test
beforeEach one: test one
beforeEach two: test one
test one
afterEach one: test one
afterEach two: test one
before suite two: suite two
beforeEach one: suite two - test
beforeEach two: suite two - test
suite two - test
afterEach one: suite two - test
afterEach two: suite two - test
after one: <root>
after two: <root>
node:internal/modules/run_main:122
    triggerUncaughtException(
    ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

  [
    'before one: <root>',
...
    'afterEach one: suite one - test',
    'afterEach two: suite one - test',
+   'beforeEach one: test one',
+   'beforeEach two: test one',
+   'test one',
+   'afterEach one: test one',
+   'afterEach two: test one',
    'before suite two: suite two',
...
    'after one: <root>',
    'after two: <root>'
  ]
    at file:///Users/mzasso/git/nodejs/v22.x/test/parallel/test-runner-no-isolation-different-cwd.mjs:16:1
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {

My guess is that the problem is the fixture's only test which requires --test-only on v22, but does not on main.

I'll take a look ASAP

Hey @cjihrig, I just took a quick look and the fixtures are not using only.

Given one.test.js:

suite('suite one', function() {
  record(this.name);

  test('suite one - test', { only: true }, function() {
    record(this.name);
  });
});

test('test one', function() {
  record(this.name);
});

and checking the output it seems that the failing output on the v22.x-staging is indeed correct.
Idk why but it seems that in main, atm, given those test files, we're only executing the tests contained inside the suite and not the standalone test ('test one').

The behaviour I would expect is the one in v22.x-staging, do we agree on this?

Note:

I just tried to execute
./node --test --experimental-test-isolation=none test/fixtures/test-runner/no-isolation/one.test.js
and
./node --test --experimental-test-isolation=process test/fixtures/test-runner/no-isolation/one.test.js.

The output is different, when isolation == none we're skipping

test('test one', function() {
  record(this.name);
});

(I just tested this in main)

I just took a quick look and the fixtures are not using only.

I believe it is:

test('suite one - test', { only: true }, function() {

I just took a quick look and the fixtures are not using only.

I believe it is:

test('suite one - test', { only: true }, function() {

Sorry, I completely missed it, even though I had just recopied it 😬

@cjihrig, but still, do you think it makes sense to have two different behaviors based on the isolation?
Honestly, I wasn't expecting this.

Note:

> ./node --test --experimental-test-isolation=none  test/fixtures/test-runner/no-isolation/one.test.js
▶ suite one
  ✔ suite one - test (0.510791ms)
✔ suite one (0.929208ms)
ℹ tests 1
ℹ suites 1
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 14.173333

and

> ./node --test --experimental-test-isolation=process  test/fixtures/test-runner/no-isolation/one.test.js
▶ suite one
  ✔ suite one - test (0.441834ms)
  ℹ 'only' and 'runOnly' require the --test-only command-line option.
✔ suite one (0.865417ms)
✔ test one (0.209416ms)
ℹ tests 2
ℹ suites 1
ℹ pass 2
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 94.562917

do you think it makes sense to have two different behaviors based on the isolation?

For the only flag, I do. When running multiple test processes in parallel, some things are inherently different. For example, look at mocha, which calls out "some important implications of the behavior" including not supporting only at all.

do you think it makes sense to have two different behaviors based on the isolation?

For the only flag, I do. When running multiple test processes in parallel, some things are inherently different. For example, look at mocha, which calls out "some important implications of the behavior" including not supporting only at all.

I agree, but I think I might be missing something here.

Why is test-only the default when isolation == none? As a user, I would expect to have to set --test-only even when isolation == none (or perhaps the reverse should be true).

What I mean is, I would expect the same default behavior regardless of the isolation mode.

This is an honest question. Considering that 22.x is still following this logic, I suppose it's something that was considered and decided recently. I'm not strongly opinionated on this, just curious.

EDIT: As always, thanks for your time and support! 🚀

The idea is that no one ever wants to type --test-only because only tests can be automatically detected and handled appropriately. only is uniquely annoying because the test runner has to discover all of the tests that it will run, determine if any only tests were found, and if so, then apply filtering.

When using process isolation the annoyance factor is 100x worse because that logic needs to be coordinated across multiple child processes. It is technically possible to support, but the complexity and performance overhead are not worth it IMO. Prior to isolation=none existing, process isolation was the only mode of execution, so we needed a way to handle only tests. That's how --test-only came to be (the idea was copied from node-tap).

@cjihrig, thank you for the explanation, it makes perfect sense 😁
I suppose this problem will be fixed as soon as #54832 is backported to 22.x, right?

I believe so.

I suppose this problem will be fixed as soon as #54832 is backported to 22.x, right?

AFAICT that PR is already on v22.x, but trying to cherry-pick the commit from this PR still have a failing test.

Hey @aduh95, I just tested locally, and I think we're missing this PR: #54881 (landed in commit dbaef33)
I tried cherry-picking that specific commit into 22.x-staging, and it fixes this test.

I suppose it can't be backported, as it's a semver-major.

Note: forgive me if what I've said doesn't make sense; I'm not super confident about backporting and release management 😬

Not having this on v22.x creates more and more divergence between 23.x and 22.x, causing more and more backports to fail. It would be very helpful to work on a backport before the tech debt has grown out of proportions. IIUC it would probably require to find a way to backport #54881 in a non-semver-major way

@aduh95 maybe #54881 should be treated as a bug fix? I don't think it would realistically break anyone and it would unblock backporting all of these other changes.

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 Sep 1, 2024

avivkeller reviewed Sep 1, 2024

View reviewed changes

Comment thread doc/api/test.md Outdated

Comment thread doc/api/test.md Outdated

pmarchini force-pushed the feat/cwd-option-run branch from 4fa3784 to 3419393 Compare September 11, 2024 09:35

avivkeller reviewed Sep 11, 2024

View reviewed changes

Comment thread doc/api/cli.md Outdated

Comment thread src/node_options.cc Outdated

Comment thread doc/api/cli.md Outdated

Comment thread lib/internal/test_runner/utils.js Outdated

Comment thread src/node_options.h Outdated

Comment thread test/parallel/test-runner-run.mjs Outdated

cjihrig reviewed Sep 11, 2024

View reviewed changes

pmarchini force-pushed the feat/cwd-option-run branch from fc95c78 to 518bb16 Compare September 11, 2024 14:48

avivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 11, 2024

avivkeller reviewed Sep 12, 2024

View reviewed changes

Comment thread lib/internal/test_runner/coverage.js Outdated

cjihrig reviewed Sep 12, 2024

View reviewed changes

Comment thread lib/internal/test_runner/harness.js Outdated

cjihrig reviewed Sep 12, 2024

View reviewed changes

Comment thread src/node_options.cc Outdated

cjihrig reviewed Sep 12, 2024

View reviewed changes

Comment thread test/parallel/test-runner-no-isolation-different-cwd.mjs Outdated

pmarchini force-pushed the feat/cwd-option-run branch 2 times, most recently from 924c375 to ac74667 Compare September 16, 2024 10:06

pmarchini marked this pull request as ready for review September 16, 2024 13:21

pmarchini commented Sep 18, 2024

View reviewed changes

pmarchini mentioned this pull request Sep 20, 2024

test_runner: do not read from process.argv and process.cwd() in run() #53867

Open

pmarchini force-pushed the feat/cwd-option-run branch 2 times, most recently from 25deaa7 to 29d93c6 Compare September 20, 2024 19:18

cjihrig reviewed Sep 21, 2024

View reviewed changes

pmarchini force-pushed the feat/cwd-option-run branch from 29d93c6 to e832333 Compare September 22, 2024 19:29

mcollina approved these changes Sep 23, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024

cjihrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Oct 3, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 3, 2024

nodejs-github-bot merged commit 1c7795e into nodejs:main Oct 3, 2024

targos added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Oct 5, 2024

aduh95 mentioned this pull request Oct 14, 2024

2024-10-16, Version 23.0.0 (Current) #55338

Merged

This was referenced Jan 31, 2025

test_runner: pass options directly to TestCoverage #55578

Merged

test_runner: exclude test files from coverage by default #56060

Merged

test_runner,cli: mark test isolation as stable #56298

Merged

Chrismart04 mentioned this pull request Jun 28, 2026

test_runner: stop reading process state in run() #64186

Open

4 tasks

	harness.resetCounters();
	globalRoot = new Test({
	__proto__: null,
	...rootTestOptions,
	harness,
	name: '<root>',
	});
	setupProcessState(globalRoot, globalOptions, harness);
	globalRoot.startTime = hrtime();
	return globalRoot;
	}


	function setupProcessState(root, globalOptions) {
	const hook = createHook({

test_runner: add cwd option to run by pmarchini · Pull Request #54705 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: add cwd option to run - #54705

test_runner: add cwd option to run#54705nodejs-github-bot merged 20 commits intonodejs:mainnodejs/node:mainfrom pmarchini:feat/cwd-option-runpmarchini/node:feat/cwd-option-runCopy head branch name to clipboard

Conversation

pmarchini commented Sep 1, 2024

Uh oh!

nodejs-github-bot commented Sep 1, 2024

Uh oh!

Uh oh!

Uh oh!

cjihrig commented Sep 3, 2024

Uh oh!

pmarchini commented Sep 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 3, 2024

Uh oh!

targos commented Oct 5, 2024

Uh oh!

cjihrig commented Oct 5, 2024

Uh oh!

pmarchini commented Oct 5, 2024

test_runner: add cwd option to run#54705
nodejs-github-bot merged 20 commits into
nodejs:mainfrom
pmarchini:feat/cwd-option-run

codecov Bot commented Sep 16, 2024 •
edited
Loading

pmarchini commented Oct 8, 2024 •
edited
Loading

pmarchini commented Oct 8, 2024 •
edited
Loading