GeezerVison

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!

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.

@@            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     

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.