util: fix test to prevent styleText color from changing #57935
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
lib/internal/test_runner/runner.js
Outdated
| @@ -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'; | |||
There was a problem hiding this comment.
#57921 (comment)
As stated here, I too don't think we should override the existing FORCE_COLOR by test.
There was a problem hiding this comment.
We definitely shouldn't be creating new, undocumented environment variables like this. There is already NODE_TEST_CONTEXT, which is documented as something that users should not rely on. We would need to leverage that environment variable.
There was a problem hiding this comment.
I addressed it by setting NOE_TEXT_CONTEXT to child-v8-test-colorize. What do you think?
There was a problem hiding this comment.
Instead of adding a new value for NODE_TEST_CONTEXT, would it work to update shouldColorize() such that:
process.env.FORCE_COLORtakes the highest priority if it is set.- The TTY logic is evaluated after that.
process.env.NODE_TEST_CONTEXTis only considered as a last resort.
There was a problem hiding this comment.
Would it be something like this?
if (process.env.FORCE_COLOR !== undefined) {
return lazyInternalTTY().getColorDepth() > 2;
}
if (stream?.isTTY) {
return typeof stream.getColorDepth === 'function' ?
stream.getColorDepth() > 2 : true;
}
if (process.env.NODE_TEST_CONTEXT === 'child-v8') {
return lazyInternalTTY().getColorDepth() > 2;
}That alone would result in behavior differing from the existing implementation in scenarios such as the following.
console.log({foo: "bar"})node --test index.js | cat > text.txt
{ foo: 'bar' }
# fixed
./out/Debug/node --test index.js | cat > text.txt
{ foo: �[32m'bar'�[39m }Regardless of the order within shouldColorize, I think the test process needs some way to know the value of opts.root.harness.shouldColorizeTestFiles in the parent process.
There was a problem hiding this comment.
If you want to start passing parent config through to the child, the NODE_TEST_CONTEXT environment variable is probably the best place to do it.
There was a problem hiding this comment.
This FORCE_COLOR change came from #48057. Is it possible to achieve the same result without hacking FORCE_COLOR? The test runner child processes already know that they are test runner child processes. Can we leverage that?
lib/internal/test_runner/runner.js
Outdated
| @@ -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'; | |||
There was a problem hiding this comment.
We definitely shouldn't be creating new, undocumented environment variables like this. There is already NODE_TEST_CONTEXT, which is documented as something that users should not rely on. We would need to leverage that environment variable.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57935 +/- ##
========================================
Coverage 90.26% 90.27%
========================================
Files 630 630
Lines 185933 186121 +188
Branches 36450 36475 +25
========================================
+ Hits 167829 168013 +184
- Misses 10972 10981 +9
+ Partials 7132 7127 -5
🚀 New features to boost your workflow:
|
| @@ -15,7 +15,10 @@ module.exports = { | |||
| clear: '', | |||
| reset: '', | |||
| hasColors: false, | |||
| shouldColorize(stream) { | |||
| shouldColorize(stream, ignoreTestContext = false) { | |||
There was a problem hiding this comment.
Is this new ignoreTestContext argument necessary? It seems like it could be computed within the function based on the value of stream.
cjihrig
dismissed
their stale review
Dismissing my block because this isn't introducing a new environment variable anymore, but I don't think the current approach is quite correct either. If anything, I'd pass parent settings as a JSON string.
Fixes: #57921