test_runner: fix support watch with run(), add globPatterns option

mcollina · RafaelGSS · commit 893c864542ac · 2024-08-05T11:15:43.000-03:00
Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #53866 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
diff --git a/doc/api/test.md b/doc/api/test.md
@@ -1239,6 +1239,9 @@ added:
   - v18.9.0
   - v16.19.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/53866
+    description: Added the `globPatterns` option.
   - version: v22.0.0
     pr-url: https://github.com/nodejs/node/pull/52038
     description: Added the `forceExit` option.
@@ -1263,6 +1266,9 @@ changes:
   * `forceExit`: {boolean} Configures the test runner to exit the process once
     all known tests have finished executing even if the event loop would
     otherwise remain active. **Default:** `false`.
+  * `globPatterns`: {Array} An array containing the list of glob patterns to
+    match test files. This option cannot be used together with `files`.
+    **Default:** matching files from [test runner execution model][].
   * `inspectPort` {number|Function} Sets inspector port of test child process.
     This can be a number, or a function that takes no arguments and returns a
     number. If a nullish value is provided, each process gets its own port,
diff --git a/lib/internal/main/test_runner.js b/lib/internal/main/test_runner.js
@@ -1,5 +1,9 @@
 'use strict';
 
+const {
+  ArrayPrototypeSlice,
+} = primordials;
+
 const {
   prepareMainThreadExecution,
   markBootstrapComplete,
@@ -42,6 +46,7 @@ const options = {
   setup: setupTestReporters,
   timeout: perFileTimeout,
   shard,
+  globPatterns: ArrayPrototypeSlice(process.argv, 1),
 };
 debug('test runner configuration:', options);
 run(options).on('test:fail', (data) => {
diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
@@ -1,4 +1,5 @@
 'use strict';
+
 const {
   ArrayIsArray,
   ArrayPrototypeEvery,
@@ -10,7 +11,6 @@ const {
   ArrayPrototypeMap,
   ArrayPrototypePush,
   ArrayPrototypeShift,
-  ArrayPrototypeSlice,
   ArrayPrototypeSome,
   ArrayPrototypeSort,
   ObjectAssign,
@@ -87,10 +87,12 @@ const kCanceledTests = new SafeSet()
 
 let kResistStopPropagation;
 
-function createTestFileList() {
+function createTestFileList(patterns) {
   const cwd = process.cwd();
-  const hasUserSuppliedPattern = process.argv.length > 1;
-  const patterns = hasUserSuppliedPattern ? ArrayPrototypeSlice(process.argv, 1) : [kDefaultPattern];
+  const hasUserSuppliedPattern = patterns != null;
+  if (!patterns || patterns.length === 0) {
+    patterns = [kDefaultPattern];
+  }
   const glob = new Glob(patterns, {
     __proto__: null,
     cwd,
@@ -344,7 +346,6 @@ function runTestFile(path, filesWatcher, opts) {
 
     let err;
 
-
     child.on('error', (error) => {
       err = error;
     });
@@ -418,8 +419,8 @@ function watchFiles(testFiles, opts) {
   opts.root.harness.watching = true;
 
   watcher.on('changed', ({ owners, eventType }) => {
-    if (eventType === 'rename') {
-      const updatedTestFiles = createTestFileList();
+    if (!opts.hasFiles && eventType === 'rename') {
+      const updatedTestFiles = createTestFileList(opts.globPatterns);
 
       const newFileName = ArrayPrototypeFind(updatedTestFiles, (x) => !ArrayPrototypeIncludes(testFiles, x));
       const previousFileName = ArrayPrototypeFind(testFiles, (x) => !ArrayPrototypeIncludes(updatedTestFiles, x));
@@ -482,6 +483,7 @@ function run(options = kEmptyObject) {
     watch,
     setup,
     only,
+    globPatterns,
   } = options;
 
   if (files != null) {
@@ -502,6 +504,16 @@ function run(options = kEmptyObject) {
   if (only != null) {
     validateBoolean(only, 'options.only');
   }
+  if (globPatterns != null) {
+    validateArray(globPatterns, 'options.globPatterns');
+  }
+
+  if (globPatterns?.length > 0 && files?.length > 0) {
+    throw new ERR_INVALID_ARG_VALUE(
+      'options.globPatterns', globPatterns, 'is not supported when specifying \'options.files\'',
+    );
+  }
+
   if (shard != null) {
     validateObject(shard, 'options.shard');
     // Avoid re-evaluating the shard object in case it's a getter
@@ -557,7 +569,7 @@ function run(options = kEmptyObject) {
     root.postRun();
     return root.reporter;
   }
-  let testFiles = files ?? createTestFileList();
+  let testFiles = files ?? createTestFileList(globPatterns);
 
   if (shard) {
     testFiles = ArrayPrototypeFilter(testFiles, (_, index) => index % shard.total === shard.index - 1);
@@ -573,6 +585,8 @@ function run(options = kEmptyObject) {
     inspectPort,
     testNamePatterns,
     testSkipPatterns,
+    hasFiles: files != null,
+    globPatterns,
     only,
     forceExit,
   };
diff --git a/test/fixtures/test-runner-watch.mjs b/test/fixtures/test-runner-watch.mjs
@@ -0,0 +1,24 @@
+import { run } from 'node:test';
+import { tap } from 'node:test/reporters';
+import { parseArgs } from 'node:util';
+
+const options = {
+  file: {
+    type: 'string',
+  },
+};
+const {
+  values,
+  positionals,
+} = parseArgs({ args: process.argv.slice(2), options });
+
+let files;
+
+if (values.file) {
+  files = [values.file];
+}
+
+run({
+  files,
+  watch: true
+}).compose(tap).pipe(process.stdout);
diff --git a/test/parallel/test-runner-run-files-undefined.mjs b/test/parallel/test-runner-run-files-undefined.mjs
@@ -0,0 +1,61 @@
+import * as common from '../common/index.mjs';
+import tmpdir from '../common/tmpdir.js';
+import { describe, it, run, beforeEach } from 'node:test';
+import { dot, spec, tap } from 'node:test/reporters';
+import { fork } from 'node:child_process';
+import assert from 'node:assert';
+
+if (common.hasCrypto) {
+  console.log('1..0 # Skipped: no crypto');
+  process.exit(0);
+}
+
+if (process.env.CHILD === 'true') {
+  describe('require(\'node:test\').run with no files', { concurrency: true }, () => {
+    beforeEach(() => {
+      tmpdir.refresh();
+      process.chdir(tmpdir.path);
+    });
+
+    it('should neither pass or fail', async () => {
+      const stream = run({
+        files: undefined
+      }).compose(tap);
+      stream.on('test:fail', common.mustNotCall());
+      stream.on('test:pass', common.mustNotCall());
+
+      // eslint-disable-next-line no-unused-vars
+      for await (const _ of stream);
+    });
+
+    it('can use the spec reporter', async () => {
+      const stream = run({
+        files: undefined
+      }).compose(spec);
+      stream.on('test:fail', common.mustNotCall());
+      stream.on('test:pass', common.mustNotCall());
+
+      // eslint-disable-next-line no-unused-vars
+      for await (const _ of stream);
+    });
+
+    it('can use the dot reporter', async () => {
+      const stream = run({
+        files: undefined
+      }).compose(dot);
+      stream.on('test:fail', common.mustNotCall());
+      stream.on('test:pass', common.mustNotCall());
+
+      // eslint-disable-next-line no-unused-vars
+      for await (const _ of stream);
+    });
+  });
+} else if (common.isAIX) {
+  console.log('1..0 # Skipped: test runner without specifying files fails on AIX');
+} else {
+  fork(import.meta.filename, [], {
+    env: { CHILD: 'true' }
+  }).on('exit', common.mustCall((code) => {
+    assert.strictEqual(code, 0);
+  }));
+}
diff --git a/test/parallel/test-runner-run-watch.mjs b/test/parallel/test-runner-run-watch.mjs
@@ -0,0 +1,175 @@
+// Flags: --expose-internals
+import * as common from '../common/index.mjs';
+import { describe, it, beforeEach } from 'node:test';
+import assert from 'node:assert';
+import { spawn } from 'node:child_process';
+import { once } from 'node:events';
+import { writeFileSync, renameSync, unlinkSync, existsSync } from 'node:fs';
+import util from 'internal/util';
+import tmpdir from '../common/tmpdir.js';
+import { join } from 'node:path';
+
+if (common.isIBMi)
+  common.skip('IBMi does not support `fs.watch()`');
+
+// 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.js': `
+const test = require('node:test');
+require('./dependency.js');
+import('./dependency.mjs');
+import('data:text/javascript,');
+test('test has ran');`,
+};
+
+let fixturePaths;
+
+function refresh() {
+  tmpdir.refresh();
+
+  fixturePaths = Object.keys(fixtureContent)
+    .reduce((acc, file) => ({ ...acc, [file]: tmpdir.resolve(file) }), {});
+  Object.entries(fixtureContent)
+    .forEach(([file, content]) => writeFileSync(fixturePaths[file], content));
+}
+
+const runner = join(import.meta.dirname, '..', 'fixtures', 'test-runner-watch.mjs');
+
+async function testWatch({ fileToUpdate, file, action = 'update', cwd = tmpdir.path }) {
+  const ran1 = util.createDeferredPromise();
+  const ran2 = util.createDeferredPromise();
+  const args = [runner];
+  if (file) args.push('--file', file);
+  const child = spawn(process.execPath,
+                      args,
+                      { encoding: 'utf8', stdio: 'pipe', cwd });
+  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 >= 1) ran1.resolve();
+    if (testRuns?.length >= 2) ran2.resolve();
+  });
+
+  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();
+    await once(child, 'exit');
+    for (const run of runs) {
+      assert.doesNotMatch(run, /run\(\) is being called recursively/);
+      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();
+    await once(child, 'exit');
+
+    for (const run of runs) {
+      assert.doesNotMatch(run, /run\(\) is being called recursively/);
+      if (action === 'rename2') {
+        assert.match(run, /MODULE_NOT_FOUND/);
+      } else {
+        assert.doesNotMatch(run, /MODULE_NOT_FOUND/);
+      }
+      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();
+    await once(child, 'exit');
+
+    for (const run of runs) {
+      assert.doesNotMatch(run, /MODULE_NOT_FOUND/);
+    }
+  };
+
+  action === 'update' && await testUpdate();
+  action === 'rename' && await testRename();
+  action === 'rename2' && await testRename();
+  action === 'delete' && await testDelete();
+}
+
+describe('test runner watch mode', () => {
+  beforeEach(refresh);
+  it('should run tests repeatedly', async () => {
+    await testWatch({ file: 'test.js', fileToUpdate: 'test.js' });
+  });
+
+  it('should run tests with dependency repeatedly', async () => {
+    await testWatch({ file: 'test.js', fileToUpdate: 'dependency.js' });
+  });
+
+  it('should run tests with ESM dependency', async () => {
+    await testWatch({ file: 'test.js', fileToUpdate: 'dependency.mjs' });
+  });
+
+  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 deleting a watched test file', { skip: common.isAIX }, async () => {
+    await testWatch({ fileToUpdate: 'test.js', action: 'delete' });
+  });
+
+  it('should run tests with dependency repeatedly in a different cwd', async () => {
+    await testWatch({
+      file: join(tmpdir.path, 'test.js'),
+      fileToUpdate: 'dependency.js',
+      cwd: import.meta.dirname,
+      action: 'rename2'
+    });
+  });
+
+  it('should handle renames in a different cwd', async () => {
+    await testWatch({
+      file: join(tmpdir.path, 'test.js'),
+      fileToUpdate: 'test.js',
+      cwd: import.meta.dirname,
+      action: 'rename2'
+    });
+  });
+});
diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs
diff --git a/test/parallel/test-runner-watch-mode.mjs b/test/parallel/test-runner-watch-mode.mjs
