test: remove interval and give more time to unsync by pmarchini · Pull Request #55006 · nodejs/node

pmarchini · 2024-09-19T10:07:16Z

This PR should address #54807.

I'm leaving this in Draft while testing more extensively 🚀

nodejs-github-bot · 2024-09-19T10:07:22Z

Review requested:

@nodejs/test_runner

avivkeller · 2024-09-19T11:29:43Z

test/parallel/test-runner-watch-mode-complex.mjs

+    await new Promise((resolve) => setTimeout(() => {
+      unlinkSync(fileToDeletePathLocal);
+      resolve();
+    }, common.platformTimeout(1000)));


Can/Should this be simplified with node:timers/promises?

I didn't think about it, thanks for the suggestion, I'll take a look 😁

Would using node:fs/promises be enough here? It seems weird to have to put a sync operation in an async one

Hey @aduh95, yes, no problem at all.
The only important logic is the "sleep" after the delete to ensure that the watcher has received the event and completed the test run before proceeding.
Thanks for your feedback, I'll fixit ASAP 😁

just update 😁

codecov · 2024-09-19T12:10:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.25%. Comparing base (0d4387e) to head (e654e80).
Report is 483 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55006      +/-   ##
==========================================
+ Coverage   88.04%   88.25%   +0.20%     
==========================================
  Files         652      651       -1     
  Lines      183765   183877     +112     
  Branches    35863    35857       -6     
==========================================
+ Hits       161789   162273     +484     
+ Misses      15229    14898     -331     
+ Partials     6747     6706      -41

see 108 files with indirect coverage changes

nodejs-github-bot · 2024-09-21T16:34:15Z

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

cjihrig

LGTM with some minor comments.

If this fixes #54807, can you add that to this PR so that the issue gets closed.

cjihrig · 2024-09-21T16:36:19Z

test/parallel/test-runner-watch-mode-complex.mjs

+function wait(ms) {
+  return new Promise((resolve) => setTimeout(resolve, ms));
+}


You can use setTimeout() from node:timers/promises for this exact functionality.

test/parallel/test-runner-watch-mode-complex.mjs

cjihrig

LGTM, thanks!

jakecastelli · 2024-09-23T08:03:38Z

I see this test fail a lot on windows platform, could we queue a stress test?

aduh95 · 2024-09-23T09:29:06Z

Stress test CI: https://ci.nodejs.org/view/Stress/job/node-stress-single-test/538/ 🟢

mcollina

You might want to use the flush option of writeFile to cause an fsync: https://nodejs.org/docs/latest/api/fs.html#fspromiseswritefilefile-data-options.

nodejs-github-bot · 2024-09-24T07:19:58Z

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

nodejs-github-bot · 2024-09-24T10:30:19Z

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

nodejs-github-bot · 2024-09-24T14:40:21Z

Landed in 02e8972

PR-URL: #55006 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>

PR-URL: nodejs#55006 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>

Refs: nodejs#55006 Refs: nodejs#54807 (comment)

Refs: #55006 Refs: #54807 (comment) PR-URL: #56470 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>

Refs: nodejs#55006 Refs: nodejs#54807 (comment) PR-URL: nodejs#56470 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@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 19, 2024

pmarchini marked this pull request as ready for review September 19, 2024 10:25

test: remove interval and give more time to unsync

9daffe6

pmarchini force-pushed the issue/54807 branch from e6e0999 to 9daffe6 Compare September 19, 2024 10:32

avivkeller added the test_runner Issues and PRs related to the test runner subsystem. label Sep 19, 2024

avivkeller reviewed Sep 19, 2024

View reviewed changes

feat: use node:fs/promises

48930c6

pmarchini force-pushed the issue/54807 branch from 140ae8d to 48930c6 Compare September 20, 2024 16:17

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

jasnell approved these changes Sep 21, 2024

View reviewed changes

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

cjihrig reviewed Sep 21, 2024

View reviewed changes

test: apply suggestions

963ab6c

cjihrig approved these changes Sep 21, 2024

View reviewed changes

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

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

This comment was marked as outdated.

Sign in to view

mcollina reviewed Sep 23, 2024

View reviewed changes

test: set flush option

e654e80

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

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

cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Sep 24, 2024

cjihrig approved these changes Sep 24, 2024

View reviewed changes

jakecastelli approved these changes Sep 24, 2024

View reviewed changes

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

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

nodejs-github-bot merged commit 02e8972 into nodejs:main Sep 24, 2024

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

test: remove interval and give more time to unsync

80da599

PR-URL: #55006 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@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 PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024

lpinca added a commit to lpinca/node that referenced this pull request Jan 4, 2025

test: remove test-runner-watch-mode-complex flaky designation

0faa6ed

Refs: nodejs#55006 Refs: nodejs#54807 (comment)

lpinca mentioned this pull request Jan 4, 2025

test: remove test-runner-watch-mode-complex flaky designation #56470

Merged

Uh oh!

Conversation

pmarchini commented Sep 19, 2024

Uh oh!

nodejs-github-bot commented Sep 19, 2024

Uh oh!

avivkeller Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

aduh95 Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Sep 21, 2024

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchini Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

jakecastelli commented Sep 23, 2024

Uh oh!

aduh95 commented Sep 23, 2024 • edited by cjihrig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 24, 2024

Uh oh!

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

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov bot commented Sep 19, 2024 •

edited

Loading

aduh95 commented Sep 23, 2024 •

edited by cjihrig

Loading

nodejs-github-bot commented Sep 24, 2024 •

edited by jakecastelli

Loading