test_runner: ensure test watcher picks up new test files by pmarchini · Pull Request #54225 · nodejs/node

pmarchini · 2024-08-06T14:55:42Z

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 · 2024-08-06T14:55:49Z

Review requested:

@nodejs/test_runner

codecov · 2024-08-06T16:45:09Z

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 ⚠️

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%)`	⬆️
lib/internal/test_runner/runner.js	`87.76% <71.42%> (-0.66%)`	⬇️

... and 24 files with indirect coverage changes

mcollina

Thanks for contributing! I've left a few notes and questions.

You'll also need to add a test to https://github.com/nodejs/node/blob/main/test/parallel/test-runner-run-watch.mjs.

lib/internal/test_runner/runner.js

test/parallel/test-runner-run.mjs

lib/internal/test_runner/runner.js

mcollina · 2024-08-09T17:28:59Z

@cjihrig PTAL

mcollina

Good work!

mcollina · 2024-08-12T06:31:22Z

lib/internal/watch_mode/files_watcher.js

+    * if (this.#mode !== 'filter') {
+    *   return;
+    * }
+    */


Which PR introduced this? Can you ping the original author?

If I'm not mistaken, this change is from @MoLow

I'm not sure what to make of this change 😄

I think it would be great to know if that logic was there for a specific reason. Removing it didn't disrupt any tests.

I added the comment just to ensure that the change didn’t go unnoticed.

Il need to get back from vacation to be in front of a computer to be sure of my answer, but using a single watcher (i.e all mode is probably more efficient and peformant) so creating additional watchers when they are not needed just adds more overhead that is unneeded

I removed the comment in the last commit @cjihrig.
I think we should wait for @MoLow to be back 🚀

I agree with this.

lib/internal/test_runner/runner.js

nodejs-github-bot · 2024-08-12T06:33:28Z

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

mcollina

lgtm

nodejs-github-bot · 2024-08-12T10:41:45Z

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

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

cjihrig

LGTM. It would be good to get @MoLow's review on this though.

lib/internal/test_runner/runner.js

jakecastelli · 2024-08-14T13:08:27Z

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 · 2024-08-14T16:23:39Z

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

cjihrig

This adds a new option to run(), so it needs to have docs.

lib/internal/test_runner/runner.js

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

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

pmarchini · 2024-08-26T13:56:54Z

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

nodejs-github-bot · 2024-08-27T00:30:04Z

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

nodejs-github-bot · 2024-08-27T09:59:24Z

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

jakecastelli

Great work

pmarchini · 2024-08-29T08:18:00Z

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?

mcollina

lgtm

nodejs-github-bot · 2024-08-29T08:55:35Z

Landed in dcf50f1

cjihrig · 2024-08-29T12:53:33Z

@pmarchini no need to open an issue.

PR-URL: #54225 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>

PR-URL: nodejs#54225 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>

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 Aug 6, 2024

pmarchini force-pushed the issue/53077 branch from ac52a1d to 5efd995 Compare August 6, 2024 15:09

pmarchini force-pushed the issue/53077 branch from 4168d71 to b23a410 Compare August 7, 2024 21:21

mcollina requested changes Aug 8, 2024

View reviewed changes

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

test/parallel/test-runner-run.mjs Outdated Show resolved Hide resolved

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

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

pmarchini force-pushed the issue/53077 branch from f582175 to 3ccbbb4 Compare August 9, 2024 17:32

mcollina reviewed Aug 12, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2024

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

mcollina requested review from MoLow, cjihrig and jakecastelli August 12, 2024 10:34

mcollina approved these changes Aug 12, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2024

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

jakecastelli reviewed Aug 12, 2024

View reviewed changes

test/parallel/test-runner-run-watch.mjs Outdated Show resolved Hide resolved

cjihrig approved these changes Aug 12, 2024

View reviewed changes

jakecastelli reviewed Aug 12, 2024

View reviewed changes

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

cjihrig requested changes Aug 14, 2024

View reviewed changes

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

pmarchini force-pushed the issue/53077 branch 2 times, most recently from f848750 to 17b7ce1 Compare August 16, 2024 05:12

pmarchini requested a review from cjihrig August 16, 2024 06:04

jakecastelli reviewed Aug 16, 2024

View reviewed changes

test/parallel/test-runner-watch-mode.mjs Show resolved Hide resolved

test/parallel/test-runner-run-watch.mjs Show resolved Hide resolved

pmarchini added 6 commits August 26, 2024 15:24

test_runner: fix paths resolution

387e1b0

test_runner: fix doc

8c38fd2

test: remove reporter specific assertions

4061176

test_runner: spawn test process with correct cwd

32150f6

test: remove unnecessary whitespace

4310da8

test_runner: remove cwd option

67f8998

pmarchini force-pushed the issue/53077 branch from 5681e4c to 67f8998 Compare August 26, 2024 13:25

jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2024

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

jakecastelli approved these changes Aug 27, 2024

View reviewed changes

cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Aug 27, 2024

mcollina approved these changes Aug 29, 2024

View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2024

nodejs-github-bot merged commit dcf50f1 into nodejs:main Aug 29, 2024

jakecastelli mentioned this pull request Aug 29, 2024

watch with test not picking up new test files #53077

Closed

RafaelGSS mentioned this pull request Aug 30, 2024

v22.8.0 proposal #54560

Merged

pmarchini mentioned this pull request Sep 1, 2024

test_runner: add cwd option to run #54705

Merged

targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024

Uh oh!

Conversation

pmarchini commented Aug 6, 2024

Uh oh!

nodejs-github-bot commented Aug 6, 2024

Uh oh!

codecov bot commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina commented Aug 9, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

cjihrig Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

MoLow Aug 20, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 12, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 12, 2024

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakecastelli commented Aug 14, 2024

Uh oh!

pmarchini commented Aug 14, 2024

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmarchini commented Aug 26, 2024

Uh oh!

nodejs-github-bot commented Aug 27, 2024

Uh oh!

nodejs-github-bot commented Aug 27, 2024 • edited by cjihrig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakecastelli left a comment

Choose a reason for hiding this comment

Uh oh!

pmarchini commented Aug 29, 2024

Uh oh!

mcollina left a comment

codecov bot commented Aug 6, 2024 •

edited

Loading

pmarchini Aug 21, 2024 •

edited

Loading

nodejs-github-bot commented Aug 27, 2024 •

edited by cjihrig

Loading