From a3f56917f083711e991da27afb037386403b89c2 Mon Sep 17 00:00:00 2001 From: jakecastelli <38635403+jakecastelli@users.noreply.github.com> Date: Sun, 2 Jun 2024 17:41:30 +0930 Subject: [PATCH] test_runner: handle file rename and deletion under watch mode Fixes: https://github.com/nodejs/node/issues/53113 PR-URL: https://github.com/nodejs/node/pull/53114 Reviewed-By: Moshe Atlow Reviewed-By: Chemi Atlow Reviewed-By: Raz Luvaton --- lib/internal/test_runner/runner.js | 18 ++++- lib/internal/watch_mode/files_watcher.js | 6 +- test/parallel/test-runner-watch-mode.mjs | 86 +++++++++++++++++++----- 3 files changed, 88 insertions(+), 22 deletions(-) diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index d95619a773c3b6..c625024d4260f8 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -3,6 +3,7 @@ const { ArrayIsArray, ArrayPrototypeEvery, ArrayPrototypeFilter, + ArrayPrototypeFind, ArrayPrototypeForEach, ArrayPrototypeIncludes, ArrayPrototypeJoin, @@ -417,7 +418,22 @@ function watchFiles(testFiles, opts) { const filesWatcher = { __proto__: null, watcher, runningProcesses, runningSubtests }; opts.root.harness.watching = true; - watcher.on('changed', ({ owners }) => { + watcher.on('changed', ({ owners, eventType }) => { + if (eventType === 'rename') { + const updatedTestFiles = createTestFileList(); + + const newFileName = ArrayPrototypeFind(updatedTestFiles, (x) => !ArrayPrototypeIncludes(testFiles, x)); + const previousFileName = ArrayPrototypeFind(testFiles, (x) => !ArrayPrototypeIncludes(updatedTestFiles, x)); + + // When file renamed + if (newFileName && previousFileName) { + owners = new SafeSet().add(newFileName); + watcher.filterFile(resolve(newFileName), owners); + } + + testFiles = updatedTestFiles; + } + watcher.unfilterFilesOwnedBy(owners); PromisePrototypeThen(SafePromiseAllReturnVoid(testFiles, async (file) => { if (!owners.has(file)) { diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js index e4cb1a1ff566d2..7577ce94b25b62 100644 --- a/lib/internal/watch_mode/files_watcher.js +++ b/lib/internal/watch_mode/files_watcher.js @@ -80,7 +80,7 @@ class FilesWatcher extends EventEmitter { watcher.handle.close(); } - #onChange(trigger) { + #onChange(trigger, eventType) { if (this.#mode === 'filter' && !this.#filteredFiles.has(trigger)) { return; } @@ -93,7 +93,7 @@ class FilesWatcher extends EventEmitter { clearTimeout(this.#debounceTimer); this.#debounceTimer = setTimeout(() => { this.#debounceTimer = null; - this.emit('changed', { owners: this.#debounceOwners }); + this.emit('changed', { owners: this.#debounceOwners, eventType }); this.#debounceOwners.clear(); }, this.#debounce).unref(); } @@ -110,7 +110,7 @@ class FilesWatcher extends EventEmitter { watcher.on('change', (eventType, fileName) => { // `fileName` can be `null` if it cannot be determined. See // https://github.com/nodejs/node/pull/49891#issuecomment-1744673430. - this.#onChange(recursive ? resolve(path, fileName ?? '') : path); + this.#onChange(recursive ? resolve(path, fileName ?? '') : path, eventType); }); this.#watchers.set(path, { handle: watcher, recursive }); if (recursive) { diff --git a/test/parallel/test-runner-watch-mode.mjs b/test/parallel/test-runner-watch-mode.mjs index b210dab1e97142..4af48ea409ddbb 100644 --- a/test/parallel/test-runner-watch-mode.mjs +++ b/test/parallel/test-runner-watch-mode.mjs @@ -3,7 +3,7 @@ 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 { writeFileSync, renameSync, unlinkSync, existsSync } from 'node:fs'; import util from 'internal/util'; import tmpdir from '../common/tmpdir.js'; @@ -30,7 +30,7 @@ const fixturePaths = Object.keys(fixtureContent) Object.entries(fixtureContent) .forEach(([file, content]) => writeFileSync(fixturePaths[file], content)); -async function testWatch({ fileToUpdate, file }) { +async function testWatch({ fileToUpdate, file, action = 'update' }) { const ran1 = util.createDeferredPromise(); const ran2 = util.createDeferredPromise(); const child = spawn(process.execPath, @@ -48,22 +48,64 @@ async function testWatch({ fileToUpdate, file }) { if (testRuns?.length >= 2) ran2.resolve(); }); - await ran1.promise; - runs.push(currentRun); - currentRun = ''; - const content = fixtureContent[fileToUpdate]; - const path = fixturePaths[fileToUpdate]; - const interval = setInterval(() => writeFileSync(path, content), common.platformTimeout(1000)); - await ran2.promise; - runs.push(currentRun); - clearInterval(interval); - child.kill(); - for (const run of runs) { - assert.match(run, /# tests 1/); - assert.match(run, /# pass 1/); - assert.match(run, /# fail 0/); - assert.match(run, /# cancelled 0/); - } + const testUpdate = async () => { + await ran1.promise; + const content = fixtureContent[fileToUpdate]; + const path = fixturePaths[fileToUpdate]; + const interval = setInterval(() => writeFileSync(path, content), common.platformTimeout(1000)); + await ran2.promise; + runs.push(currentRun); + clearInterval(interval); + child.kill(); + for (const run of runs) { + assert.match(run, /# tests 1/); + assert.match(run, /# pass 1/); + assert.match(run, /# fail 0/); + assert.match(run, /# cancelled 0/); + } + }; + + const testRename = async () => { + await ran1.promise; + const fileToRenamePath = tmpdir.resolve(fileToUpdate); + const newFileNamePath = tmpdir.resolve(`test-renamed-${fileToUpdate}`); + const interval = setInterval(() => renameSync(fileToRenamePath, newFileNamePath), common.platformTimeout(1000)); + await ran2.promise; + runs.push(currentRun); + clearInterval(interval); + child.kill(); + + for (const run of runs) { + assert.match(run, /# tests 1/); + assert.match(run, /# pass 1/); + assert.match(run, /# fail 0/); + assert.match(run, /# cancelled 0/); + } + }; + + const testDelete = async () => { + await ran1.promise; + const fileToDeletePath = tmpdir.resolve(fileToUpdate); + const interval = setInterval(() => { + if (existsSync(fileToDeletePath)) { + unlinkSync(fileToDeletePath); + } else { + ran2.resolve(); + } + }, common.platformTimeout(1000)); + await ran2.promise; + runs.push(currentRun); + clearInterval(interval); + child.kill(); + + for (const run of runs) { + assert.doesNotMatch(run, /MODULE_NOT_FOUND/); + } + }; + + action === 'update' && await testUpdate(); + action === 'rename' && await testRename(); + action === 'delete' && await testDelete(); } describe('test runner watch mode', () => { @@ -82,4 +124,12 @@ describe('test runner watch mode', () => { it('should support running tests without a file', async () => { await testWatch({ fileToUpdate: 'test.js' }); }); + + it('should support a watched test file rename', async () => { + await testWatch({ fileToUpdate: 'test.js', action: 'rename' }); + }); + + it('should not throw when delete a watched test file', async () => { + await testWatch({ fileToUpdate: 'test.js', action: 'delete' }); + }); });