Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to FORCE_COLOR with test runner #55383

Open
deadbeef84 opened this issue Oct 14, 2024 · 12 comments
Open

Unable to FORCE_COLOR with test runner #55383

deadbeef84 opened this issue Oct 14, 2024 · 12 comments
Labels
test_runner Issues and PRs related to the test runner subsystem.

Comments

@deadbeef84
Copy link
Contributor

Version

v22.9.0

Platform

Linux deadbeef 6.8.0-45-generic #45-Ubuntu SMP PREEMPT_DYNAMIC Fri Aug 30 12:02:04 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

test_runner

What steps will reproduce the bug?

When running the test-runner outside of a tty, it's not possible to force colors.

FORCE_COLOR=1 node --test | cat

How often does it reproduce? Is there a required condition?

No condition.

What is the expected behavior? Why is that the expected behavior?

When FORCE_COLOR is set to 1, 2 or 3, I expect ansi color codes to be written to stdout even if it's not in a tty.

What do you see instead?

It doesn't respect the FORCE_COLOR and no ansi color codes are in the output.

Additional information

No response

@pmarchini pmarchini added the test_runner Issues and PRs related to the test runner subsystem. label Oct 14, 2024
@pmarchini
Copy link
Member

Hey @deadbeef84, thanks for reporting this.
I will take a look and come back to you😁

@RedYetiDev
Copy link
Member

RedYetiDev commented Oct 14, 2024

I can't reproduce:

require('node:test')('...', () => {
    console.log(require('node:util').styleText('red', 'red'));
});

w/

FORCE_COLOR=1 node --test repro.js | cat

It prints 'red' in red

@RedYetiDev RedYetiDev added repro-exists Issues with reproductions. and removed repro-exists Issues with reproductions. labels Oct 14, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Oct 14, 2024

You're testing console.log() and styleText(). I can reproduce the issue.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 14, 2024

Actually, I believe the issue is that on Node 22, a non-tty causes the test runner to use the TAP reporter which doesn't use color. If I add --test-reporter=spec to the command, I do see colored output.

@RedYetiDev
Copy link
Member

RedYetiDev commented Oct 14, 2024

I still can't reproduce the issue, even with TAP 🤔 :
image
image

@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2024

You're testing console.log() and styleText().

That red text doesn't come from the test runner, and the test runner doesn't strip the colors.

@pmarchini
Copy link
Member

pmarchini commented Oct 15, 2024

I haven't had the chance to take a close look at the issue, but I wonder if

refresh() {
if (process.stderr.isTTY) {
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;
}
},
};
module.exports.refresh();
could be the reason. I suppose that if process.stderr.isTTY is false, then even with force enabled, we're still getting "empty" colors.

WDYT?

@RedYetiDev
Copy link
Member

RedYetiDev commented Oct 15, 2024

Yes. I tried to fix it in the past, but failed (due to test failures).

See #54289

@RedYetiDev
Copy link
Member

This issue is a duplicate of #52249

@RedYetiDev RedYetiDev closed this as not planned Won't fix, can't repro, duplicate, stale Oct 15, 2024
@pmarchini
Copy link
Member

Hey @RedYetiDev, the reported issue is different, even though the fix is related.
I don't know if it makes sense to close this issue as not planned since it is, in fact, an issue, at least from my point of view.
I would expect test cases to also cover this specific scenario in a possible fix.

What do you think about this?
Should we just add a comment in the other issue pointing out this expected behavior as well?
I would expect some tests to be related to what was highlighted in this specific issue by @deadbeef84.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2024

I agree with @pmarchini. Let's keep this open for now.

@cjihrig cjihrig reopened this Oct 15, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2024

could be the reason. I suppose that if process.stderr.isTTY is false, then even with force enabled, we're still getting "empty" colors.

I believe you're right @pmarchini. The current code will only ever colorize output when a TTY is used. We probably don't need that if statement since the next line calls shouldColorize(), which does check the FORCE_COLOR env var.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants