Skip to content

Commit

Permalink
test: refactor fs-watch tests due to macOS issue
Browse files Browse the repository at this point in the history
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
`libuv@1.49.0` which was making these tests very flaky.

PR-URL: #54498
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
  • Loading branch information
santigimeno authored and targos committed Oct 2, 2024
1 parent 62953ef commit 4f82673
Show file tree
Hide file tree
Showing 8 changed files with 142 additions and 66 deletions.
7 changes: 7 additions & 0 deletions test/parallel/test-fs-promises-watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down
42 changes: 26 additions & 16 deletions test/parallel/test-fs-watch-recursive-add-file-to-new-folder.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
11 changes: 11 additions & 0 deletions test/parallel/test-fs-watch-recursive-symlink.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
32 changes: 21 additions & 11 deletions test/parallel/test-fs-watch-recursive-sync-write.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
25 changes: 18 additions & 7 deletions test/parallel/test-fs-watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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()),
Expand Down
15 changes: 11 additions & 4 deletions test/parallel/test-fs-watchfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
}
}));
}
32 changes: 21 additions & 11 deletions test/pummel/test-fs-watch-non-recursive.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
44 changes: 27 additions & 17 deletions test/sequential/test-fs-watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 4f82673

Please sign in to comment.