From b17d4e013c78cb0ebc335e000412e4ff40a3813b Mon Sep 17 00:00:00 2001 From: islandryu Date: Sun, 20 Apr 2025 00:35:21 +0900 Subject: [PATCH 1/3] util: fix test to prevent styleText color from changing Fixes: https://github.com/nodejs/node/issues/57921 --- lib/internal/test_runner/runner.js | 2 +- lib/internal/util/colors.js | 5 ++++- lib/util.js | 7 ++++++- test/fixtures/test-runner/force-color.js | 5 +++++ .../output/style-text-output-in-test-tty.mjs | 14 +++++++++++++ .../style-text-output-in-test-tty.snapshot | 20 +++++++++++++++++++ .../output/style-text-output-in-test.mjs | 14 +++++++++++++ .../output/style-text-output-in-test.snapshot | 20 +++++++++++++++++++ test/parallel/test-runner-force-color.mjs | 15 ++++++++++++++ test/parallel/test-runner-output.mjs | 9 +++++++++ 10 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/test-runner/force-color.js create mode 100644 test/fixtures/test-runner/output/style-text-output-in-test-tty.mjs create mode 100644 test/fixtures/test-runner/output/style-text-output-in-test-tty.snapshot create mode 100644 test/fixtures/test-runner/output/style-text-output-in-test.mjs create mode 100644 test/fixtures/test-runner/output/style-text-output-in-test.snapshot create mode 100644 test/parallel/test-runner-force-color.mjs diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index bbc2515473c41d..2e8a0d73a5ac50 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -373,7 +373,7 @@ function runTestFile(path, filesWatcher, opts) { env.WATCH_REPORT_DEPENDENCIES = '1'; } if (opts.root.harness.shouldColorizeTestFiles) { - env.FORCE_COLOR = '1'; + env.TEST_REPORT_COLORIZE = '1'; } const child = spawn( diff --git a/lib/internal/util/colors.js b/lib/internal/util/colors.js index 0b37694b513fb0..063b0aa18ba343 100644 --- a/lib/internal/util/colors.js +++ b/lib/internal/util/colors.js @@ -15,7 +15,10 @@ module.exports = { clear: '', reset: '', hasColors: false, - shouldColorize(stream) { + shouldColorize(stream, ignoreTestContext = false) { + if (!ignoreTestContext && process.env.TEST_REPORT_COLORIZE) { + return lazyInternalTTY().getColorDepth() > 2; + } if (process.env.FORCE_COLOR !== undefined) { return lazyInternalTTY().getColorDepth() > 2; } diff --git a/lib/util.js b/lib/util.js index 233da10e83c48d..d9448287145d18 100644 --- a/lib/util.js +++ b/lib/util.js @@ -130,7 +130,12 @@ function styleText(format, text, { validateStream = true, stream = process.stdou } // If the stream is falsy or should not be colorized, set skipColorize to true - skipColorize = !lazyUtilColors().shouldColorize(stream); + skipColorize = !lazyUtilColors().shouldColorize( + stream, + // Avoid styleText color changes in tests + // https://github.com/nodejs/node/issues/57921 + stream !== process.stdout && stream !== process.stderr, + ); } // If the format is not an array, convert it to an array diff --git a/test/fixtures/test-runner/force-color.js b/test/fixtures/test-runner/force-color.js new file mode 100644 index 00000000000000..c6f2dc139a8bc9 --- /dev/null +++ b/test/fixtures/test-runner/force-color.js @@ -0,0 +1,5 @@ +const {WriteStream} = require('tty'); +const assert = require('assert'); + +assert.strictEqual(process.env.FORCE_COLOR, '3'); +assert.strictEqual(WriteStream(0).getColorDepth(), 24); diff --git a/test/fixtures/test-runner/output/style-text-output-in-test-tty.mjs b/test/fixtures/test-runner/output/style-text-output-in-test-tty.mjs new file mode 100644 index 00000000000000..8c0a4d68ab5da0 --- /dev/null +++ b/test/fixtures/test-runner/output/style-text-output-in-test-tty.mjs @@ -0,0 +1,14 @@ +import { Writable } from 'node:stream'; +import { styleText } from 'node:util'; + +const streamTTY = new class extends Writable { + isTTY = true; +}(); +const streamNoTTY = new class extends Writable { + isTTY = false; +}(); + +console.log(styleText('bgYellow', 'TTY', { stream: streamTTY })); +console.log(styleText('bgYellow', 'No TTY', { stream: streamNoTTY })); +console.log(styleText('bgYellow', 'stdout', { stream: process.stdout })); +console.log(styleText('bgYellow', 'stderr', { stream: process.stderr })); diff --git a/test/fixtures/test-runner/output/style-text-output-in-test-tty.snapshot b/test/fixtures/test-runner/output/style-text-output-in-test-tty.snapshot new file mode 100644 index 00000000000000..d2cd621bd678dc --- /dev/null +++ b/test/fixtures/test-runner/output/style-text-output-in-test-tty.snapshot @@ -0,0 +1,20 @@ +TAP version 13 +# [43mTTY[49m +# No TTY +# [43mstdout[49m +# [43mstderr[49m +# Subtest: /test/fixtures/test-runner/output/style-text-output-in-test-tty.mjs +ok 1 - /test/fixtures/test-runner/output/style-text-output-in-test-tty.mjs + --- + duration_ms: * + type: 'test' + ... +1..1 +# tests 1 +# suites 0 +# pass 1 +# fail 0 +# cancelled 0 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/fixtures/test-runner/output/style-text-output-in-test.mjs b/test/fixtures/test-runner/output/style-text-output-in-test.mjs new file mode 100644 index 00000000000000..8c0a4d68ab5da0 --- /dev/null +++ b/test/fixtures/test-runner/output/style-text-output-in-test.mjs @@ -0,0 +1,14 @@ +import { Writable } from 'node:stream'; +import { styleText } from 'node:util'; + +const streamTTY = new class extends Writable { + isTTY = true; +}(); +const streamNoTTY = new class extends Writable { + isTTY = false; +}(); + +console.log(styleText('bgYellow', 'TTY', { stream: streamTTY })); +console.log(styleText('bgYellow', 'No TTY', { stream: streamNoTTY })); +console.log(styleText('bgYellow', 'stdout', { stream: process.stdout })); +console.log(styleText('bgYellow', 'stderr', { stream: process.stderr })); diff --git a/test/fixtures/test-runner/output/style-text-output-in-test.snapshot b/test/fixtures/test-runner/output/style-text-output-in-test.snapshot new file mode 100644 index 00000000000000..90c1c66ecac45b --- /dev/null +++ b/test/fixtures/test-runner/output/style-text-output-in-test.snapshot @@ -0,0 +1,20 @@ +TAP version 13 +# TTY +# No TTY +# stdout +# stderr +# Subtest: /test/fixtures/test-runner/output/style-text-output-in-test.mjs +ok 1 - /test/fixtures/test-runner/output/style-text-output-in-test.mjs + --- + duration_ms: * + type: 'test' + ... +1..1 +# tests 1 +# suites 0 +# pass 1 +# fail 0 +# cancelled 0 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/parallel/test-runner-force-color.mjs b/test/parallel/test-runner-force-color.mjs new file mode 100644 index 00000000000000..6e791e076c2f8e --- /dev/null +++ b/test/parallel/test-runner-force-color.mjs @@ -0,0 +1,15 @@ +import * as common from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.js'; +import assert from 'node:assert'; + +common.spawnPromisified(process.execPath, [ + '--test', + fixtures.path('test-runner', 'force-color.js'), +], { + env: { + ...process.env, + FORCE_COLOR: 3, + } +}).then(common.mustCall((result) => { + assert.strictEqual(result.code, 0); +})); diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index 8eb622be35c313..e1c720930d0d8f 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -313,6 +313,15 @@ const tests = [ flags: ['--test-reporter=tap', '--test-coverage-exclude=../output/**'], cwd: fixtures.path('test-runner/coverage-snap'), } : false, + canColorize ? { + name: 'test-runner/output/style-text-output-in-test.mjs', + flags: ['--test', '--test-reporter=tap'], + } : false, + canColorize ? { + name: 'test-runner/output/style-text-output-in-test-tty.mjs', + flags: ['--test', '--test-reporter=tap'], + tty: true, + } : false, ] .filter(Boolean) .map(({ flags, name, tty, transform, cwd }) => ({ From 90924d3e371a7dd8c04aac22f64ec143787f2716 Mon Sep 17 00:00:00 2001 From: islandryu Date: Sun, 20 Apr 2025 10:03:52 +0900 Subject: [PATCH 2/3] use NOE_TEXT_CONTEXT to determine if you should colorize --- lib/internal/test_runner/runner.js | 2 +- lib/internal/test_runner/utils.js | 3 ++- lib/internal/util/colors.js | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 2e8a0d73a5ac50..18a974d7957b58 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -373,7 +373,7 @@ function runTestFile(path, filesWatcher, opts) { env.WATCH_REPORT_DEPENDENCIES = '1'; } if (opts.root.harness.shouldColorizeTestFiles) { - env.TEST_REPORT_COLORIZE = '1'; + env.NODE_TEST_CONTEXT = 'child-v8-test-colorize'; } const child = spawn( diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index 4e1a21df976b17..c0defac7665f0d 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -203,7 +203,8 @@ function parseCommandLine() { const watch = getOptionValue('--watch'); const timeout = getOptionValue('--test-timeout') || Infinity; const isChildProcess = process.env.NODE_TEST_CONTEXT === 'child'; - const isChildProcessV8 = process.env.NODE_TEST_CONTEXT === 'child-v8'; + const isChildProcessV8 = process.env.NODE_TEST_CONTEXT === 'child-v8' || + process.env.NODE_TEST_CONTEXT === 'child-v8-test-colorize'; let globalSetupPath; let concurrency; let coverageExcludeGlobs; diff --git a/lib/internal/util/colors.js b/lib/internal/util/colors.js index 063b0aa18ba343..d7e8b46f9f24c2 100644 --- a/lib/internal/util/colors.js +++ b/lib/internal/util/colors.js @@ -16,7 +16,7 @@ module.exports = { reset: '', hasColors: false, shouldColorize(stream, ignoreTestContext = false) { - if (!ignoreTestContext && process.env.TEST_REPORT_COLORIZE) { + if (!ignoreTestContext && process.env.NODE_TEST_CONTEXT === 'child-v8-test-colorize') { return lazyInternalTTY().getColorDepth() > 2; } if (process.env.FORCE_COLOR !== undefined) { From 0348c08172d707804500cea803c61f2ede20ca6f Mon Sep 17 00:00:00 2001 From: islandryu Date: Fri, 25 Apr 2025 18:54:22 +0900 Subject: [PATCH 3/3] make test context JSON object --- lib/internal/test_runner/runner.js | 7 ++++++- lib/internal/test_runner/utils.js | 12 +++++++++--- lib/internal/util/colors.js | 19 +++++++++++++++++-- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 18a974d7957b58..4a5a9db123f341 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -15,6 +15,7 @@ const { ArrayPrototypeSlice, ArrayPrototypeSome, ArrayPrototypeSort, + JSONStringify, ObjectAssign, PromisePrototypeThen, PromiseWithResolvers, @@ -373,7 +374,11 @@ function runTestFile(path, filesWatcher, opts) { env.WATCH_REPORT_DEPENDENCIES = '1'; } if (opts.root.harness.shouldColorizeTestFiles) { - env.NODE_TEST_CONTEXT = 'child-v8-test-colorize'; + env.NODE_TEST_CONTEXT = JSONStringify({ + __proto__: null, + contextName: 'child-v8', + shouldColorize: true, + }); } const child = spawn( diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index c0defac7665f0d..9e2a44a2e57815 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -8,6 +8,7 @@ const { ArrayPrototypePush, ArrayPrototypeReduce, ArrayPrototypeSome, + JSONParse, MathFloor, MathMax, MathMin, @@ -202,9 +203,14 @@ function parseCommandLine() { const updateSnapshots = getOptionValue('--test-update-snapshots'); const watch = getOptionValue('--watch'); const timeout = getOptionValue('--test-timeout') || Infinity; - const isChildProcess = process.env.NODE_TEST_CONTEXT === 'child'; - const isChildProcessV8 = process.env.NODE_TEST_CONTEXT === 'child-v8' || - process.env.NODE_TEST_CONTEXT === 'child-v8-test-colorize'; + let contextName; + try { + contextName = JSONParse(process.env.NODE_TEST_CONTEXT).contextName; + } catch { + contextName = process.env.NODE_TEST_CONTEXT; + } + const isChildProcess = contextName === 'child'; + const isChildProcessV8 = contextName === 'child-v8'; let globalSetupPath; let concurrency; let coverageExcludeGlobs; diff --git a/lib/internal/util/colors.js b/lib/internal/util/colors.js index d7e8b46f9f24c2..a5ed0a15cf291d 100644 --- a/lib/internal/util/colors.js +++ b/lib/internal/util/colors.js @@ -1,5 +1,9 @@ 'use strict'; +const { + JSONParse, +} = primordials; + let internalTTy; function lazyInternalTTY() { internalTTy ??= require('internal/tty'); @@ -16,8 +20,19 @@ module.exports = { reset: '', hasColors: false, shouldColorize(stream, ignoreTestContext = false) { - if (!ignoreTestContext && process.env.NODE_TEST_CONTEXT === 'child-v8-test-colorize') { - return lazyInternalTTY().getColorDepth() > 2; + + + if (!ignoreTestContext) { + let shouldTestColorize; + try { + shouldTestColorize = JSONParse(process.env.NODE_TEST_CONTEXT).shouldColorize; + } catch { + shouldTestColorize = false; + } + + if (shouldTestColorize) { + return lazyInternalTTY().getColorDepth() > 2; + } } if (process.env.FORCE_COLOR !== undefined) { return lazyInternalTTY().getColorDepth() > 2;