FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: split test-runner-run-watch.mjs by joyeecheung · Pull Request #60653 · nodejs/node · GitHub

/ node Public

test: split test-runner-run-watch.mjs - #60653

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
joyeecheung:split-test-runner-run-watch
Nov 11, 2025
ConversationCommits (1)ChecksFiles changed
Merged

test: split test-runner-run-watch.mjs#60653
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
joyeecheung:split-test-runner-run-watch

Conversation

joyeecheung commented Nov 9, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

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

test: split test-runner-run-watch.mjs
27796e6 
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.

nodejs-github-bot commented Nov 9, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 9, 2025
joyeecheung mentioned this pull request Nov 9, 2025
Closed
mcollina approved these changes Nov 9, 2025
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2025

codecov Bot commented Nov 9, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.45%. Comparing base (8e5c80d) to head (27796e6).
⚠️ Report is 33 commits behind head on main.

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

see 62 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.

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2025

nodejs-github-bot commented Nov 9, 2025

Copy link
Copy Markdown
Collaborator

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

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 9, 2025

nodejs-github-bot commented Nov 9, 2025

Copy link
Copy Markdown
Collaborator

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

jakecastelli added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2025
joyeecheung commented Nov 10, 2025
View reviewed changes
Comment thread test/common/watch.js
common.platformTimeout(1000),
);
await ran2.promise;
clearInterval(interval);

joyeecheung Nov 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I copied the original logic in

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

Lines 80 to 83 in 9524908

const interval = setInterval(() => writeFileSync(path, content), common.platformTimeout(1000));
await ran2.promise;
runs.push(currentRun);
clearInterval(interval);
here because I want to keep this PR strictly a split, and not change logic for now, so this is not reusing that 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 )

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 11, 2025
nodejs-github-bot merged commit ea1a240 into nodejs:main Nov 11, 2025
79 of 80 checks passed

nodejs-github-bot commented Nov 11, 2025

Copy link
Copy Markdown
Collaborator

Landed in ea1a240

targos pushed a commit that referenced this pull request Nov 27, 2025
test: split test-runner-run-watch.mjs
313f0dc 
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>
RafaelGSS pushed a commit that referenced this pull request Jan 13, 2026
test: split test-runner-run-watch.mjs
5a976cb 
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>
github-actions Bot mentioned this pull request Jan 16, 2026
Merged
aduh95 pushed a commit that referenced this pull request Jan 19, 2026
test: split test-runner-run-watch.mjs
b09129d 
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>
github-actions Bot mentioned this pull request Feb 3, 2026
Merged
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
atlowChemi atlowChemi approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL