test: add runner watch mode isolation tests by pmarchini · Pull Request #54888 · nodejs/node · GitHub

This PR extends the existing tests for the Node.js test runner's --watch mode by adding support for the --experimental-test-isolation option.

@cjihrig I was unable to find a test that covers this combination.
I also noticed that the should run new tests when a new file is created in the watched directory test behaves differently depending on the isolation modes.
What do you think?

Review requested:

@nodejs/test_runner

What do you think?

Without having investigated the discrepancy, I'd say they should behave the same.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.06%. Comparing base (6db320a) to head (0eab852).
Report is 526 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54888      +/-   ##
==========================================
- Coverage   88.07%   88.06%   -0.01%     
==========================================
  Files         651      651              
  Lines      183538   183538              
  Branches    35861    35864       +3     
==========================================
- Hits       161652   161635      -17     
+ Misses      15145    15140       -5     
- Partials     6741     6763      +22

see 34 files with indirect coverage changes

Mostly LGTM. Two small comments. Have you been able to identify why the one test fails without isolation?

We already skip the entire file at the very top when running on AIX.

done

Instead of an if statement, we could make it a TODO or SKIP test. TODO will run but a failure will not fail the test suite. SKIP won't run at all.

I left the comment because I was planning to fix the issue in this PR, but I haven't had the time in the last two days. I'm going to address your comments, and then I'll investigate the issue ASAP.
(Thanks, as always, for the feedback 😁)

updated! 🚀

Mostly LGTM. Two small comments. Have you been able to identify why the one test fails without isolation?

Hey @cjihrig, I just had the chance to take a look, and every time that a file changes (rename, delete, update), if we are in isolation none, then restartTestFile restarts all the tests without filtering based on the "added, updated, removed" logic.

I'll try to address this issue in a separate PR if you agree.

That makes sense. Thanks for digging in. I'd say if you want to try working on it and think you can improve the experience then go for it! If not, it's still good to have these tests 😄

That makes sense. Thanks for digging in. I'd say if you want to try working on it and think you can improve the experience then go for it! If not, it's still good to have these tests 😄

I'll definitely give it a try 😁

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

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

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

lgtm

Commit Queue failed

- Loading data for nodejs/node/pull/54888 ✔ Done loading data for nodejs/node/pull/54888 ----------------------------------- PR info ------------------------------------ Title test: add runner watch mode isolation tests (#54888) Author Pietro Marchini <pietro.marchini94@gmail.com> (@pmarchini) Branch pmarchini:test/test-runner-watch-mode-isolation -> nodejs:main Labels test, author ready Commits 3 - test: add runner watch mode isolation tests - test: remove unused skip - test: add todo test Committers 1 - Pietro Marchini <pietro.marchini94@gmail.com> PR-URL: https://github.com/nodejs/node/pull/54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 11 Sep 2024 14:40:20 GMT ✔ Approvals: 2 ✔ - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/54888#pullrequestreview-2308136143 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54888#pullrequestreview-2318229689 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-09-17T00:43:41Z: https://ci.nodejs.org/job/node-test-pull-request/62489/ - Querying data for job/node-test-pull-request/62489/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD 99433a2d7a..f79fd03f41 main -> origin/main ✔ origin/main is now up-to-date main is out of sync with origin/main. Mismatched commits: - e84fdebd7b test_runner: add support for coverage via run() - f79fd03f41 test_runner: add support for coverage via run() -------------------------------------------------------------------------------- HEAD is now at f79fd03f41 test_runner: add support for coverage via run() ✔ Reset to origin/main - Downloading patch for 54888 From https://github.com/nodejs/node * branch refs/pull/54888/merge -> FETCH_HEAD ✔ Fetched commits as f79fd03f4161..0eab8529cfdc -------------------------------------------------------------------------------- [main 2096064b13] test: add runner watch mode isolation tests Author: Pietro Marchini <pietro.marchini94@gmail.com> Date: Sun Sep 15 10:46:24 2024 +0200 1 file changed, 27 insertions(+), 18 deletions(-) [main 69a35d3c4d] test: remove unused skip Author: Pietro Marchini <pietro.marchini94@gmail.com> Date: Sun Sep 15 10:46:36 2024 +0200 1 file changed, 3 insertions(+), 3 deletions(-) [main 82759fbd55] test: add todo test Author: Pietro Marchini <pietro.marchini94@gmail.com> Date: Sun Sep 15 10:46:36 2024 +0200 1 file changed, 7 insertions(+), 6 deletions(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: add runner watch mode isolation tests

PR-URL: #54888
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD ea69604a6f] test: add runner watch mode isolation tests
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Sun Sep 15 10:46:24 2024 +0200
1 file changed, 27 insertions(+), 18 deletions(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: remove unused skip

PR-URL: #54888
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 5dc478195c] test: remove unused skip
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Sun Sep 15 10:46:36 2024 +0200
1 file changed, 3 insertions(+), 3 deletions(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: add todo test

PR-URL: #54888
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 72ccb24833] test: add todo test
Author: Pietro Marchini <pietro.marchini94@gmail.com>
Date: Sun Sep 15 10:46:36 2024 +0200
1 file changed, 7 insertions(+), 6 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/10960018630

