From b0051de23bee04b57c6b2291c788aa71356ca315 Mon Sep 17 00:00:00 2001 From: Carlos Espa <43477095+Ceres6@users.noreply.github.com> Date: Sun, 17 Nov 2024 11:52:05 +0100 Subject: [PATCH] fs: prevent unwanted `dependencyOwners` removal Remove files from watcher `dependencyOwners` on file change only if it has no other owners. Co-authored-by: Pietro Marchini PR-URL: https://github.com/nodejs/node/pull/55565 Reviewed-By: Pietro Marchini Reviewed-By: James M Snell --- lib/internal/watch_mode/files_watcher.js | 5 +- .../test-runner-complex-dependencies.mjs | 126 ++++++++++++++++++ .../test-watch-file-shared-dependency.mjs | 54 ++++++++ 3 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-runner-complex-dependencies.mjs create mode 100644 test/parallel/test-watch-file-shared-dependency.mjs diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js index f917a3767e3..112f0b8be53 100644 --- a/lib/internal/watch_mode/files_watcher.js +++ b/lib/internal/watch_mode/files_watcher.js @@ -180,7 +180,10 @@ class FilesWatcher extends EventEmitter { owners.forEach((owner) => { this.#ownerDependencies.get(owner)?.forEach((dependency) => { this.#filteredFiles.delete(dependency); - this.#dependencyOwners.delete(dependency); + this.#dependencyOwners.get(dependency)?.delete(owner); + if (this.#dependencyOwners.get(dependency)?.size === 0) { + this.#dependencyOwners.delete(dependency); + } }); this.#filteredFiles.delete(owner); this.#dependencyOwners.delete(owner); diff --git a/test/parallel/test-runner-complex-dependencies.mjs b/test/parallel/test-runner-complex-dependencies.mjs new file mode 100644 index 00000000000..e8aa9630d12 --- /dev/null +++ b/test/parallel/test-runner-complex-dependencies.mjs @@ -0,0 +1,126 @@ +// Flags: --expose-internals +import * as common from '../common/index.mjs'; +import { describe, it } from 'node:test'; +import assert from 'node:assert'; +import { spawn } from 'node:child_process'; +import { writeFileSync } from 'node:fs'; +import tmpdir from '../common/tmpdir.js'; + +if (common.isIBMi) + common.skip('IBMi does not support `fs.watch()`'); + +if (common.isAIX) + common.skip('folder watch capability is limited in AIX.'); + +tmpdir.refresh(); + +// Set up test files and dependencies +const fixtureContent = { + 'dependency.js': 'module.exports = {};', + 'test.js': ` +const test = require('node:test'); +require('./dependency.js'); +test('first test has ran');`, + 'test-2.js': ` +const test = require('node:test'); +require('./dependency.js'); +test('second test has ran');`, +}; + +const fixturePaths = Object.fromEntries(Object.keys(fixtureContent) + .map((file) => [file, tmpdir.resolve(file)])); + +Object.entries(fixtureContent) + .forEach(([file, content]) => writeFileSync(fixturePaths[file], content)); + +describe('test runner watch mode with more complex setup', () => { + it('should re-run appropriate tests when dependencies change', async () => { + // Start the test runner in watch mode + const child = spawn(process.execPath, + ['--watch', '--test'], + { encoding: 'utf8', stdio: 'pipe', cwd: tmpdir.path }); + + let currentRunOutput = ''; + const testRuns = []; + + const firstRunCompleted = Promise.withResolvers(); + const secondRunCompleted = Promise.withResolvers(); + const thirdRunCompleted = Promise.withResolvers(); + const fourthRunCompleted = Promise.withResolvers(); + + child.stdout.on('data', (data) => { + const str = data.toString(); + currentRunOutput += str; + + if (/duration_ms\s\d+/.test(str)) { + // Test run has completed + testRuns.push(currentRunOutput); + currentRunOutput = ''; + switch (testRuns.length) { + case 1: + firstRunCompleted.resolve(); + break; + case 2: + secondRunCompleted.resolve(); + break; + case 3: + thirdRunCompleted.resolve(); + break; + case 4: + fourthRunCompleted.resolve(); + break; + } + } + }); + + // Wait for the initial test run to complete + await firstRunCompleted.promise; + + // Modify 'dependency.js' to trigger re-run of both tests + writeFileSync(fixturePaths['dependency.js'], 'module.exports = { modified: true };'); + + // Wait for the second test run to complete + await secondRunCompleted.promise; + + // Modify 'test.js' to trigger re-run of only 'test.js' + writeFileSync(fixturePaths['test.js'], ` +const test = require('node:test'); +require('./dependency.js'); +test('first test has ran again');`); + + // Wait for the third test run to complete + await thirdRunCompleted.promise; + + // Modify 'dependency.js' again to trigger re-run of both tests + writeFileSync(fixturePaths['dependency.js'], 'module.exports = { modified: true, again: true };'); + + // Wait for the fourth test run to complete + await fourthRunCompleted.promise; + + // Kill the child process + child.kill(); + + // Analyze the test runs + assert.strictEqual(testRuns.length, 4); + + // First test run - Both tests should run + const firstRunOutput = testRuns[0]; + assert.match(firstRunOutput, /first test has ran/); + assert.match(firstRunOutput, /second test has ran/); + + // Second test run - We have modified 'dependency.js' only, so both tests should re-run + const secondRunOutput = testRuns[1]; + assert.match(secondRunOutput, /first test has ran/); + assert.match(secondRunOutput, /second test has ran/); + + // Third test run - We have modified 'test.js' only + const thirdRunOutput = testRuns[2]; + assert.match(thirdRunOutput, /first test has ran again/); + assert.doesNotMatch(thirdRunOutput, /second test has ran/); + + // Fourth test run - We have modified 'dependency.js' again, so both tests should re-run + const fourthRunOutput = testRuns[3]; + assert.match(fourthRunOutput, /first test has ran again/); + assert.match(fourthRunOutput, /second test has ran/); + }); +}); diff --git a/test/parallel/test-watch-file-shared-dependency.mjs b/test/parallel/test-watch-file-shared-dependency.mjs new file mode 100644 index 00000000000..37ac647f823 --- /dev/null +++ b/test/parallel/test-watch-file-shared-dependency.mjs @@ -0,0 +1,54 @@ +// Flags: --expose-internals +import * as common from '../common/index.mjs'; +import { describe, it } from 'node:test'; +import assert from 'node:assert'; +import tmpdir from '../common/tmpdir.js'; +import watcher from 'internal/watch_mode/files_watcher'; +import { writeFileSync } from 'node:fs'; + +if (common.isIBMi) + common.skip('IBMi does not support `fs.watch()`'); + +if (common.isAIX) + common.skip('folder watch capability is limited in AIX.'); + +tmpdir.refresh(); + +const { FilesWatcher } = watcher; + +tmpdir.refresh(); + +// Set up test files and dependencies +const fixtureContent = { + 'dependency.js': 'module.exports = {};', + 'test.js': 'require(\'./dependency.js\');', + 'test-2.js': 'require(\'./dependency.js\');', +}; + +const fixturePaths = Object.fromEntries(Object.keys(fixtureContent) + .map((file) => [file, tmpdir.resolve(file)])); + +Object.entries(fixtureContent) + .forEach(([file, content]) => writeFileSync(fixturePaths[file], content)); + +describe('watch file with shared dependency', () => { + it('should not remove shared dependencies when unfiltering an owner', () => { + const controller = new AbortController(); + const watcher = new FilesWatcher({ signal: controller.signal, debounce: 200 }); + + watcher.on('changed', ({ owners }) => { + assert.strictEqual(owners.size, 2); + assert.ok(owners.has(fixturePaths['test.js'])); + assert.ok(owners.has(fixturePaths['test-2.js'])); + controller.abort(); + }); + watcher.filterFile(fixturePaths['test.js']); + watcher.filterFile(fixturePaths['test-2.js']); + watcher.filterFile(fixturePaths['dependency.js'], fixturePaths['test.js']); + watcher.filterFile(fixturePaths['dependency.js'], fixturePaths['test-2.js']); + watcher.unfilterFilesOwnedBy([fixturePaths['test.js']]); + watcher.filterFile(fixturePaths['test.js']); + watcher.filterFile(fixturePaths['dependency.js'], fixturePaths['test.js']); + writeFileSync(fixturePaths['dependency.js'], 'module.exports = { modified: true };'); + }); +});