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.

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

Investigate flaky test-fs-watch-non-recursive on Windows #40728

Closed
targos opened this issue Nov 5, 2021 · 10 comments
Closed

Investigate flaky test-fs-watch-non-recursive on Windows #40728

targos opened this issue Nov 5, 2021 · 10 comments
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. windows Issues and PRs related to the Windows platform.

Comments

@targos
Copy link
Member

targos commented Nov 5, 2021

  • Test: test-fs-watch-non-recursive
  • Platform: Windows
  • Console Output:
node:assert:171
  throw err;
  ^

AssertionError [ERR_ASSERTION]: function should not have been called at C:\workspace\node-test-binary-windows-js-suites\node\test\pummel\test-fs-watch-non-recursive.js:43
called with arguments: 'change', 'testsubdir'
    at FSWatcher.mustNotCall (C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:479:12)
    at FSWatcher.emit (node:events:390:28)
    at FSEvent.FSWatcher._handle.onchange (node:internal/fs/watchers:212:12) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: undefined,
  operator: 'fail'
}
@VoltrexKeyva VoltrexKeyva added flaky-test Issues and PRs related to the tests with unstable failures on the CI. windows Issues and PRs related to the Windows platform. labels Nov 5, 2021
@joyeecheung
Copy link
Member

It seems this first appears in nodejs/reliability#92, the referenced PRs don't seem to be related so I suspect it's an issue with the test.

@targos
Copy link
Member Author

targos commented Nov 8, 2021

I'm am totally unable to make this test pass on my Windows machine, even if I increase the timeouts.
Maybe the issue is with some recent Windows update?

@Mesteery Mesteery added the fs Issues and PRs related to the fs subsystem / file system. label Nov 8, 2021
@Trott
Copy link
Member

Trott commented Nov 9, 2021

@nodejs/platform-windows

@Flarna
Copy link
Member

Flarna commented Nov 14, 2021

It seems that the creation of file testsubdir/watch.txt changes the modification time of testsubdir - which seems correct to me.

I wonder that this test ever passed.

Adding e.g. fs.writeFileSync(filepath, 'init'); before starting the watcher avoids this and test passes on my machine. But I'm not sure if this changes the intention of test.

@joyeecheung
Copy link
Member

It seems that the creation of file testsubdir/watch.txt changes the modification time of testsubdir - which seems correct to me.

The commit that added this test was 515607a with the message

fs: make fs.watch() non-recursive by default
Fixes a behavioral regression introduced in commit 691b9eb.

So I guess the intended behavior is that the changes shouldn't be notified in this case.

I looked into the logs a bit more and it seems this first (as far as the reliability bot records) appeared in nodejs/reliability#68, none of the referenced PRs seem to be related, either. I'd say it's time to mark this as flaky already.

@lpinca
Copy link
Member

lpinca commented Nov 22, 2021

I've opened #40916 but I'm unable to reproduce the issue on my Windows machine.

$ npx envinfo --system

  System:
    OS: Windows 10 10.0.19044
    CPU: (8) x64 Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz
    Memory: 3.88 GB / 7.90 GB

@targos
Copy link
Member Author

targos commented Nov 22, 2021

Windows 10 10.0.19044

Is this the latest version of Windows 10?

@lpinca
Copy link
Member

lpinca commented Nov 22, 2021

Yes, updated a few minutes ago.

@Flarna
Copy link
Member

Flarna commented Nov 22, 2021

I tested again on a different (slower) Windows machine. Problem is not reproducible there but once I have VSCode running in background the test starts to fail (still executed without debugger from command line).

Maybe VSCode does also some watching which causes such side effects.

On the machine I tested first the test is flaky without VSCode running but fails stable if it runs in background.

@lpinca
Copy link
Member

lpinca commented Nov 22, 2021

I've VSCode and a virtual machine (Windows 10 on Hyper-V) running but I can't reproduce.

nodejs-github-bot pushed a commit that referenced this issue Nov 23, 2021
Refs: #40728

PR-URL: #40916
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue Nov 26, 2021
Refs: #40728

PR-URL: #40916
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Jan 30, 2022
Refs: #40728

PR-URL: #40916
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
Refs: #40728

PR-URL: #40916
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
lpinca added a commit to lpinca/node that referenced this issue Sep 23, 2024
This reverts commit 2a871df.

It has been almost three years since the test was marked flaky. Remove
the unstable designation to see if the problem persists on Windows.

Fixes: nodejs#40728
targos pushed a commit that referenced this issue Oct 4, 2024
This reverts commit 2a871df.

It has been almost three years since the test was marked flaky. Remove
the unstable designation to see if the problem persists on Windows.

Fixes: #40728
PR-URL: #55079
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
This reverts commit 2a871df.

It has been almost three years since the test was marked flaky. Remove
the unstable designation to see if the problem persists on Windows.

Fixes: nodejs#40728
PR-URL: nodejs#55079
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

7 participants