lib: ensure FORCE_COLOR forces color output in non-TTY environments #55404

pmarchini · 2024-10-16T12:28:24Z

This PR should address #55383.

nodejs-github-bot · 2024-10-16T12:28:29Z

Review requested:

@nodejs/test_runner

avivkeller · 2024-10-16T12:29:26Z

The subsystem here shouldn't be test, it should be lib

pmarchini · 2024-10-16T12:30:29Z

Thanks @redyetidev, I was not sure about this, gonna change it!

avivkeller · 2024-10-16T12:30:45Z

test/parallel/test-runner-output.mjs

+  {
+    name: 'test-runner/output/non-tty-forced-color-output.js',
+    flags: ['--test-reporter=spec'],
+  },


Instead of testing the test runner, IMO add a test where a non-TTY FORCE_COLOR is used.

This change isn't specific to the test runner

Although, I had some weird test failures when doing that (unrelated to the change, more how the test was setup), so if that's not possible, this works.

You’re right, I added the test to cover the fix for the specific issue.
We should add tests to cover every use.
But does it make sense, considering we would be adding tests to parts of the codebase that are "out of scope"? Also, I wouldn't feel confident about the quality of the tests I’d add in this way.

Also, should this change be considered a breaking change since it changes the evident behaviour?

IMO it's not breaking, it's a bug fix

codecov · 2024-10-16T14:58:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (b0ffe9e) to head (dd82e49).
Report is 261 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55404      +/-   ##
==========================================
- Coverage   88.42%   88.41%   -0.01%     
==========================================
  Files         653      653              
  Lines      187498   187496       -2     
  Branches    36100    36103       +3     
==========================================
- Hits       165791   165783       -8     
- Misses      14957    14960       +3     
- Partials     6750     6753       +3

Files with missing lines	Coverage Δ
lib/internal/util/colors.js	`100.00% <100.00%> (ø)`

... and 24 files with indirect coverage changes

avivkeller · 2024-10-16T15:04:39Z

Fixes #55383
Fixes #52249

nodejs-github-bot · 2024-10-16T20:03:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63147/

nodejs-github-bot · 2024-10-16T21:06:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63148/

nodejs-github-bot · 2024-10-17T06:11:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63154/

pmarchini · 2024-10-17T09:43:50Z

Fixes #55383 Fixes #52249

Regarding #52249, I would say we have two ways to ensure the fix:

Add a test in this PR to confirm that the issue is indeed resolved.
After this PR gets merged, open another PR with a test to ensure the fix and close the issue.

Given

node/lib/internal/test_runner/harness.js

Lines 66 to 69 in e2242b4

    
           counters: null, 
        
           shouldColorizeTestFiles: shouldColorizeTestFiles(globalOptions.destinations), 
        
           teardown: null, 
        
           snapshotManager: null,

I believe this PR should resolve both issues, as you suggest 😁.

Personally, I would go with option 1.

WDYT @redyetidev @cjihrig

avivkeller · 2024-10-17T10:58:23Z

I'd go with 1

mcollina

lgtm

cjihrig · 2024-10-17T13:00:35Z

Option 1 sounds good.

pmarchini · 2024-10-17T19:21:47Z

test/parallel/test-runner-output.mjs

