nodejs
test_runner: add cwd option to run
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: https://github.com/nodejs/node/pull/54225#issuecomment-2308943539
I just pushed a new commit.
Nice. I realized that this new
cwdoption may introduce new bugs in at least three places:
For this reason, we decided to work on this in a separate PR (https://github.com/nodejs/node/pull/54225#issuecomment-2308944216).
We need to turn the cwd option into a URL and get the href.
If we make the cwd a part of the globalOptions object, the coverage code will have access to it.
Same thing. The test object there will have a reference to the cwd.
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 :warning: |
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%) |
:arrow_down: |
| lib/internal/test_runner/harness.js | 92.67% <100.00%> (+0.02%) |
:arrow_up: |
| lib/internal/test_runner/runner.js | 89.42% <78.57%> (+2.38%) |
:arrow_up: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
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!
@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
@cjihrig, I'm still looking into this. I've been able to replicate it 3 times in 12 hours, after thousands of retries
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?
WDYT?
I don't have a great answer. This is why I try to avoid working on watch mode.
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.
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
onlyflag, 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 supportingonlyat 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 https://github.com/nodejs/node/pull/54832 is backported to 22.x, right?
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: https://github.com/nodejs/node/pull/54881 (landed in commit https://github.com/nodejs/node/commit/dbaef339aad329aaf7e1b23072da7462ed9543e1)
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 😬