From e4eb26bd7eb252308136acfe318769629e2186ca Mon Sep 17 00:00:00 2001 From: pulkit-30 Date: Wed, 6 Dec 2023 22:30:47 +0530 Subject: [PATCH 1/5] test_runner: fixed test object is incorrectly passed to setup() --- lib/internal/test_runner/harness.js | 4 +++- lib/internal/test_runner/runner.js | 4 +++- lib/internal/test_runner/utils.js | 19 ++++++++++++++----- test/parallel/test-runner-run.mjs | 17 +++++++++++++++++ 4 files changed, 37 insertions(+), 7 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 2f18b0bcf091ac..982cd788ddf085 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -23,6 +23,7 @@ const { parseCommandLine, reporterScope, setupTestReporters, + colorizeTestFiles, } = require('internal/test_runner/utils'); const { bigint: hrtime } = process.hrtime; @@ -205,7 +206,8 @@ function getGlobalRoot() { process.exitCode = kGenericUserError; } }); - reportersSetup = setupTestReporters(globalRoot); + reportersSetup = setupTestReporters(globalRoot.reporter); + colorizeTestFiles(globalRoot); } return globalRoot; } diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 34100213ebd935..a745820868cca3 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -70,6 +70,7 @@ const { convertStringToRegExp, countCompletedTest, kDefaultPattern, + colorizeTestFiles, } = require('internal/test_runner/utils'); const { Glob } = require('internal/fs/glob'); const { once } = require('events'); @@ -491,6 +492,7 @@ function run(options = kEmptyObject) { return root.reporter; } let testFiles = files ?? createTestFileList(); + colorizeTestFiles(root); if (shard) { testFiles = ArrayPrototypeFilter(testFiles, (_, index) => index % shard.total === shard.index - 1); @@ -512,7 +514,7 @@ function run(options = kEmptyObject) { }); }; - PromisePrototypeThen(PromisePrototypeThen(PromiseResolve(setup?.(root)), runFiles), postRun); + PromisePrototypeThen(PromisePrototypeThen(PromiseResolve(setup?.(root.reporter)), runFiles), postRun); return root.reporter; } diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index 6b4663f14302c3..b5e444e98785c9 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -3,6 +3,7 @@ const { ArrayPrototypeJoin, ArrayPrototypeMap, ArrayPrototypeFlatMap, + ArrayPrototypeForEach, ArrayPrototypePush, ArrayPrototypeReduce, ObjectGetOwnPropertyDescriptor, @@ -128,10 +129,17 @@ function tryBuiltinReporter(name) { return require(builtinPath); } -async function getReportersMap(reporters, destinations, rootTest) { +function colorizeTestFiles(rootTest) { + const { reporters, destinations } = parseCommandLine(); + ArrayPrototypeForEach(reporters, (_, index) => { + const destination = kBuiltinDestinations.get(destinations[index]) ?? createWriteStream(destinations[index]); + rootTest.harness.shouldColorizeTestFiles ||= shouldColorize(destination); + }); +} + +async function getReportersMap(reporters, destinations) { return SafePromiseAllReturnArrayLike(reporters, async (name, i) => { const destination = kBuiltinDestinations.get(destinations[i]) ?? createWriteStream(destinations[i]); - rootTest.harness.shouldColorizeTestFiles ||= shouldColorize(destination); // Load the test reporter passed to --test-reporter let reporter = tryBuiltinReporter(name); @@ -166,12 +174,12 @@ async function getReportersMap(reporters, destinations, rootTest) { } const reporterScope = new AsyncResource('TestReporterScope'); -const setupTestReporters = reporterScope.bind(async (rootTest) => { +const setupTestReporters = reporterScope.bind(async (rootReporter) => { const { reporters, destinations } = parseCommandLine(); - const reportersMap = await getReportersMap(reporters, destinations, rootTest); + const reportersMap = await getReportersMap(reporters, destinations); for (let i = 0; i < reportersMap.length; i++) { const { reporter, destination } = reportersMap[i]; - compose(rootTest.reporter, reporter).pipe(destination); + compose(rootReporter, reporter).pipe(destination); } }); @@ -413,6 +421,7 @@ function getCoverageReport(pad, summary, symbol, color, table) { } module.exports = { + colorizeTestFiles, convertStringToRegExp, countCompletedTest, createDeferredCallback, diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index c712f500153b42..1fa5da4da05b43 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -1,10 +1,14 @@ +// Flags: --expose-internals + import * as common from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; import { join } from 'node:path'; import { describe, it, run } from 'node:test'; import { dot, spec, tap } from 'node:test/reporters'; import assert from 'node:assert'; +import stream from 'internal/test_runner/tests_stream'; +const { TestsStream } = stream; const testFixtures = fixtures.path('test-runner'); describe('require(\'node:test\').run', { concurrency: true }, () => { @@ -465,6 +469,19 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { code: 'ERR_INVALID_ARG_TYPE' })); }); + + it('should pass instance of stream to setup', async () => { + const stream = run({ + files: [join(testFixtures, 'default-behavior/test/random.cjs')], + setup: common.mustCall((root) => { + assert(root instanceof TestsStream); + }), + }); + stream.on('test:fail', common.mustNotCall()); + stream.on('test:pass', common.mustCall()); + // eslint-disable-next-line no-unused-vars + for await (const _ of stream); + }); }); it('should run with no files', async () => { From 175af142dfd74195f6d6b04d8ba3ac5390395b30 Mon Sep 17 00:00:00 2001 From: pulkit-30 Date: Tue, 12 Dec 2023 16:26:56 +0530 Subject: [PATCH 2/5] fix suggestions --- lib/internal/test_runner/utils.js | 2 +- test/parallel/test-runner-run.mjs | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index b5e444e98785c9..f32dfc1275a88a 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -132,7 +132,7 @@ function tryBuiltinReporter(name) { function colorizeTestFiles(rootTest) { const { reporters, destinations } = parseCommandLine(); ArrayPrototypeForEach(reporters, (_, index) => { - const destination = kBuiltinDestinations.get(destinations[index]) ?? createWriteStream(destinations[index]); + const destination = kBuiltinDestinations.get(destinations[index]); rootTest.harness.shouldColorizeTestFiles ||= shouldColorize(destination); }); } diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index 1fa5da4da05b43..52ff5f5d1bd6f4 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -1,14 +1,10 @@ -// Flags: --expose-internals - import * as common from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; import { join } from 'node:path'; import { describe, it, run } from 'node:test'; import { dot, spec, tap } from 'node:test/reporters'; import assert from 'node:assert'; -import stream from 'internal/test_runner/tests_stream'; -const { TestsStream } = stream; const testFixtures = fixtures.path('test-runner'); describe('require(\'node:test\').run', { concurrency: true }, () => { @@ -474,7 +470,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { const stream = run({ files: [join(testFixtures, 'default-behavior/test/random.cjs')], setup: common.mustCall((root) => { - assert(root instanceof TestsStream); + assert.strictEqual(root.constructor.name, "TestsStream"); }), }); stream.on('test:fail', common.mustNotCall()); From 775ea08011263e07f6bf8436445bd89cb89dab06 Mon Sep 17 00:00:00 2001 From: pulkit-30 Date: Tue, 12 Dec 2023 16:30:36 +0530 Subject: [PATCH 3/5] fix lint --- lib/internal/test_runner/harness.js | 2 +- lib/internal/test_runner/runner.js | 2 +- test/parallel/test-runner-run.mjs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 982cd788ddf085..6faf8ff5099b33 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -20,10 +20,10 @@ const { exitCodes: { kGenericUserError } } = internalBinding('errors'); const { kEmptyObject } = require('internal/util'); const { kCancelledByParent, Test, Suite } = require('internal/test_runner/test'); const { + colorizeTestFiles, parseCommandLine, reporterScope, setupTestReporters, - colorizeTestFiles, } = require('internal/test_runner/utils'); const { bigint: hrtime } = process.hrtime; diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index a745820868cca3..321b66eff42144 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -67,10 +67,10 @@ const { } = require('internal/test_runner/test'); const { + colorizeTestFiles, convertStringToRegExp, countCompletedTest, kDefaultPattern, - colorizeTestFiles, } = require('internal/test_runner/utils'); const { Glob } = require('internal/fs/glob'); const { once } = require('events'); diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index 52ff5f5d1bd6f4..8f95de4db16296 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -470,7 +470,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { const stream = run({ files: [join(testFixtures, 'default-behavior/test/random.cjs')], setup: common.mustCall((root) => { - assert.strictEqual(root.constructor.name, "TestsStream"); + assert.strictEqual(root.constructor.name, 'TestsStream'); }), }); stream.on('test:fail', common.mustNotCall()); From 9f64e75d13961c1b892b0fcc900da1553b7e6913 Mon Sep 17 00:00:00 2001 From: pulkit-30 Date: Tue, 12 Dec 2023 16:32:35 +0530 Subject: [PATCH 4/5] call colorizeTestFil fn before abort --- lib/internal/test_runner/runner.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 321b66eff42144..8663bc42893baa 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -488,11 +488,12 @@ function run(options = kEmptyObject) { } const root = createTestTree({ __proto__: null, concurrency, timeout, signal }); + colorizeTestFiles(root); + if (process.env.NODE_TEST_CONTEXT !== undefined) { return root.reporter; } let testFiles = files ?? createTestFileList(); - colorizeTestFiles(root); if (shard) { testFiles = ArrayPrototypeFilter(testFiles, (_, index) => index % shard.total === shard.index - 1); From ca75b72efc68f973c1aba7d2d5ac2b89a6045f81 Mon Sep 17 00:00:00 2001 From: pulkit-30 Date: Tue, 12 Dec 2023 23:20:27 +0530 Subject: [PATCH 5/5] fix suggestions --- lib/internal/test_runner/harness.js | 4 ++-- lib/internal/test_runner/runner.js | 4 ++-- lib/internal/test_runner/utils.js | 11 ++++++----- test/parallel/test-runner-reporters.js | 19 +++++++++++++++++++ 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 6faf8ff5099b33..469ca903c7048c 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -20,10 +20,10 @@ const { exitCodes: { kGenericUserError } } = internalBinding('errors'); const { kEmptyObject } = require('internal/util'); const { kCancelledByParent, Test, Suite } = require('internal/test_runner/test'); const { - colorizeTestFiles, parseCommandLine, reporterScope, setupTestReporters, + shouldColorizeTestFiles, } = require('internal/test_runner/utils'); const { bigint: hrtime } = process.hrtime; @@ -207,7 +207,7 @@ function getGlobalRoot() { } }); reportersSetup = setupTestReporters(globalRoot.reporter); - colorizeTestFiles(globalRoot); + globalRoot.harness.shouldColorizeTestFiles ||= shouldColorizeTestFiles(globalRoot); } return globalRoot; } diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 8663bc42893baa..242e807fa68883 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -67,10 +67,10 @@ const { } = require('internal/test_runner/test'); const { - colorizeTestFiles, convertStringToRegExp, countCompletedTest, kDefaultPattern, + shouldColorizeTestFiles, } = require('internal/test_runner/utils'); const { Glob } = require('internal/fs/glob'); const { once } = require('events'); @@ -488,7 +488,7 @@ function run(options = kEmptyObject) { } const root = createTestTree({ __proto__: null, concurrency, timeout, signal }); - colorizeTestFiles(root); + root.harness.shouldColorizeTestFiles ||= shouldColorizeTestFiles(root); if (process.env.NODE_TEST_CONTEXT !== undefined) { return root.reporter; diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index f32dfc1275a88a..184d44dce6c162 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -3,9 +3,9 @@ const { ArrayPrototypeJoin, ArrayPrototypeMap, ArrayPrototypeFlatMap, - ArrayPrototypeForEach, ArrayPrototypePush, ArrayPrototypeReduce, + ArrayPrototypeSome, ObjectGetOwnPropertyDescriptor, MathFloor, MathMax, @@ -129,11 +129,12 @@ function tryBuiltinReporter(name) { return require(builtinPath); } -function colorizeTestFiles(rootTest) { +function shouldColorizeTestFiles(rootTest) { + // This function assumes only built-in destinations (stdout/stderr) supports coloring const { reporters, destinations } = parseCommandLine(); - ArrayPrototypeForEach(reporters, (_, index) => { + return ArrayPrototypeSome(reporters, (_, index) => { const destination = kBuiltinDestinations.get(destinations[index]); - rootTest.harness.shouldColorizeTestFiles ||= shouldColorize(destination); + return destination && shouldColorize(destination); }); } @@ -421,7 +422,6 @@ function getCoverageReport(pad, summary, symbol, color, table) { } module.exports = { - colorizeTestFiles, convertStringToRegExp, countCompletedTest, createDeferredCallback, @@ -430,5 +430,6 @@ module.exports = { parseCommandLine, reporterScope, setupTestReporters, + shouldColorizeTestFiles, getCoverageReport, }; diff --git a/test/parallel/test-runner-reporters.js b/test/parallel/test-runner-reporters.js index bb831491366dfc..e40861eb87831f 100644 --- a/test/parallel/test-runner-reporters.js +++ b/test/parallel/test-runner-reporters.js @@ -155,4 +155,23 @@ describe('node:test reporters', { concurrency: true }, () => { assert.strictEqual(child.stdout.toString(), 'Going to throw an error\n'); assert.match(child.stderr.toString(), /Emitted 'error' event on Duplex instance/); }); + + it('should support stdout as a destination with spec reporter', async () => { + process.env.FORCE_COLOR = '1'; + const file = tmpdir.resolve(`${tmpFiles++}.txt`); + const child = spawnSync(process.execPath, + ['--test', '--test-reporter', 'spec', '--test-reporter-destination', file, testFile]); + assert.strictEqual(child.stderr.toString(), ''); + assert.strictEqual(child.stdout.toString(), ''); + const fileConent = fs.readFileSync(file, 'utf8'); + assert.match(fileConent, /▶ nested/); + assert.match(fileConent, /✔ ok/); + assert.match(fileConent, /✖ failing/); + assert.match(fileConent, /ℹ tests 4/); + assert.match(fileConent, /ℹ pass 2/); + assert.match(fileConent, /ℹ fail 2/); + assert.match(fileConent, /ℹ cancelled 0/); + assert.match(fileConent, /ℹ skipped 0/); + assert.match(fileConent, /ℹ todo 0/); + }); });