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

+ 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

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


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

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>

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>

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 9daffe6Compare 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 48930c6Compare 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-actionsBot 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-actionsBot 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-actionsBot 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:mainSep 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 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!

Uh oh!

Conversation

pmarchini commented Sep 19, 2024

Uh oh!

nodejs-github-bot commented Sep 19, 2024

Uh oh!

avivkellerSep 19, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchiniSep 19, 2024

Choose a reason for hiding this comment

Uh oh!

aduh95Sep 19, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchiniSep 19, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchiniSep 20, 2024

Choose a reason for hiding this comment

Uh oh!

codecovBot 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!

cjihrigSep 21, 2024

Choose a reason for hiding this comment

Uh oh!

pmarchiniSep 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

codecovBot 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