-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test_runner: add cwd option to run #54705
Conversation
Review requested:
|
We need to turn the If we make the Same thing. The test object there will have a reference to the |
@cjihrig thanks for the support, I'm gonna take a look ASAP! |
4fa3784
to
3419393
Compare
src/node_options.cc
Outdated
@@ -671,6 +671,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { | |||
&EnvironmentOptions::test_coverage_lines, | |||
kAllowedInEnvvar); | |||
|
|||
AddOption("--experimental-test-cwd", |
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.
I don't think we should add this. The cwd
option should be supported by the run()
API only IMO.
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.
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
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.
If the user wants to use the CLI, they could do something like cd path/to/tests && node --test
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.
I agree
fc95c78
to
518bb16
Compare
lib/internal/test_runner/runner.js
Outdated
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'); | ||
} |
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.
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.
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.
I removed the validation and added a couple of tests to check that run
handles an incorrect cwd without throwing errors
lib/internal/test_runner/coverage.js
Outdated
@@ -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(); |
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.
We should ensure that options.cwd
always exists and remove the ?? process.cwd()
fallback.
lib/internal/test_runner/harness.js
Outdated
@@ -83,7 +83,7 @@ function createTestTree(rootTestOptions, globalOptions) { | |||
return globalRoot; | |||
} | |||
|
|||
function createProcessEventHandler(eventName, rootTest) { | |||
function createProcessEventHandler(eventName, rootTest, cwd) { |
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.
I would prefer to get the cwd
from the rootTest
here instead of adding another argument.
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.
done
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.
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?
924c375
to
ac74667
Compare
Codecov ReportAttention: Patch coverage is
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
|
const interval = setInterval(() => renameSync(fileToRenamePath, newFileNamePath), common.platformTimeout(1000)); | ||
const interval = setInterval(() => { | ||
renameSync(fileToRenamePath, newFileNamePath); | ||
clearInterval(interval); | ||
}, common.platformTimeout(1000)); |
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.
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
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.
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?
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.
Hey @cjihrig, I agree, I hadn’t thought about replacing it with setTimeout
😬
I’ll do it ASAP !
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.
@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
25deaa7
to
29d93c6
Compare
test/fixtures/test-runner-watch.mjs
Outdated
...(cwd ? { cwd } : {}), | ||
...(isolation ? { isolation } : {}), |
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.
Can this be simplified to:
...(cwd ? { cwd } : {}), | |
...(isolation ? { isolation } : {}), | |
cwd, | |
isolation, |
@@ -29,6 +29,10 @@ import('data:text/javascript,'); | |||
test('test has ran');`, | |||
}; | |||
|
|||
function wait(ms) { |
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.
Please use setTimeout()
from node:timers/promises
(or is this just the changes from your other PR?)
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.
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)
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.
done
lib/internal/test_runner/harness.js
Outdated
@@ -252,7 +252,11 @@ function lazyBootstrapRoot() { | |||
__proto__: null, | |||
entryFile: process.argv?.[1], | |||
}; | |||
const globalOptions = parseCommandLine(); | |||
const globalOptions = { |
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.
I wonder if it would be more efficient to either:
- Add
cwd
to the object returned inparseCommandLine()
. Although then that function is not strictly only working with the command line. - Do
globalOptions.cwd = process.cwd()
here instead of creating an object inparseCommandLine()
, spreading it here, and creating a new object containing all the same values as the original object.
I'm personally leaning toward option 2.
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.
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
29d93c6
to
e832333
Compare
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
Landed in 1c7795e |
The commit lands cleanly on v22.x-staging but test fails:
|
My guess is that the problem is the fixture's |
I'll take a look ASAP |
Hey @cjihrig, I just took a quick look and the fixtures are not using 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 The behaviour I would expect is the one in Note: I just tried to execute The output is different, when test('test one', function() {
record(this.name);
}); (I just tested this in main) |
I believe it is:
|
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? 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 |
For the |
I agree, but I think I might be missing something here. Why is 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 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 |
I believe so. |
AFAICT that PR is already on |
Hey @aduh95, I just tested locally, and I think we're missing this PR: #54881 (landed in commit dbaef33) I suppose it can't be backported, as it's a Note: forgive me if what I've said doesn't make sense; I'm not super confident about backporting and release management 😬 |
PR-URL: nodejs#54705 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#54705 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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)
For this reason, we decided to work on this in a separate PR (#54225 (comment)).