fix suggestions

nodejs · Dec 12, 2023 · ca75b72 · ca75b72
1 parent 9f64e75
commit ca75b72
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 9 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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/);
+  });
 });