watch: fix watch args not being properly filtered (second attempt) #58279
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
currently when --watch is used, the argv arguments that the target script receives are filtered so that they don't include watch related arguments, however the current filtering logic is incorrect and it causes some watch values to incorrectly pass the filtering, the changes here address such issue
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58279 +/- ##
==========================================
- Coverage 90.19% 90.18% -0.02%
==========================================
Files 631 631
Lines 186670 186670
Branches 36670 36664 -6
==========================================
- Hits 168374 168344 -30
- Misses 11112 11122 +10
- Partials 7184 7204 +20 🚀 New features to boost your workflow:
|
benjamingr
approved these changes
May 11, 2025
dario-piotrowicz
added
the
request-ci
github-actions
bot
removed
the
request-ci
jasnell
approved these changes
May 12, 2025
dario-piotrowicz
added
author ready
nodejs-github-bot
removed
the
commit-queue
|
Landed in ae8a6de |
targos
pushed a commit
that referenced
this pull request
May 16, 2025
currently when --watch is used, the argv arguments that the target script receives are filtered so that they don't include watch related arguments, however the current filtering logic is incorrect and it causes some watch values to incorrectly pass the filtering, the changes here address such issue PR-URL: #58279 Fixes: #57124 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95
pushed a commit
that referenced
this pull request
Jun 10, 2025
currently when --watch is used, the argv arguments that the target script receives are filtered so that they don't include watch related arguments, however the current filtering logic is incorrect and it causes some watch values to incorrectly pass the filtering, the changes here address such issue PR-URL: #58279 Fixes: #57124 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm re-introducing this change that previously landed in #57936 but got reverted in #58190.
I believe that one of the main reasons for reverting the PR was the fact that when it landed it didn't include the changes that I however later introduced in #58183 (that also got reverted as part of #58190) (making me wonder if the revert was actually completely necessary after all 🤔).
Another reason for the revert was that I was adding the tests to the
test/sequential/test-watch-mode.mjsfile (which is marked as flaky) instead of creating a new test file.I am not sure if there were other issues associated to the original PR, however I am opening this PR that reintroduces the changes and also addresses the two above issues (if there were other issues I'd imagine we'll see them when running ci against this PR).
Fixes: #57124