test_runner: ensure test watcher picks up new test files #54225
Conversation
|
Review requested:
|
Codecov ReportAttention: Patch coverage is
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
|
There was a problem hiding this comment.
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.
|
@cjihrig PTAL |
| * if (this.#mode !== 'filter') { | ||
| * return; | ||
| * } | ||
| */ |
There was a problem hiding this comment.
Which PR introduced this? Can you ping the original author?
There was a problem hiding this comment.
If I'm not mistaken, this change is from @MoLow
There was a problem hiding this comment.
I'm not sure what to make of this change 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
will start CI again as soon as this PR lands (just make sure there are no false positive test results) thanks for your patient 🙏 |
|
I've seen that we now have conflicts, gonna fix them ASAP 🚀 |
There was a problem hiding this comment.
This adds a new option to run(), so it needs to have docs.
f848750 to
17b7ce1
Compare
|
@jakecastelli, I had to rebase again cause of conflicts 🚀 |
@cjihrig, if this PR lands, I'll start working on it 😁 |
|
Landed in dcf50f1 |
|
@pmarchini no need to open an issue. |
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.