test: refactor fs-watch tests due to macOS issue

In `macOS`, fsevents generated immediately before start watching may leak into the event callback. See: #54450 for an explanation. This might be fixed at some point in `libuv` though it may take some time (see: libuv/libuv#3866). This commit comes in anticipation of the soon-to-be-released `[email protected]` which was making these tests very flaky. PR-URL: #54498 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
nodejs · Sep 12, 2024 · 9601169 · 9601169
1 parent 2c2ae80
commit 9601169
Show file tree

Hide file tree

Showing 8 changed files with 142 additions and 66 deletions.
diff --git a/test/parallel/test-fs-promises-watch.js b/test/parallel/test-fs-promises-watch.js
@@ -8,6 +8,7 @@ const { watch } = require('fs/promises');
 const fs = require('fs');
 const assert = require('assert');
 const { join } = require('path');
+const { setTimeout } = require('timers/promises');
 const tmpdir = require('../common/tmpdir');
 
 class WatchTestCase {
@@ -49,6 +50,12 @@ for (const testCase of kCases) {
 
   let interval;
   async function test() {
+    if (common.isMacOS) {
+      // On macOS delay watcher start to avoid leaking previous events.
+      // Refs: https://github.com/libuv/libuv/pull/4503
+      await setTimeout(common.platformTimeout(100));
+    }
+
     const watcher = watch(testCase[testCase.field]);
     for await (const { eventType, filename } of watcher) {
       clearInterval(interval);

diff --git a/test/parallel/test-fs-watch-recursive-add-file-to-new-folder.js b/test/parallel/test-fs-watch-recursive-add-file-to-new-folder.js
@@ -31,24 +31,34 @@ const filePath = path.join(testDirectory, 'folder-3');
 const childrenFile = 'file-4.txt';
 const childrenAbsolutePath = path.join(filePath, childrenFile);
 const childrenRelativePath = path.join(path.basename(filePath), childrenFile);
-
-const watcher = fs.watch(testDirectory, { recursive: true });
 let watcherClosed = false;
-watcher.on('change', function(event, filename) {
-  assert.strictEqual(event, 'rename');
-  assert.ok(filename === path.basename(filePath) || filename === childrenRelativePath);
-
-  if (filename === childrenRelativePath) {
-    watcher.close();
-    watcherClosed = true;
-  }
-});
 
-// Do the write with a delay to ensure that the OS is ready to notify us.
-setTimeout(() => {
-  fs.mkdirSync(filePath);
-  fs.writeFileSync(childrenAbsolutePath, 'world');
-}, common.platformTimeout(200));
+function doWatch() {
+  const watcher = fs.watch(testDirectory, { recursive: true });
+  watcher.on('change', function(event, filename) {
+    assert.strictEqual(event, 'rename');
+    assert.ok(filename === path.basename(filePath) || filename === childrenRelativePath);
+
+    if (filename === childrenRelativePath) {
+      watcher.close();
+      watcherClosed = true;
+    }
+  });
+
+  // Do the write with a delay to ensure that the OS is ready to notify us.
+  setTimeout(() => {
+    fs.mkdirSync(filePath);
+    fs.writeFileSync(childrenAbsolutePath, 'world');
+  }, common.platformTimeout(200));
+}
+
+if (common.isMacOS) {
+  // On macOS delay watcher start to avoid leaking previous events.
+  // Refs: https://github.com/libuv/libuv/pull/4503
+  setTimeout(doWatch, common.platformTimeout(100));
+} else {
+  doWatch();
+}
 
 process.once('exit', function() {
   assert(watcherClosed, 'watcher Object was not closed');

diff --git a/test/parallel/test-fs-watch-recursive-symlink.js b/test/parallel/test-fs-watch-recursive-symlink.js
@@ -35,6 +35,11 @@ tmpdir.refresh();
   const symlinkFolder = path.join(rootDirectory, 'symlink-folder');
   fs.symlinkSync(rootDirectory, symlinkFolder);
 
+  if (common.isMacOS) {
+    // On macOS delay watcher start to avoid leaking previous events.
+    // Refs: https://github.com/libuv/libuv/pull/4503
+    await setTimeout(common.platformTimeout(100));
+  }
 
   const watcher = fs.watch(rootDirectory, { recursive: true });
   let watcherClosed = false;
@@ -74,6 +79,12 @@ tmpdir.refresh();
   const forbiddenFile = path.join(subDirectory, 'forbidden.txt');
   const acceptableFile = path.join(trackingSubDirectory, 'acceptable.txt');
 
+  if (common.isMacOS) {
+    // On macOS delay watcher start to avoid leaking previous events.
+    // Refs: https://github.com/libuv/libuv/pull/4503
+    await setTimeout(common.platformTimeout(100));
+  }
+
   const watcher = fs.watch(trackingSubDirectory, { recursive: true });
   let watcherClosed = false;
   watcher.on('change', function(event, filename) {

diff --git a/test/parallel/test-fs-watch-recursive-sync-write.js b/test/parallel/test-fs-watch-recursive-sync-write.js
@@ -25,14 +25,24 @@ const keepalive = setTimeout(() => {
   throw new Error('timed out');
 }, common.platformTimeout(30_000));
 
-const watcher = watch(tmpDir, { recursive: true }, common.mustCall((eventType, _filename) => {
-  clearTimeout(keepalive);
-  watcher.close();
-  assert.strictEqual(eventType, 'rename');
-  assert.strictEqual(join(tmpDir, _filename), filename);
-}));
-
-// Do the write with a delay to ensure that the OS is ready to notify us.
-setTimeout(() => {
-  writeFileSync(filename, 'foobar2');
-}, common.platformTimeout(200));
+function doWatch() {
+  const watcher = watch(tmpDir, { recursive: true }, common.mustCall((eventType, _filename) => {
+    clearTimeout(keepalive);
+    watcher.close();
+    assert.strictEqual(eventType, 'rename');
+    assert.strictEqual(join(tmpDir, _filename), filename);
+  }));
+
+  // Do the write with a delay to ensure that the OS is ready to notify us.
+  setTimeout(() => {
+    writeFileSync(filename, 'foobar2');
+  }, common.platformTimeout(200));
+}
+
+if (common.isMacOS) {
+  // On macOS delay watcher start to avoid leaking previous events.
+  // Refs: https://github.com/libuv/libuv/pull/4503
+  setTimeout(doWatch, common.platformTimeout(100));
+} else {
+  doWatch();
+}
diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js
@@ -41,13 +41,7 @@ const cases = [
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
 
-for (const testCase of cases) {
-  if (testCase.shouldSkip) continue;
-  fs.mkdirSync(testCase.dirPath);
-  // Long content so it's actually flushed.
-  const content1 = Date.now() + testCase.fileName.toLowerCase().repeat(1e4);
-  fs.writeFileSync(testCase.filePath, content1);
-
+function doWatchTest(testCase) {
   let interval;
   const pathToWatch = testCase[testCase.field];
   const watcher = fs.watch(pathToWatch);
@@ -87,6 +81,23 @@ for (const testCase of cases) {
   }, 100);
 }
 
+for (const testCase of cases) {
+  if (testCase.shouldSkip) continue;
+  fs.mkdirSync(testCase.dirPath);
+  // Long content so it's actually flushed.
+  const content1 = Date.now() + testCase.fileName.toLowerCase().repeat(1e4);
+  fs.writeFileSync(testCase.filePath, content1);
+  if (common.isMacOS) {
+    // On macOS delay watcher start to avoid leaking previous events.
+    // Refs: https://github.com/libuv/libuv/pull/4503
+    setTimeout(() => {
+      doWatchTest(testCase);
+    }, common.platformTimeout(100));
+  } else {
+    doWatchTest(testCase);
+  }
+}
+
 [false, 1, {}, [], null, undefined].forEach((input) => {
   assert.throws(
     () => fs.watch(input, common.mustNotCall()),

diff --git a/test/parallel/test-fs-watchfile.js b/test/parallel/test-fs-watchfile.js
@@ -86,10 +86,7 @@ watcher.on('stop', common.mustCall());
 // Omitting AIX. It works but not reliably.
 if (common.isLinux || common.isMacOS || common.isWindows) {
   const dir = tmpdir.resolve('watch');
-
-  fs.mkdir(dir, common.mustCall(function(err) {
-    if (err) assert.fail(err);
-
+  function doWatch() {
     const handle = fs.watch(dir, common.mustCall(function(eventType, filename) {
       clearInterval(interval);
       handle.close();
@@ -101,5 +98,15 @@ if (common.isLinux || common.isMacOS || common.isWindows) {
         if (err) assert.fail(err);
       }));
     }, 1);
+  }
+
+  fs.mkdir(dir, common.mustSucceed(() => {
+    if (common.isMacOS) {
+      // On macOS delay watcher start to avoid leaking previous events.
+      // Refs: https://github.com/libuv/libuv/pull/4503
+      setTimeout(doWatch, common.platformTimeout(100));
+    } else {
+      doWatch();
+    }
   }));
 }
diff --git a/test/pummel/test-fs-watch-non-recursive.js b/test/pummel/test-fs-watch-non-recursive.js
@@ -38,14 +38,24 @@ const filepath = path.join(testsubdir, 'watch.txt');
 
 fs.mkdirSync(testsubdir, 0o700);
 
-const watcher = fs.watch(testDir, { persistent: true }, (event, filename) => {
-  // This function may be called with the directory depending on timing but
-  // must not be called with the file..
-  assert.strictEqual(filename, 'testsubdir');
-});
-setTimeout(() => {
-  fs.writeFileSync(filepath, 'test');
-}, 100);
-setTimeout(() => {
-  watcher.close();
-}, 500);
+function doWatch() {
+  const watcher = fs.watch(testDir, { persistent: true }, (event, filename) => {
+    // This function may be called with the directory depending on timing but
+    // must not be called with the file..
+    assert.strictEqual(filename, 'testsubdir');
+  });
+  setTimeout(() => {
+    fs.writeFileSync(filepath, 'test');
+  }, 100);
+  setTimeout(() => {
+    watcher.close();
+  }, 500);
+}
+
+if (common.isMacOS) {
+  // On macOS delay watcher start to avoid leaking previous events.
+  // Refs: https://github.com/libuv/libuv/pull/4503
+  setTimeout(doWatch, common.platformTimeout(100));
+} else {
+  doWatch();
+}
diff --git a/test/sequential/test-fs-watch.js b/test/sequential/test-fs-watch.js
@@ -95,24 +95,34 @@ function repeat(fn) {
   const testsubdir = fs.mkdtempSync(testDir + path.sep);
   const filepath = path.join(testsubdir, 'newfile.txt');
 
-  const watcher =
-    fs.watch(testsubdir, common.mustCall(function(event, filename) {
-      const renameEv = common.isSunOS || common.isAIX ? 'change' : 'rename';
-      assert.strictEqual(event, renameEv);
-      if (expectFilePath) {
-        assert.strictEqual(filename, 'newfile.txt');
-      } else {
-        assert.strictEqual(filename, null);
-      }
-      clearInterval(interval);
-      watcher.close();
-    }));
+  function doWatch() {
+    const watcher =
+      fs.watch(testsubdir, common.mustCall(function(event, filename) {
+        const renameEv = common.isSunOS || common.isAIX ? 'change' : 'rename';
+        assert.strictEqual(event, renameEv);
+        if (expectFilePath) {
+          assert.strictEqual(filename, 'newfile.txt');
+        } else {
+          assert.strictEqual(filename, null);
+        }
+        clearInterval(interval);
+        watcher.close();
+      }));
+
+    const interval = repeat(() => {
+      fs.rmSync(filepath, { force: true });
+      const fd = fs.openSync(filepath, 'w');
+      fs.closeSync(fd);
+    });
+  }
 
-  const interval = repeat(() => {
-    fs.rmSync(filepath, { force: true });
-    const fd = fs.openSync(filepath, 'w');
-    fs.closeSync(fd);
-  });
+  if (common.isMacOS) {
+    // On macOS delay watcher start to avoid leaking previous events.
+    // Refs: https://github.com/libuv/libuv/pull/4503
+    setTimeout(doWatch, common.platformTimeout(100));
+  } else {
+    doWatch();
+  }
 }
 
 // https://github.com/joyent/node/issues/2293 - non-persistent watcher should