From 893c864542ac1f366edc729856a5cc97e34c0e3d Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Wed, 24 Jul 2024 15:11:20 +0200 Subject: [PATCH] test_runner: fix support watch with run(), add globPatterns option Signed-off-by: Matteo Collina PR-URL: https://github.com/nodejs/node/pull/53866 Reviewed-By: Chemi Atlow Reviewed-By: Colin Ihrig Reviewed-By: Moshe Atlow --- doc/api/test.md | 6 + lib/internal/main/test_runner.js | 5 + lib/internal/test_runner/runner.js | 30 ++- test/fixtures/test-runner-watch.mjs | 24 +++ .../test-runner-run-files-undefined.mjs | 61 ++++++ test/parallel/test-runner-run-watch.mjs | 175 ++++++++++++++++++ test/parallel/test-runner-run.mjs | 47 ++--- test/parallel/test-runner-watch-mode.mjs | 25 ++- 8 files changed, 324 insertions(+), 49 deletions(-) create mode 100644 test/fixtures/test-runner-watch.mjs create mode 100644 test/parallel/test-runner-run-files-undefined.mjs create mode 100644 test/parallel/test-runner-run-watch.mjs diff --git a/doc/api/test.md b/doc/api/test.md index c1f57c2b991db8..7c7c591c67c84f 100644 --- 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 index 3f86ddb84bb64e..a42c1e5dff64cc 100644 --- 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 index 263aedd9abccd2..31a253c776a81e 100644 --- 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 new file mode 100644 index 00000000000000..6780b31c541690 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..9d08b10a4550cd --- /dev/null +++ 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 new file mode 100644 index 00000000000000..0445637bb6123c --- /dev/null +++ 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 index 0b3c0908601004..7a575da9c95275 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -135,6 +135,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { }) .compose(tap) .toArray(); + assert.strictEqual(result[2], 'ok 1 - this should be executed\n'); assert.strictEqual(result[4], '1..1\n'); assert.strictEqual(result[5], '# tests 1\n'); @@ -467,6 +468,19 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { })); }); + it('should only allow array in options.globPatterns', async () => { + [Symbol(), {}, () => {}, 0, 1, 0n, 1n, '', '1', Promise.resolve([]), true, false] + .forEach((globPatterns) => assert.throws(() => run({ globPatterns }), { + code: 'ERR_INVALID_ARG_TYPE' + })); + }); + + it('should not allow files and globPatterns used together', () => { + assert.throws(() => run({ files: ['a.js'], globPatterns: ['*.js'] }), { + code: 'ERR_INVALID_ARG_VALUE' + }); + }); + it('should only allow object as options', () => { [Symbol(), [], () => {}, 0, 1, 0n, 1n, '', '1', true, false] .forEach((options) => assert.throws(() => run(options), { @@ -488,39 +502,6 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { }); }); - it('should run with no files', 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('should run with no files and use 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('should run with no files and use 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); - }); - it('should avoid running recursively', async () => { const stream = run({ files: [join(testFixtures, 'recursive_run.js')] }); let stderr = ''; diff --git a/test/parallel/test-runner-watch-mode.mjs b/test/parallel/test-runner-watch-mode.mjs index 4af48ea409ddbb..a55f404a4f7010 100644 --- a/test/parallel/test-runner-watch-mode.mjs +++ b/test/parallel/test-runner-watch-mode.mjs @@ -1,17 +1,17 @@ // Flags: --expose-internals import * as common from '../common/index.mjs'; -import { describe, it } from 'node:test'; +import { describe, it, beforeEach } from 'node:test'; +import { once } from 'node:events'; import assert from 'node:assert'; import { spawn } from 'node:child_process'; import { writeFileSync, renameSync, unlinkSync, existsSync } 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()`'); -tmpdir.refresh(); +let fixturePaths; // This test updates these files repeatedly, // Reading them from disk is unreliable due to race conditions. @@ -25,10 +25,14 @@ import('./dependency.mjs'); import('data:text/javascript,'); test('test has ran');`, }; -const fixturePaths = Object.keys(fixtureContent) - .reduce((acc, file) => ({ ...acc, [file]: tmpdir.resolve(file) }), {}); -Object.entries(fixtureContent) - .forEach(([file, content]) => writeFileSync(fixturePaths[file], content)); + +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)); +} async function testWatch({ fileToUpdate, file, action = 'update' }) { const ran1 = util.createDeferredPromise(); @@ -57,6 +61,8 @@ async function testWatch({ fileToUpdate, file, action = 'update' }) { runs.push(currentRun); clearInterval(interval); child.kill(); + await once(child, 'exit'); + for (const run of runs) { assert.match(run, /# tests 1/); assert.match(run, /# pass 1/); @@ -74,6 +80,7 @@ async function testWatch({ fileToUpdate, file, action = 'update' }) { runs.push(currentRun); clearInterval(interval); child.kill(); + await once(child, 'exit'); for (const run of runs) { assert.match(run, /# tests 1/); @@ -97,6 +104,7 @@ async function testWatch({ fileToUpdate, file, action = 'update' }) { runs.push(currentRun); clearInterval(interval); child.kill(); + await once(child, 'exit'); for (const run of runs) { assert.doesNotMatch(run, /MODULE_NOT_FOUND/); @@ -109,6 +117,7 @@ async function testWatch({ fileToUpdate, file, action = 'update' }) { } describe('test runner watch mode', () => { + beforeEach(refresh); it('should run tests repeatedly', async () => { await testWatch({ file: 'test.js', fileToUpdate: 'test.js' }); }); @@ -129,7 +138,7 @@ describe('test runner watch mode', () => { await testWatch({ fileToUpdate: 'test.js', action: 'rename' }); }); - it('should not throw when delete a watched test file', async () => { + it('should not throw when delete a watched test file', { skip: common.isAIX }, async () => { await testWatch({ fileToUpdate: 'test.js', action: 'delete' }); }); });