-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add runner watch mode isolation tests #54888
test: add runner watch mode isolation tests #54888
Conversation
Review requested:
|
Without having investigated the discrepancy, I'd say they should behave the same. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. Two small comments. Have you been able to identify why the one test fails without isolation?
await testWatch({ fileToUpdate: 'test.js', action: 'rename', isolation }); | ||
}); | ||
|
||
it('should not throw when delete a watched test file', { skip: common.isAIX }, async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already skip the entire file at the very top when running on AIX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
await testWatch({ fileToUpdate: 'test.js', action: 'delete', isolation }); | ||
}); | ||
|
||
if (isolation !== 'none') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😁)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated! 🚀
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 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 😄 |
I'll definitely give it a try 😁 |
63a09ed
to
0eab852
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)https://github.com/nodejs/node/actions/runs/10960018630 |
(Ignore my last [deleted] comment) I've added |
Landed in e35299a |
PR-URL: #54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#54888 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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?