util: fix test to prevent styleText color from changing

[islandryu]

Fixes: #57921

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[islandryu]

#57921 (comment)
As stated here, I too don't think we should override the existing FORCE_COLOR by test.

[cjihrig]

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.

[islandryu]

I addressed it by setting NOE_TEXT_CONTEXT to child-v8-test-colorize. What do you think?

[cjihrig]

Instead of adding a new value for NODE_TEST_CONTEXT, would it work to update shouldColorize() such that:

• process.env.FORCE_COLOR takes the highest priority if it is set.
• The TTY logic is evaluated after that.
• process.env.NODE_TEST_CONTEXT is only considered as a last resort.

[islandryu]

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.

[cjihrig]

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.

[cjihrig]

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?

[cjihrig]

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]

Codecov Report

Attention: Patch coverage is 87.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.27%. Comparing base (52d95f5) to head (0348c08).
Report is 571 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/runner.js	16.66%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57935      +/-   ##
==========================================
+ Coverage   90.26%   90.27%   +0.01%     
==========================================
  Files         630      630              
  Lines      185933   186193     +260     
  Branches    36450    36477      +27     
==========================================
+ Hits       167829   168089     +260     
- Misses      10972    10981       +9     
+ Partials     7132     7123       -9     

Files with missing lines	Coverage Δ
lib/internal/test_runner/utils.js	60.89% <100.00%> (+0.42%)	⬆️
lib/internal/util/colors.js	100.00% <100.00%> (ø)
lib/util.js	97.78% <100.00%> (+0.02%)	⬆️
lib/internal/test_runner/runner.js	88.46% <16.66%> (-0.41%)	⬇️

... and 55 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cjihrig]

Is this new ignoreTestContext argument necessary? It seems like it could be computed within the function based on the value of stream.

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.

[islandryu]

I've made a change to store the options in NODE_TEST_CONTEXT as a JSON-formatted string.