From b0095135718e4fc64f2c988da6fc31dccb535cb9 Mon Sep 17 00:00:00 2001 From: Carlos Espa Date: Sun, 27 Oct 2024 20:00:31 +0100 Subject: [PATCH 1/5] test: prevent unwanted dependencyOwners removal remove file from watcher dependencyOwners on file change only if it has no other owners. Co-authored-by: Pietro Marchini --- lib/internal/watch_mode/files_watcher.js | 5 +- .../test-runner-complex-dependencies.mjs | 129 ++++++++++++++++++ 2 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-runner-complex-dependencies.mjs diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js index f917a3767e3235..112f0b8be53c50 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 00000000000000..860d08d63cb718 --- /dev/null +++ b/test/parallel/test-runner-complex-dependencies.mjs @@ -0,0 +1,129 @@ +// 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 util from 'internal/util'; +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', () => { + // This test is failing and needs to be fixed + // The expected behavior is that the test runner should re-run the appropriate tests when a shared dependency changes + it.todo('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 = util.createDeferredPromise(); + const secondRunCompleted = util.createDeferredPromise(); + const thirdRunCompleted = util.createDeferredPromise(); + const fourthRunCompleted = util.createDeferredPromise(); + + 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/); + }); +}); From 02b7df5669d99b734439d26650bb8fa1b5403df9 Mon Sep 17 00:00:00 2001 From: Carlos Espa Date: Wed, 30 Oct 2024 12:24:13 +0100 Subject: [PATCH 2/5] fixup! fs: prevent unwanted dependencyOwners removal --- test/parallel/test-runner-complex-dependencies.mjs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/test/parallel/test-runner-complex-dependencies.mjs b/test/parallel/test-runner-complex-dependencies.mjs index 860d08d63cb718..e8aa9630d12af0 100644 --- a/test/parallel/test-runner-complex-dependencies.mjs +++ b/test/parallel/test-runner-complex-dependencies.mjs @@ -4,7 +4,6 @@ import { describe, it } from 'node:test'; import assert from 'node:assert'; import { spawn } from 'node:child_process'; import { writeFileSync } from 'node:fs'; -import util from 'internal/util'; import tmpdir from '../common/tmpdir.js'; if (common.isIBMi) @@ -35,9 +34,7 @@ Object.entries(fixtureContent) .forEach(([file, content]) => writeFileSync(fixturePaths[file], content)); describe('test runner watch mode with more complex setup', () => { - // This test is failing and needs to be fixed - // The expected behavior is that the test runner should re-run the appropriate tests when a shared dependency changes - it.todo('should re-run appropriate tests when dependencies change', async () => { + it('should re-run appropriate tests when dependencies change', async () => { // Start the test runner in watch mode const child = spawn(process.execPath, ['--watch', '--test'], @@ -46,10 +43,10 @@ describe('test runner watch mode with more complex setup', () => { let currentRunOutput = ''; const testRuns = []; - const firstRunCompleted = util.createDeferredPromise(); - const secondRunCompleted = util.createDeferredPromise(); - const thirdRunCompleted = util.createDeferredPromise(); - const fourthRunCompleted = util.createDeferredPromise(); + 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(); From bd9a3ed229506aa863dddaa6036a712a8d0767a7 Mon Sep 17 00:00:00 2001 From: Carlos Espa Date: Wed, 30 Oct 2024 18:23:35 +0100 Subject: [PATCH 3/5] fixup! fixup! fs: prevent unwanted dependencyOwners removal --- .../test-watch-file-shared-dependency.mjs | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 test/parallel/test-watch-file-shared-dependency.mjs 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 00000000000000..c307ab5b579625 --- /dev/null +++ b/test/parallel/test-watch-file-shared-dependency.mjs @@ -0,0 +1,53 @@ +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 };'); + }); +}); From 754699d9dd508f9c384f4aae4c01e91f8e4e57a8 Mon Sep 17 00:00:00 2001 From: Carlos Espa Date: Wed, 30 Oct 2024 18:37:21 +0100 Subject: [PATCH 4/5] fixup! fixup! fixup! fs: prevent unwanted dependencyOwners removal --- test/parallel/test-watch-file-shared-dependency.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-watch-file-shared-dependency.mjs b/test/parallel/test-watch-file-shared-dependency.mjs index c307ab5b579625..c35c1ffdc1c861 100644 --- a/test/parallel/test-watch-file-shared-dependency.mjs +++ b/test/parallel/test-watch-file-shared-dependency.mjs @@ -22,7 +22,7 @@ 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)])); From 44f0da079c643b757ce9c62030c4fdc48b48b6a9 Mon Sep 17 00:00:00 2001 From: Carlos Espa <43477095+Ceres6@users.noreply.github.com> Date: Thu, 31 Oct 2024 09:25:46 +0100 Subject: [PATCH 5/5] Update test/parallel/test-watch-file-shared-dependency.mjs Co-authored-by: Pietro Marchini --- test/parallel/test-watch-file-shared-dependency.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/parallel/test-watch-file-shared-dependency.mjs b/test/parallel/test-watch-file-shared-dependency.mjs index c35c1ffdc1c861..37ac647f82310e 100644 --- a/test/parallel/test-watch-file-shared-dependency.mjs +++ b/test/parallel/test-watch-file-shared-dependency.mjs @@ -1,3 +1,4 @@ +// Flags: --expose-internals import * as common from '../common/index.mjs'; import { describe, it } from 'node:test'; import assert from 'node:assert';