test_runner: handle file rename and deletion under watch mode

Fixes: #53113 PR-URL: #53114 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
nodejs · Jun 2, 2024 · 7125931 · 7125931
1 parent 41d90aa
commit 7125931
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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' });
+  });
 });