Skip to content
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.

Sign up for GitHub

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

Jump to bottom

investigate flaky test-fs-promises-watch on macOS #37637

Closed
Trott opened this issue Mar 6, 2021 · 2 comments · Fixed by #40863
Closed

investigate flaky test-fs-promises-watch on macOS #37637

Trott opened this issue Mar 6, 2021 · 2 comments · Fixed by #40863
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system.

Comments

@Trott
Copy link
Member

Trott commented Mar 6, 2021

  • Test: test-fs-promises-watch
  • Platform: macOS 10.15
  • Console Output:
not ok 1225 parallel/test-fs-promises-watch
  ---
  duration_ms: 120.38
  severity: fail
  exitcode: -15
  stack: |-
    timeout
  ...
@Trott
Copy link
Member Author

Trott commented Mar 6, 2021

This is almost certainly a race condition whereby a watcher is returned but is not yet registering/reporting events, or something similar to that.

@Trott Trott added flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. labels Mar 6, 2021
@Linkgoron
Copy link
Member

Linkgoron commented Apr 3, 2021 •
edited
Loading

I think I've managed to reproduce this a few times. At least for me, it's consistently the directory check that fails. Maybe adding watchers on directories is somehow slower, and, as you've said, it creates a race condition on MacOS. Note that the callback-based test that this is based on actually does a setInterval whereas this test does setImmediate (well, it appears that this same issue was present last time and that's why it's using setinterval, although when I tested this test locally, a ~500ms unrefed timeout was also good enough, but maybe an interval is safer).

lpinca added a commit to lpinca/node that referenced this issue Nov 18, 2021
@lpinca
test: deflake fs-promises-watch
c79b3c0 
Change the contents of the file every 100 milliseconds until the watcher
notices the change. This is already done in the callback based version
of the test (`test-fs-watch.js`).

Fixes: nodejs#37637
@lpinca lpinca mentioned this issue Nov 18, 2021
Merged
nodejs-github-bot pushed a commit that referenced this issue Nov 21, 2021
@lpinca
test: deflake fs-promises-watch
c6352f5 
Change the contents of the file every 100 milliseconds until the watcher
notices the change. This is already done in the callback based version
of the test (`test-fs-watch.js`).

Fixes: #37637

PR-URL: #40863
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos pushed a commit that referenced this issue Nov 26, 2021
@lpinca @targos
test: deflake fs-promises-watch
009ab4d 
Change the contents of the file every 100 milliseconds until the watcher
notices the change. This is already done in the callback based version
of the test (`test-fs-watch.js`).

Fixes: #37637

PR-URL: #40863
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 30, 2022
@lpinca @danielleadams
test: deflake fs-promises-watch
a76558b 
Change the contents of the file every 100 milliseconds until the watcher
notices the change. This is already done in the callback based version
of the test (`test-fs-watch.js`).

Fixes: #37637

PR-URL: #40863
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
@lpinca @danielleadams
test: deflake fs-promises-watch
9e2e443 
Change the contents of the file every 100 milliseconds until the watcher
notices the change. This is already done in the callback based version
of the test (`test-fs-watch.js`).

Fixes: #37637

PR-URL: #40863
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: deflake fs-promises-watch lpinca/node
2 participants
@Trott @Linkgoron