(Ignore my last [deleted] comment) I've added commit-queue-squash to this PR, it should be good to commit now.

Landed in e35299a

PR-URL: #54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 11, 2024

cjihrig reviewed Sep 13, 2024

View reviewed changes

cjihrig approved these changes Sep 14, 2024

View reviewed changes

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 14, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 14, 2024

pmarchini added 3 commits September 15, 2024 10:46

test: add runner watch mode isolation tests

37654fe

test: remove unused skip

dbb8292

test: add todo test

0eab852

pmarchini force-pushed the test/test-runner-watch-mode-isolation branch from 63a09ed to 0eab852 Compare September 15, 2024 08:49

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 15, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 15, 2024

cjihrig approved these changes Sep 17, 2024

View reviewed changes

cjihrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels Sep 17, 2024

mcollina approved these changes Sep 20, 2024

View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 20, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 20, 2024

avivkeller removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 20, 2024

cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 20, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 20, 2024

nodejs-github-bot merged commit e35299a into nodejs:main Sep 20, 2024

targos pushed a commit that referenced this pull request Oct 4, 2024

test: add runner watch mode isolation tests

98ff615

PR-URL: #54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

aduh95 mentioned this pull request Oct 9, 2024

V22.10.0 proposal #55343

Merged

marco-ippolito added the dont-land-on-v20.x label Nov 16, 2024

test: add runner watch mode isolation tests by pmarchini · Pull Request #54888 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: add runner watch mode isolation tests - #54888

test: add runner watch mode isolation tests#54888nodejs-github-bot merged 3 commits intonodejs:mainnodejs/node:mainfrom pmarchini:test/test-runner-watch-mode-isolationpmarchini/node:test/test-runner-watch-mode-isolationCopy head branch name to clipboard

Conversation

pmarchini commented Sep 11, 2024

Uh oh!

nodejs-github-bot commented Sep 11, 2024

Uh oh!

cjihrig commented Sep 11, 2024

Uh oh!

codecov Bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Sep 14, 2024

Choose a reason for hiding this comment

Uh oh!

cjihrig Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Sep 14, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini commented Sep 14, 2024

Uh oh!

cjihrig commented Sep 14, 2024

Uh oh!

pmarchini commented Sep 14, 2024

Uh oh!

nodejs-github-bot commented Sep 14, 2024

Uh oh!

nodejs-github-bot commented Sep 15, 2024

Uh oh!

nodejs-github-bot commented Sep 17, 2024 • edited by jakecastelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 20, 2024

PR-URL: #54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Successfully rebased and updated refs/heads/main.

Uh oh!

avivkeller commented Sep 20, 2024

Uh oh!

nodejs-github-bot commented Sep 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: add runner watch mode isolation tests#54888
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
pmarchini:test/test-runner-watch-mode-isolation

codecov Bot commented Sep 11, 2024 •
edited
Loading

nodejs-github-bot commented Sep 17, 2024 •
edited by jakecastelli
Loading

PR-URL: #54888
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #54888
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #54888
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>