watch: fix watch args not being properly filtered

[dario-piotrowicz]

Currently the logic for filtering out watch related flags before passing them to the watch target script is flawed, as it includes any flag (starting with -) that follows a flag starting with --watch (code), this allows some watch flags to make it pass the filtering resulting in the generation of multiple watch processes which is both wasteful and can cause duplicate terminal logs (alongside other issues if the target script is side-effectful).

For example if I run

node --watch --watch index.js

the current implementation will spawn a process equivalent to:

node --watch index.js

which will then finally spawn the correct:

node index.js

Here I'm addressing the incorrect filtering so that all the watch flags are correctly filtered out (making the extra process spawning disappear)

---

Fixes #57124

[dario-piotrowicz]

Note: maybe node should error if multiple --watch flags are provided?

I did not go with such a solution because that would be a breaking change, just ignoring extra --watch flags is a non breaking change and it solves the issue at hand

If someone believes that multiple --watch flags should cause an error I would propose to first land this change right now as a patch then as a followup I can also add the stricter validation which will be included as a breaking change in the next major 🙂

[dario-piotrowicz]

And/or maybe, the erroring when multiple --watch flags are provided is part of the bigger issues of node being quite permissive with its cli flags: #57864 and could be addressed as part of that 🤔

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.27%. Comparing base (25842c5) to head (685fb31).
Report is 162 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57936      +/-   ##
==========================================
- Coverage   92.26%   90.27%   -1.99%     
==========================================
  Files         325      630     +305     
  Lines      126673   186112   +59439     
  Branches    20783    36470   +15687     
==========================================
+ Hits       116869   168017   +51148     
- Misses       9576    10976    +1400     
- Partials      228     7119    +6891     

see 412 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 4acb854

[joyeecheung]

I am not sure if it's this PR, but test-watch-mode has been extremely flaky in both Jenkins and GitHub actions recently e.g. https://ci.nodejs.org/job/node-test-commit/79645/ https://ci.nodejs.org/job/node-test-commit/79644/ and many others. Please refrain from appending new test cases in existing test files as https://github.com/nodejs/node/blob/main/doc/contributing/writing-tests.md suggests - especially this file because it's already being ignored by Jenkins so you are not going to get any proper coverage by appending test cases here, it's only going to make the CI more orange than before, and when it increases flakiness you won't be able to notice.

[dario-piotrowicz]

I see, thank you very much and sorry for the trouble, I will keep this in mind and create tests in new files whenever possible 🙇

[joyeecheung]

Actually, this PR landed despite failing many platforms in the test file it appends to: https://ci.nodejs.org/job/node-test-pull-request/66587/

[joyeecheung]

Sending a revert since it should not have landed #58190

[dario-piotrowicz]

Actually, this PR landed despite failing many platforms in the test file it appends to: https://ci.nodejs.org/job/node-test-pull-request/66587/

Note: I've addressed those issues in #58183, I wonder if there were actually other issues as well 🤔