Skip to content

Commit

Permalink
test_runner: fix delete test file cause dependency file not watched
Browse files Browse the repository at this point in the history
When a watched test file is being deleted then the referenced dependency
file(s) will be updated incorrect when `unfilterFilesOwnedBy` method is
called, which will cause tests not being rerun when its referenced
dependency changed. To prevent this case, we can simply `return` when we
detect a watched test file being deleted.

PR-URL: #53533
Refs: #53114
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
jakecastelli authored Aug 14, 2024
1 parent c1ec099 commit 1212eca
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,13 +424,18 @@ function watchFiles(testFiles, opts) {
const newFileName = ArrayPrototypeFind(updatedTestFiles, (x) => !ArrayPrototypeIncludes(testFiles, x));
const previousFileName = ArrayPrototypeFind(testFiles, (x) => !ArrayPrototypeIncludes(updatedTestFiles, x));

testFiles = updatedTestFiles;

// When file renamed
if (newFileName && previousFileName) {
owners = new SafeSet().add(newFileName);
watcher.filterFile(resolve(newFileName), owners);
}

testFiles = updatedTestFiles;
if (!newFileName && previousFileName) {
return; // Avoid rerunning files when file deleted
}

}

watcher.unfilterFilesOwnedBy(owners);
Expand Down
104 changes: 104 additions & 0 deletions test/parallel/test-runner-watch-mode-complex.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// 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, unlinkSync } 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();

// This test updates these files repeatedly,
// Reading them from disk is unreliable due to race conditions.
const fixtureContent = {
'dependency.js': 'module.exports = {};',
'dependency.mjs': 'export const a = 1;',
// Test 1
'test.js': `
const test = require('node:test');
require('./dependency.js');
import('./dependency.mjs');
import('data:text/javascript,');
test('first test has ran');`,
// Test 2
'test-2.mjs': `
import test from 'node:test';
import './dependency.js';
import { a } from './dependency.mjs';
import 'data:text/javascript,';
test('second test has ran');`,
// Test file to be deleted
'test-to-delete.mjs': `
import test from 'node:test';
import './dependency.js';
import { a } from './dependency.mjs';
import 'data:text/javascript,';
test('test to delete 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 run tests when a dependency changed after a watched test file being deleted', async () => {
const ran1 = util.createDeferredPromise();
const ran2 = util.createDeferredPromise();
const child = spawn(process.execPath,
['--watch', '--test'],
{ encoding: 'utf8', stdio: 'pipe', cwd: tmpdir.path });
let stdout = '';
let currentRun = '';
const runs = [];

child.stdout.on('data', (data) => {
stdout += data.toString();
currentRun += data.toString();
const testRuns = stdout.match(/# duration_ms\s\d+/g);

if (testRuns?.length >= 2) {
ran2.resolve();
return;
}
if (testRuns?.length >= 1) ran1.resolve();
});

await ran1.promise;
runs.push(currentRun);
currentRun = '';
const fileToDeletePathLocal = tmpdir.resolve('test-to-delete.mjs');
unlinkSync(fileToDeletePathLocal);

const content = fixtureContent['dependency.mjs'];
const path = fixturePaths['dependency.mjs'];
const interval = setInterval(() => writeFileSync(path, content), common.platformTimeout(1000));
await ran2.promise;
runs.push(currentRun);
currentRun = '';
clearInterval(interval);
child.kill();

assert.strictEqual(runs.length, 2);

const [firstRun, secondRun] = runs;

assert.match(firstRun, /# tests 3/);
assert.match(firstRun, /# pass 3/);
assert.match(firstRun, /# fail 0/);
assert.match(firstRun, /# cancelled 0/);

assert.match(secondRun, /# tests 2/);
assert.match(secondRun, /# pass 2/);
assert.match(secondRun, /# fail 0/);
assert.match(secondRun, /# cancelled 0/);
});
});

0 comments on commit 1212eca

Please sign in to comment.