test: split test-runner-run-watch.mjs #60653
Merged
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
This test contains too many independent test cases and as a result, marking it as flaky on all major platforms means actual regressions could be covered up, and it's constantly making the CI orange and requires extra resuming on the flaked platforms which is still not great. Split it into individual files so that the actual flake can be identified out of the monolith.
Collaborator
|
Review requested:
|
nodejs-github-bot
added
needs-ci
atlowChemi
approved these changes
Nov 9, 2025
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60653 +/- ##
==========================================
- Coverage 88.54% 88.45% -0.09%
==========================================
Files 704 703 -1
Lines 208103 208058 -45
Branches 40089 40001 -88
==========================================
- Hits 184256 184031 -225
- Misses 15876 16029 +153
- Partials 7971 7998 +27 🚀 New features to boost your workflow:
|
github-actions
bot
removed
the
request-ci
Collaborator
joyeecheung
added
the
author ready
Collaborator
benjamingr
approved these changes
Nov 10, 2025
benjamingr
approved these changes
Nov 10, 2025
jakecastelli
added
the
commit-queue
joyeecheung
commented
Nov 10, 2025
| common.platformTimeout(1000), | ||
| ); | ||
| await ran2.promise; | ||
| clearInterval(interval); |
Member
Author
There was a problem hiding this comment.
I copied the original logic in
node/test/parallel/test-runner-run-watch.mjs
Lines 80 to 83 in 9524908
performFileOperation() helper; test-run-watch-dependency-repeatedly.mjs flaked in https://ci.nodejs.org/job/node-test-pull-request/70115/ and I think the culprit might be in this logic - the interval fires too fast and before the test finishes, the write fires, then the watched process get killed with SIGTERM to run again. I think the follow up to deflake it might be to just do what's in the branch below and not do the interval.
(I am somewhat puzzled why it's using an interval in the original logic, I think update + timeout like the branch below which was split from the --watch test should suffice; repeated updates could trigger additional events that kill the process being watched unexpectedly @mcollina )
lpinca
approved these changes
Nov 10, 2025
nodejs-github-bot
removed
the
commit-queue
Collaborator
|
Landed in ea1a240 |
targos
pushed a commit
that referenced
this pull request
Nov 27, 2025
This test contains too many independent test cases and as a result, marking it as flaky on all major platforms means actual regressions could be covered up, and it's constantly making the CI orange and requires extra resuming on the flaked platforms which is still not great. Split it into individual files so that the actual flake can be identified out of the monolith. PR-URL: #60653 Refs: #54534 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
This test contains too many independent test cases and as a result, marking it as flaky on all major platforms means actual regressions could be covered up, and it's constantly making the CI orange and requires extra resuming on the flaked platforms which is still not great. Also, it can increase the flakiness when the tests share the same files being watched due to file system events coalescing. Split it into individual files so that the actual flake can be identified out of the monolith and reduce flakiness.
Refs: #54534