gh-129061: Fix FORCE_COLOR and NO_COLOR when empty strings

[hugovk]

First refactor the tests into subtests, which makes it easier compare the env var inputs and expected outputs.

• Issue: Empty values of FORCE_COLOR and NO_COLOR are not ignored #129061

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @hugovk for commit ec36e22 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F30617%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[hugovk]

Like #129137 (comment), test_external_inspection failed on Fedora and RHEL8 buildbots and is unrelated.

[serhiy-storchaka]

We only need to test pairs of environment variables that set conflicting values.
For example, NO_COLOR=1 and TERM=dumb should be tested with FORCE_COLOR=1 and PYTHON_COLOR=1. Tests for a single environment variable which forces color (FORCE_COLOR and PYTHON_COLOR) should be performed when the fallback value determined by isatty or _supports_virtual_terminal is False.

We need also to test can_colorize() when stderr is a TTY, but stdout is not, and with an explicit file argument. Including corner cases: no file.fileno, no file.isatty, file.isatty() and os.isatty(file.fileno()) return different results. BTW, I think that file.isatty() should perhaps be tested before os.isatty(file.fileno()).

This is a large issue and can be done in a separate PR.

[serhiy-storchaka]

Test also an empty value. Or other values (e.g. "2").

[hugovk]

Added "true" and "2".

[serhiy-storchaka]

Test also an empty value.

[serhiy-storchaka]

Since the fallback value (determined by TERM, isatty or _supports_virtual_terminal) is True, setting FORCE_COLOR to "1" does not have any effect. We need to test it when the fallback value is False.

You can test {"TERM": "dumb", "FORCE_COLOR": "1"}.

[hugovk]

{"FORCE_COLOR": "1"} returns early before checking TERM, isatty and nt._supports_virtual_terminal, so I think there is value testing {"FORCE_COLOR": "1"}.

I'll add {"TERM": "dumb", "FORCE_COLOR": "1"} as well.

[serhiy-storchaka]

Both variables set the same condition. This test does not add anything.

[hugovk]

These variables are contradictory, one says to force colour, the other to disable:

• "FORCE_COLOR": "0": the variable is present and not an empty string (regardless of its value) --> force colour
• "NO_COLOR": "1": the variable is present and not an empty string (regardless of its value) --> disable colour
• But NO_COLOR takes priority over FORCE_COLOR --> disable colour wins

We're testing that NO_COLOR takes priority and that the value doesn't matter.

True, in the implementation, we return early when checking NO_COLOR before looking at NO_COLOR, but the tests don't care about implementation details, only the end result.

And these tests are fast to run.

[serhiy-storchaka]

The FORCE_COLOR values does not matter, and this was already tested by {"FORCE_COLOR": "1", "NO_COLOR": "1"}. So this test is unneded.

[hugovk]

• "FORCE_COLOR": "" is empty so this has no effect.
• "NO_COLOR": "1" means disable colour.
• So disabling colour wins.

Yes, this does have the same result as {"FORCE_COLOR": "1", "NO_COLOR": "1"}, and because we check NO_COLOR first and return early, FORCE_COLOR has no effect for both.

But I think it's fine to ensure these inputs give this expected output.

However, if you feel strongly about it, I'm happy to remove them.

[hugovk]

Some failures on Windows for:

                ({"PYTHON_COLORS": "true"}, True),
                ({"PYTHON_COLORS": "2"}, True),
                ({"PYTHON_COLORS": ""}, True),

That's because according to the docs:

If this variable is set to 1, the interpreter will colorize various kinds of output. Setting it to 0 deactivates this behavior.

Therefore everything that is not "0" or "1" is ignored, and the result depends on other conditions.

---

This also fails on Windows:

                ({"NO_COLOR": ""}, True),

That's because an empty value means we ignore it, and again the result depends on other conditions.

---

Shall we remove these four, or use them in combination with some other variables?

[serhiy-storchaka]

See improved tests in #129234. They cover all corner cases. I propose to push it first, and then add tests for empty values.

[serhiy-storchaka]

Only the first two checks are necessary. Others do not add anything useful. They look out of place and make the intention of the tests less clear.

The first group tests the different values of the variables. The second group tests relative priority of the variables (it does not repeat tests for different values). Then the same is done for PYTHON_COLORS -- first the different values of PYTHON_COLORS (including the empty value), then its relative priority to other variables.