test_runner: ensure test watcher picks up new test files

[pmarchini]

trafficstars

This is the initial implementation addressing issue #53077.

I've noticed the issue has been inactive for a while, so I attempted to resolve it by following the provided suggestions.

While this solution seems to work, I'm not entirely satisfied with the implementation.

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/test_runner

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 72.41379% with 8 lines in your changes missing coverage. Please review.

Project coverage is 87.31%. Comparing base (05bd3cf) to head (67f8998). Report is 321 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/runner.js	71.42%	8 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54225      +/-   ##
==========================================
- Coverage   87.33%   87.31%   -0.02%     
==========================================
  Files         649      649              
  Lines      182622   182638      +16     
  Branches    35037    35040       +3     
==========================================
- Hits       159494   159479      -15     
- Misses      16393    16418      +25     
- Partials     6735     6741       +6     

Files with missing lines	Coverage Δ
lib/internal/watch_mode/files_watcher.js	75.74% <100.00%> (+1.23%)	:arrow_up:
lib/internal/test_runner/runner.js	87.76% <71.42%> (-0.66%)	:arrow_down:

... and 24 files with indirect coverage changes

[mcollina]

@cjihrig PTAL

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[jakecastelli]

will start CI again as soon as this PR lands (just make sure there are no false positive test results) thanks for your patient 🙏

[pmarchini]

I've seen that we now have conflicts, gonna fix them ASAP 🚀

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[cjihrig]

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.

[pmarchini]

We have also landed support for running processes in the current process, so we would need to make sure that case works as well.

@cjihrig about this: should we prevent the user from setting both isolation: none and cwd, and handle this in another PR?

I'm implementing a test starting from test-runner-no-isolation.mjs but, at least in my local env, if I try, e.g., to set an hardcode wrong value here https://github.com/nodejs/node/blob/43f699d4d2799cfc17cbcad5770e1889075d5dbe/lib/internal/test_runner/runner.js#L683 then the test doesn't fail.

In order to make it fail I had to change the test as follow:

....
let errors = 0;
stream.on('test:fail', () => {
  errors++;
});
stream.on('test:pass', mustCall(5));
// eslint-disable-next-line no-unused-vars
for await (const _ of stream);
deepStrictEqual(errors, 0);
allowGlobals(globalThis.GLOBAL_ORDER);
deepStrictEqual(globalThis.GLOBAL_ORDER, [
  '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',
.......

[cjihrig]

should we prevent the user from setting both isolation: none and cwd, and handle this in another PR?

I don't think so. I think we should introduce the cwd in one PR separate from these changes to the file watching logic.

[pmarchini]

@cjihrig, ok, I'm going to remove the new option solving only the initial issue.

again: thanks 🚀

[jakecastelli]

Would you mind rebasing 😄 so I can kick start the CI again 👍

[pmarchini]

@jakecastelli, rebased 🚀

[nodejs-github-bot]

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

[pmarchini]

@jakecastelli, I had to rebase again cause of conflicts 🚀

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[pmarchini]

Also, I hope you're still planning to add the cwd option 😄

@cjihrig, if this PR lands, I'll start working on it 😁 Should I open an issue before beginning?

[nodejs-github-bot]

Landed in dcf50f15bc7e49637b79876440b6fcdf827aebcc

[cjihrig]

@pmarchini no need to open an issue.