test_runner: add cwd option to run #54705

pmarchini · 2024-09-01T19:23:51Z

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)).

nodejs-github-bot · 2024-09-01T19:23:56Z

Review requested:

@nodejs/test_runner

doc/api/test.md

cjihrig · 2024-09-03T18:50:41Z

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.

pmarchini · 2024-09-03T19:03:04Z

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

doc/api/cli.md

src/node_options.cc

doc/api/cli.md

lib/internal/test_runner/utils.js

src/node_options.h

test/parallel/test-runner-run.mjs

cjihrig · 2024-09-11T13:59:52Z

src/node_options.cc

@@ -671,6 +671,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
            &EnvironmentOptions::test_coverage_lines,
            kAllowedInEnvvar);

+  AddOption("--experimental-test-cwd", 


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

lib/internal/test_runner/coverage.js

cjihrig · 2024-09-12T14:07:39Z

lib/internal/test_runner/runner.js

+  if (existsSync(cwd) === false) {
+    throw new ERR_INVALID_ARG_VALUE('options.cwd', cwd, 'expects an existing directory');
+  } else if (!lstatSync(cwd).isDirectory()) {
+    throw new ERR_INVALID_ARG_VALUE('options.cwd', cwd, 'expects a directory, a file was provided');
+  }


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

cjihrig · 2024-09-12T19:55:56Z

lib/internal/test_runner/coverage.js

@@ -477,7 +477,7 @@ function sortCoverageFiles(a, b) {

 function setupCoverage(options) {
  let originalCoverageDirectory = process.env.NODE_V8_COVERAGE;
-  const cwd = process.cwd();
+  const cwd = options.cwd ?? process.cwd();


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

cjihrig · 2024-09-12T19:56:43Z

lib/internal/test_runner/harness.js

@@ -83,7 +83,7 @@ function createTestTree(rootTestOptions, globalOptions) {
  return globalRoot;
 }

-function createProcessEventHandler(eventName, rootTest) {
+function createProcessEventHandler(eventName, rootTest, cwd) {


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

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?

lib/internal/test_runner/harness.js

src/node_options.cc

test/parallel/test-runner-no-isolation-different-cwd.mjs

codecov · 2024-09-16T14:51:12Z

Codecov Report

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

Project coverage is 88.41%. Comparing base (89a2f56) to head (e0b7a48).
Report is 5 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.41%   +0.02%     
==========================================
  Files         652      652              
  Lines      186565   186571       +6     
  Branches    36046    36047       +1     
==========================================
+ Hits       164916   164963      +47     
+ Misses      14908    14880      -28     
+ Partials     6741     6728      -13

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 32 files with indirect coverage changes

pmarchini · 2024-09-18T08:06:37Z

test/parallel/test-runner-watch-mode.mjs

-    const interval = setInterval(() => renameSync(fileToRenamePath, newFileNamePath), common.platformTimeout(1000));
+    const interval = setInterval(() => {
+      renameSync(fileToRenamePath, newFileNamePath);
+      clearInterval(interval);
+    }, common.platformTimeout(1000));


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

cjihrig · 2024-09-21T20:49:49Z

test/fixtures/test-runner-watch.mjs

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


Can this be simplified to:

Suggested change

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

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

cwd,

isolation,

cjihrig · 2024-09-21T20:51:35Z

test/parallel/test-runner-watch-mode.mjs

@@ -29,6 +29,10 @@ import('data:text/javascript,');
 test('test has ran');`,
 };

+function wait(ms) {


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)

cjihrig · 2024-09-21T21:01:25Z

lib/internal/test_runner/harness.js

@@ -252,7 +252,11 @@ function lazyBootstrapRoot() {
      __proto__: null,
      entryFile: process.argv?.[1],
    };
-    const globalOptions = parseCommandLine();
+    const globalOptions = {


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

mcollina

lgtm

nodejs-github-bot · 2024-09-24T13:56:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62731/

cjihrig · 2024-09-24T17:48:21Z

It looks like there may be relevant failures in the latest CI run.

pmarchini · 2024-09-24T17:58:51Z

It looks like there may be relevant failures in the latest CI run.

Hey @cjihrig, yes I've seen, I'm preparing my Windows build!

pmarchini · 2024-09-25T08:07:39Z

@cjihrig after some attempts I was able to reproduce the same errors on my Windows 10 machine.
I'll take a look ASAP, it's definitely flaky

pmarchini · 2024-09-25T22:03:38Z

@cjihrig, I'm still looking into this. I've been able to replicate it 3 times in 12 hours, after thousands of retries

pmarchini · 2024-09-26T07:59:50Z

Hey @cjihrig, I have just pushed a debug-only commit, I will revert it later.

In my local Windows environment, something very strange is happening:

The error, like the one we see in the pipeline, occurs only when running the test on a machine that has just been restarted from sleep.
It makes me think there might be some correlation with my very slow disk and the recovery from sleep mode.

~~What surprises me is that, looking at the stack trace, it seems that the origin of the error is the same abort triggered in the unit test after the test:watch:drained~~ (EDIT: this is expected).

While I keep testing, I will need another pipeline run.
(I don't like trying to debug using the CI, but I'm not finding another way)

WDYT?

cjihrig · 2024-09-28T15:44:39Z

WDYT?

I don't have a great answer. This is why I try to avoid working on watch mode.

Co-authored-by: Aviv Keller <telavivkeller@gmail.com>

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

RedYetiDev reviewed Sep 1, 2024

View reviewed changes

doc/api/test.md Outdated Show resolved Hide resolved

doc/api/test.md Outdated Show resolved Hide resolved

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

RedYetiDev reviewed Sep 11, 2024

View reviewed changes

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

RedYetiDev 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

RedYetiDev reviewed Sep 12, 2024

View reviewed changes

lib/internal/test_runner/coverage.js Outdated Show resolved Hide resolved

cjihrig reviewed Sep 12, 2024

View reviewed changes

lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved

cjihrig reviewed Sep 12, 2024

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

cjihrig reviewed Sep 12, 2024

View reviewed changes

test/parallel/test-runner-no-isolation-different-cwd.mjs Outdated Show resolved Hide resolved

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 the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

pmarchini force-pushed the feat/cwd-option-run branch from 558b2c7 to 0d90e7e Compare October 1, 2024 16:04

pmarchini and others added 18 commits October 1, 2024 18:04

test_runner: add cwd option to run

91858b0

test_runner: add --experimental-test-cwd option to test runner

85e7dbb

Update doc/api/cli.md

e35b227

Co-authored-by: Aviv Keller <telavivkeller@gmail.com>

test_runner: refactor cwd handling in test runner

7d34799

test: add isolation test cases to test runner run watch

18138e9

test: add test cases

8fac041

test_runner: add validation for run cwd option

0e9d10d

test_runner: remove cwd path validation

e6e8329

test: update test case

69acadf

test_runner: revert new line

b600dda

test_runner: remove cwd fallback

9120999

test_runner: remove additional param

37014cc

test_runner: fix unix failures

565a938

test: lint

9aea826

test: remove setInterval

c9288af

test: apply suggestions

71ea33d

test_runner: avoid spread operation

e995bec

test: refactor watch cwd tests

c6d0cec

pmarchini force-pushed the feat/cwd-option-run branch from 0d90e7e to c6d0cec Compare October 1, 2024 19:02

test: divide test cases

e0b7a48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: add cwd option to run #54705

test_runner: add cwd option to run #54705

pmarchini commented Sep 1, 2024

nodejs-github-bot commented Sep 1, 2024

cjihrig commented Sep 3, 2024

pmarchini commented Sep 3, 2024

cjihrig Sep 11, 2024

pmarchini Sep 11, 2024

RedYetiDev Sep 11, 2024

pmarchini Sep 11, 2024

cjihrig Sep 12, 2024

pmarchini Sep 12, 2024

cjihrig Sep 12, 2024

cjihrig Sep 12, 2024

pmarchini Sep 16, 2024

pmarchini Sep 16, 2024

codecov bot commented Sep 16, 2024 •

edited

Loading

pmarchini Sep 18, 2024

cjihrig Sep 18, 2024

pmarchini Sep 20, 2024

pmarchini Sep 20, 2024

cjihrig Sep 21, 2024

cjihrig Sep 21, 2024

pmarchini Sep 22, 2024

pmarchini Sep 22, 2024

cjihrig Sep 21, 2024

pmarchini Sep 22, 2024

mcollina left a comment

nodejs-github-bot commented Sep 24, 2024

cjihrig commented Sep 24, 2024

pmarchini commented Sep 24, 2024

pmarchini commented Sep 25, 2024

pmarchini commented Sep 25, 2024

pmarchini commented Sep 26, 2024 •

edited

Loading

cjihrig commented Sep 28, 2024

	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({

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

test_runner: add cwd option to run #54705

Are you sure you want to change the base?

test_runner: add cwd option to run #54705

Conversation

pmarchini commented Sep 1, 2024

nodejs-github-bot commented Sep 1, 2024

cjihrig commented Sep 3, 2024

pmarchini commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 24, 2024

cjihrig commented Sep 24, 2024

pmarchini commented Sep 24, 2024

pmarchini commented Sep 25, 2024

pmarchini commented Sep 25, 2024

pmarchini commented Sep 26, 2024 • edited Loading

cjihrig commented Sep 28, 2024

codecov bot commented Sep 16, 2024 •

edited

Loading

pmarchini commented Sep 26, 2024 •

edited

Loading