gh-127353: Allow to force color output on Windows

[PalmtopTiger]

I think nt._supports_virtual_terminal call should be placed above the isatty check so that the behavior of can_colorize function on Windows matches its behavior on POSIX OSes.

Screenshot of exception with patched can_colorize function in ConEmu terminal:

• Issue: Unable to force color output on Windows #127353

[ZeroIntensity]

Thanks for contributing! It's great to see new people :)

Generally, all bug fixes need tests. I'm not 100% sure if we can test for this though; could you look around in the traceback tests?

[PalmtopTiger]

Tests added. I split tests for non-Windows and Windows OSes into two different methods, hence the many changes in test__colorize.py.

[hugovk]

Thank you, looks good.

I don't have Windows, it'd be useful if someone else could do some manual verification on Windows.

[hugovk]

I've tested this on GitHub Actions. Currently, adding FORCE_COLOR: 1 does not add colour, but it does with this PR. Let's merge, thanks!

[miss-islington-app]

Thanks @PalmtopTiger for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

[bedevere-app]

GH-127886 is a backport of this pull request to the 3.13 branch.

[ZeroIntensity]

It looks like we just blew up the build bots.

[hugovk]

It looks like we just blew up the build bots.

We did! It's this:

0:02:53 load avg: 9.23 [411/481/1] test__colorize failed (1 failure)
test_colorized_detection_checks_for_environment_variables (test.test__colorize.TestColorizeFunction.test_colorized_detection_checks_for_environment_variables) ... FAIL
test_colorized_detection_checks_for_environment_variables_no_vt (test.test__colorize.TestColorizeFunction.test_colorized_detection_checks_for_environment_variables_no_vt) ... skipped 'Windows only'

======================================================================
FAIL: test_colorized_detection_checks_for_environment_variables (test.test__colorize.TestColorizeFunction.test_colorized_detection_checks_for_environment_variables)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/support/__init__.py", line 2848, in wrapper
    return func(*args, **kwargs)
  File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test__colorize.py", line 54, in test_colorized_detection_checks_for_environment_variables
    self.assertEqual(_colorize.can_colorize(), True)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: False != True

----------------------------------------------------------------------
Ran 2 tests in 0.007s

FAILED (failures=1, skipped=1)
test test__colorize failed

I've disabled automerge of the backport (#127886), and we'd better revert this. Then we can open a new PR and make sure the buildbots pass before merging.

[hugovk]

Revert PR: #127889

[hugovk]

This is the line that failed for buildbots.

We're mocking most of the things in _colorize.can_colorize(), but not this?

    if not hasattr(sys.stderr, "fileno"):
        return False

@PalmtopTiger What do you think? Do we need extra mocking here?

[PalmtopTiger]

Apparently so. I'm thinking of two variants.
A simple one:

with (unittest.mock.patch("os.isatty") as isatty_mock,
      unittest.mock.patch("sys.stderr.isatty") as stderr_isatty_mock):
    isatty_mock.return_value = True
    stderr_isatty_mock.return_value = True
    ...
    isatty_mock.return_value = False
    stderr_isatty_mock.return_value = False

Or a more complex one that implements all the necessary stderr methods:

with (unittest.mock.patch("os.isatty") as isatty_mock,
     unittest.mock.patch("sys.stderr") as stderr_mock):
    isatty_mock.return_value = True
    stderr_mock.fileno.return_value = 2
    stderr_mock.isatty.return_value = True
    ...
    isatty_mock.return_value = False
    stderr_mock.isatty.return_value = False

But in this case, the implementation of sys.stderr.isatty becomes unnecessary.
Which option do you think is better?

Also I don't know how to commit the fix now. 😅 To this branch?

[hugovk]

I think the simpler one, we can make it more complex if a need arises. Or should we just skip this test line?

Also I don't know how to commit the fix now. 😅 To this branch?

You can commit to any branch you like, but you will need to open a fresh PR. We can use the same issue number.

By default, we only run GitHub Actions on PRs, but we can manually request some buildbots.

[PalmtopTiger]

Tested both. Simple one doesn't pass hasattr(sys.stderr, "fileno") check. Used complex one.

Or should we just skip this test line?

Then, the case where color output is supported will not be tested.