Skip to content

Commit

Permalink
test_runner: handle file rename and deletion under watch mode
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
jakecastelli authored and marco-ippolito committed Jul 19, 2024
1 parent 7a126e8 commit 9d3699b
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 23 deletions.
19 changes: 17 additions & 2 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const {
ArrayFrom,
ArrayIsArray,
ArrayPrototypeFilter,
ArrayPrototypeFind,
ArrayPrototypeForEach,
ArrayPrototypeIncludes,
ArrayPrototypeMap,
Expand Down Expand Up @@ -41,7 +42,6 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_TEST_FAILURE,
ERR_OUT_OF_RANGE,
},
} = require('internal/errors');
const {
Expand Down Expand Up @@ -458,7 +458,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)) {
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/watch_mode/files_watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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();
}
Expand All @@ -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) {
Expand Down
86 changes: 68 additions & 18 deletions test/parallel/test-runner-watch-mode.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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,
Expand All @@ -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', () => {
Expand All @@ -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' });
});
});

0 comments on commit 9d3699b

Please sign in to comment.