+  },
+  {
+    name: 'test-runner/output/assertion-color-tty.mjs',
+    flags: ['--test', '--stack-trace-limit=0'],


Hey @cjihrig, I had to add the --stack-trace-limit=0 flag because modifying

node/test/common/assertSnapshot.js

Lines 7 to 9 in 4d6d7d6

const stackFramesRegexp = /(?<=\n)(\s+)((.+?)\s+$)?(?:\(?(.+?):(\d+)(?::(\d+))?)$?(\s+\{)?(\[\d+m)?(\n|$)/g;

const windowNewlineRegexp = /\r/g;

to match colored stack traces also seems a little painful.

Do you think it's acceptable?

Yes, that's fine IMO.

pmarchini · 2024-10-17T21:43:25Z

While working on the tests I've seen this behaviour:

This comes from this recently merged PR #54862.

import assert from 'node:assert/strict'
import { test } from 'node:test'

test('failing assertion', () => {
  assert.strictEqual('apple', 'pear')
})

results in:

Honestly I've mixed feelings about this from a devEx pov.

@nodejs/test_runner @nodejs/assert

jasnell · 2024-10-20T22:28:03Z

lib/internal/util/colors.js

+    const hasColors = module.exports.shouldColorize(process.stderr);
+    module.exports.blue = hasColors ? '\u001b[34m' : '';
+    module.exports.green = hasColors ? '\u001b[32m' : '';
+    module.exports.white = hasColors ? '\u001b[39m' : '';
+    module.exports.yellow = hasColors ? '\u001b[33m' : '';
+    module.exports.red = hasColors ? '\u001b[31m' : '';
+    module.exports.gray = hasColors ? '\u001b[90m' : '';
+    module.exports.clear = hasColors ? '\u001bc' : '';
+    module.exports.reset = hasColors ? '\u001b[0m' : '';
+    module.exports.hasColors = hasColors;


Non-blocking and admittedly more verbose (and I know not introduced in this PR) but I'd generally prefer something like the following instead.

Suggested change

const hasColors = module.exports.shouldColorize(process.stderr);

module.exports.blue = hasColors ? '\u001b[34m' : '';

module.exports.green = hasColors ? '\u001b[32m' : '';

module.exports.white = hasColors ? '\u001b[39m' : '';

module.exports.yellow = hasColors ? '\u001b[33m' : '';

module.exports.red = hasColors ? '\u001b[31m' : '';

module.exports.gray = hasColors ? '\u001b[90m' : '';

module.exports.clear = hasColors ? '\u001bc' : '';

module.exports.reset = hasColors ? '\u001b[0m' : '';

module.exports.hasColors = hasColors;

if (module.exports.shouldColorize(process.stderr)) {

module.exports.blue = '\u001b[34m';

module.exports.green = '\u001b[32m';

module.exports.white = '\u001b[39m';

module.exports.yellow = '\u001b[33m';

module.exports.red = '\u001b[31m';

module.exports.gray = '\u001b[90m';

module.exports.clear = '\u001bc';

module.exports.reset = '\u001b[0m';

module.exports.hasColors = true;

} else {

module.exports.blue = '';

module.exports.green = ';

module.exports.white = ';

module.exports.yellow = ';

module.exports.red = '';

module.exports.gray = '';

module.exports.clear = '';

module.exports.reset = '';

module.exports.hasColors = false;

}

Hey @jasnell, thanks for the review.
I don't have strong opinions on this specific case, but in general, I think the more readable, the better.

We would have some repetitions, but I think it would be worth it.

Up to you 🚀 If you want, I can use this PR to address this readability issue as well 😁

The CI is green now, maybe consider doing a follow up PR (I agree with suggested change though).

I agree, also because I have the impression that osx CI has been a little unstable in the last two days

nodejs-github-bot · 2024-10-21T08:37:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63228/

nodejs-github-bot · 2024-10-21T10:35:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63229/

nodejs-github-bot · 2024-10-21T16:00:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63235/

nodejs-github-bot · 2024-10-21T17:47:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63239/

nodejs-github-bot · 2024-10-21T22:58:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63243/

nodejs-github-bot · 2024-10-23T06:51:52Z

Landed in 025d8ad

PR-URL: #55404 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#55404 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #55404 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com>

ruyadorno · 2024-11-27T16:18:29Z

This commit does land cleanly on v22.x-staging but make tests fail and will need manual backport in case we want it in v22.x.

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Oct 16, 2024

avivkeller reviewed Oct 16, 2024

View reviewed changes

pmarchini force-pushed the issue/55383 branch from 57e19b4 to 05ae05f Compare October 16, 2024 12:31

pmarchini changed the title ~~test: ensure FORCE_COLOR forces color output in non-TTY environments~~ lib: ensure FORCE_COLOR forces color output in non-TTY environments Oct 16, 2024

avivkeller added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 16, 2024

lpinca approved these changes Oct 16, 2024

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2024

avivkeller added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2024

pmarchini requested review from cjihrig and jakecastelli October 17, 2024 09:35

jakecastelli approved these changes Oct 17, 2024

View reviewed changes

mcollina approved these changes Oct 17, 2024

View reviewed changes

pmarchini commented Oct 17, 2024

View reviewed changes

atlowChemi approved these changes Oct 17, 2024

View reviewed changes

pmarchini force-pushed the issue/55383 branch from 60cb508 to 28a532e Compare October 17, 2024 21:37

pmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2024

pmarchini added 2 commits October 20, 2024 16:54

test: update fixture

b74485e

test: add canColorize

dd82e49

pmarchini force-pushed the issue/55383 branch from 28a532e to dd82e49 Compare October 20, 2024 15:48

MoLow approved these changes Oct 20, 2024

View reviewed changes

avivkeller removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Oct 20, 2024

jasnell reviewed Oct 20, 2024

View reviewed changes

jasnell approved these changes Oct 20, 2024

View reviewed changes

pmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2024

pmarchini mentioned this pull request Oct 21, 2024

assert: fix the string length check for printing the simple diff #55474

Merged

pmarchini added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 22, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 23, 2024

nodejs-github-bot merged commit 025d8ad into nodejs:main Oct 23, 2024
63 checks passed

aduh95 mentioned this pull request Oct 24, 2024

V23.1.0 proposal #55513

Merged

cjihrig mentioned this pull request Nov 21, 2024

Unable to FORCE_COLOR with test runner #55383

Closed

pmarchini mentioned this pull request Nov 25, 2024

lib: refactor code to improve readability #55995

Merged

ruyadorno added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: ensure FORCE_COLOR forces color output in non-TTY environments #55404

lib: ensure FORCE_COLOR forces color output in non-TTY environments #55404

pmarchini commented Oct 16, 2024

nodejs-github-bot commented Oct 16, 2024

avivkeller commented Oct 16, 2024

pmarchini commented Oct 16, 2024

avivkeller Oct 16, 2024 •

edited

Loading

pmarchini Oct 16, 2024

pmarchini Oct 16, 2024

avivkeller Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

avivkeller commented Oct 16, 2024

nodejs-github-bot commented Oct 16, 2024

nodejs-github-bot commented Oct 16, 2024

nodejs-github-bot commented Oct 17, 2024

pmarchini commented Oct 17, 2024

avivkeller commented Oct 17, 2024

mcollina left a comment

cjihrig commented Oct 17, 2024

pmarchini Oct 17, 2024

cjihrig Oct 17, 2024

pmarchini commented Oct 17, 2024 •

edited

Loading

jasnell Oct 20, 2024 •

edited

Loading

pmarchini Oct 21, 2024

jakecastelli Oct 22, 2024 •

edited

Loading

pmarchini Oct 22, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 23, 2024

ruyadorno commented Nov 27, 2024 •

edited by aduh95

Loading


	const stackFramesRegexp = /(?<=\n)(\s+)((.+?)\s+\()?(?:\(?(.+?):(\d+)(?::(\d+))?)\)?(\s+\{)?(\[\d+m)?(\n\|$)/g;
	const windowNewlineRegexp = /\r/g;

-    const hasColors = module.exports.shouldColorize(process.stderr);
-    module.exports.blue = hasColors ? '\u001b[34m' : '';
-    module.exports.green = hasColors ? '\u001b[32m' : '';
-    module.exports.white = hasColors ? '\u001b[39m' : '';
-    module.exports.yellow = hasColors ? '\u001b[33m' : '';
-    module.exports.red = hasColors ? '\u001b[31m' : '';
-    module.exports.gray = hasColors ? '\u001b[90m' : '';
-    module.exports.clear = hasColors ? '\u001bc' : '';
-    module.exports.reset = hasColors ? '\u001b[0m' : '';
-    module.exports.hasColors = hasColors;
+    if (module.exports.shouldColorize(process.stderr)) {
+      module.exports.blue = '\u001b[34m';
+      module.exports.green = '\u001b[32m';
+      module.exports.white = '\u001b[39m';
+      module.exports.yellow = '\u001b[33m';
+      module.exports.red = '\u001b[31m';
+      module.exports.gray = '\u001b[90m';
+      module.exports.clear = '\u001bc';
+      module.exports.reset = '\u001b[0m';
+      module.exports.hasColors = true;
+    } else {
+      module.exports.blue = '';
+      module.exports.green = ';
+      module.exports.white = ';
+      module.exports.yellow = ';
+      module.exports.red = '';
+      module.exports.gray = '';
+      module.exports.clear = '';
+      module.exports.reset = '';
+      module.exports.hasColors = false;
+    }

lib: ensure FORCE_COLOR forces color output in non-TTY environments #55404

lib: ensure FORCE_COLOR forces color output in non-TTY environments #55404

Conversation

pmarchini commented Oct 16, 2024

nodejs-github-bot commented Oct 16, 2024

avivkeller commented Oct 16, 2024

pmarchini commented Oct 16, 2024

avivkeller Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

pmarchini Oct 16, 2024

Choose a reason for hiding this comment

pmarchini Oct 16, 2024

Choose a reason for hiding this comment

avivkeller Oct 16, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

avivkeller commented Oct 16, 2024

nodejs-github-bot commented Oct 16, 2024

nodejs-github-bot commented Oct 16, 2024

nodejs-github-bot commented Oct 17, 2024

pmarchini commented Oct 17, 2024

avivkeller commented Oct 17, 2024

mcollina left a comment

Choose a reason for hiding this comment

cjihrig commented Oct 17, 2024

pmarchini Oct 17, 2024

Choose a reason for hiding this comment

cjihrig Oct 17, 2024

Choose a reason for hiding this comment

pmarchini commented Oct 17, 2024 • edited Loading

jasnell Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

pmarchini Oct 21, 2024

Choose a reason for hiding this comment

jakecastelli Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

pmarchini Oct 22, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 21, 2024

nodejs-github-bot commented Oct 23, 2024

ruyadorno commented Nov 27, 2024 • edited by aduh95 Loading

avivkeller Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

pmarchini commented Oct 17, 2024 •

edited

Loading

jasnell Oct 20, 2024 •

edited

Loading

jakecastelli Oct 22, 2024 •

edited

Loading

ruyadorno commented Nov 27, 2024 •

edited by aduh95

Loading