From 96c2e76bb3ee6ce11fbd957224b84ba1e31b12b3 Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Date: Tue, 21 Jan 2025 16:37:45 +0200 Subject: [PATCH 1/6] Refactor into subtests --- Lib/test/test__colorize.py | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/Lib/test/test__colorize.py b/Lib/test/test__colorize.py index 25519ba7e92e1f..3deb86e784c6d0 100644 --- a/Lib/test/test__colorize.py +++ b/Lib/test/test__colorize.py @@ -42,25 +42,20 @@ def test_colorized_detection_checks_for_environment_variables(self): stdout_mock.isatty.return_value = True stderr_mock.fileno.return_value = 2 stderr_mock.isatty.return_value = True - with unittest.mock.patch("os.environ", {'TERM': 'dumb'}): - self.assertEqual(_colorize.can_colorize(), False) - with unittest.mock.patch("os.environ", {'PYTHON_COLORS': '1'}): - self.assertEqual(_colorize.can_colorize(), True) - with unittest.mock.patch("os.environ", {'PYTHON_COLORS': '0'}): - self.assertEqual(_colorize.can_colorize(), False) - with unittest.mock.patch("os.environ", {'NO_COLOR': '1'}): - self.assertEqual(_colorize.can_colorize(), False) - with unittest.mock.patch("os.environ", - {'NO_COLOR': '1', "PYTHON_COLORS": '1'}): - self.assertEqual(_colorize.can_colorize(), True) - with unittest.mock.patch("os.environ", {'FORCE_COLOR': '1'}): - self.assertEqual(_colorize.can_colorize(), True) - with unittest.mock.patch("os.environ", - {'FORCE_COLOR': '1', 'NO_COLOR': '1'}): - self.assertEqual(_colorize.can_colorize(), False) - with unittest.mock.patch("os.environ", - {'FORCE_COLOR': '1', "PYTHON_COLORS": '0'}): - self.assertEqual(_colorize.can_colorize(), False) + + for env_vars, expected in [ + ({"TERM": "dumb"}, False), + ({"PYTHON_COLORS": "1"}, True), + ({"PYTHON_COLORS": "0"}, False), + ({"NO_COLOR": "1"}, False), + ({"NO_COLOR": "1", "PYTHON_COLORS": "1"}, True), + ({"FORCE_COLOR": "1"}, True), + ({"FORCE_COLOR": "1", "NO_COLOR": "1"}, False), + ({"FORCE_COLOR": "1", "PYTHON_COLORS": "0"}, False), + ]: + with self.subTest(env_vars=env_vars, expected=expected): + with unittest.mock.patch("os.environ", env_vars): + self.assertEqual(_colorize.can_colorize(), expected) with unittest.mock.patch("os.environ", {}): if sys.platform == "win32": From c7b541962e0ac6f3f84be3886a357cfa442b65e3 Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Date: Tue, 21 Jan 2025 18:54:33 +0200 Subject: [PATCH 2/6] Fix FORCE_COLOR and NO_COLOR when empty strings --- Lib/_colorize.py | 4 ++-- Lib/test/test__colorize.py | 17 +++++++++++++++++ ...25-01-21-18-52-32.gh-issue-129061.4idD_B.rst | 1 + 3 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2025-01-21-18-52-32.gh-issue-129061.4idD_B.rst diff --git a/Lib/_colorize.py b/Lib/_colorize.py index 5e36e45734a5fb..41e818f2a747ff 100644 --- a/Lib/_colorize.py +++ b/Lib/_colorize.py @@ -42,11 +42,11 @@ def can_colorize(*, file=None) -> bool: return False if os.environ.get("PYTHON_COLORS") == "1": return True - if "NO_COLOR" in os.environ: + if os.environ.get("NO_COLOR"): return False if not COLORIZE: return False - if "FORCE_COLOR" in os.environ: + if os.environ.get("FORCE_COLOR"): return True if os.environ.get("TERM") == "dumb": return False diff --git a/Lib/test/test__colorize.py b/Lib/test/test__colorize.py index 3deb86e784c6d0..26a0c27fca21b1 100644 --- a/Lib/test/test__colorize.py +++ b/Lib/test/test__colorize.py @@ -48,17 +48,34 @@ def test_colorized_detection_checks_for_environment_variables(self): ({"PYTHON_COLORS": "1"}, True), ({"PYTHON_COLORS": "0"}, False), ({"NO_COLOR": "1"}, False), + ({"NO_COLOR": "0"}, False), ({"NO_COLOR": "1", "PYTHON_COLORS": "1"}, True), ({"FORCE_COLOR": "1"}, True), ({"FORCE_COLOR": "1", "NO_COLOR": "1"}, False), + ({"FORCE_COLOR": "1", "NO_COLOR": "0"}, False), + ({"FORCE_COLOR": "1", "NO_COLOR": ""}, True), + ({"FORCE_COLOR": "0", "NO_COLOR": "1"}, False), + ({"FORCE_COLOR": "", "NO_COLOR": "1"}, False), ({"FORCE_COLOR": "1", "PYTHON_COLORS": "0"}, False), + ({"FORCE_COLOR": "0", "PYTHON_COLORS": "0"}, False), ]: with self.subTest(env_vars=env_vars, expected=expected): with unittest.mock.patch("os.environ", env_vars): self.assertEqual(_colorize.can_colorize(), expected) + with unittest.mock.patch("os.environ", {"NO_COLOR": ""}): + if sys.platform == "win32": + vt_mock.return_value = False + self.assertEqual(_colorize.can_colorize(), False) + + vt_mock.return_value = True + self.assertEqual(_colorize.can_colorize(), True) + else: + self.assertEqual(_colorize.can_colorize(), True) + with unittest.mock.patch("os.environ", {}): if sys.platform == "win32": + vt_mock.return_value = False self.assertEqual(_colorize.can_colorize(), False) vt_mock.return_value = True diff --git a/Misc/NEWS.d/next/Library/2025-01-21-18-52-32.gh-issue-129061.4idD_B.rst b/Misc/NEWS.d/next/Library/2025-01-21-18-52-32.gh-issue-129061.4idD_B.rst new file mode 100644 index 00000000000000..5c5c05e1161e86 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-01-21-18-52-32.gh-issue-129061.4idD_B.rst @@ -0,0 +1 @@ +Fix FORCE_COLOR and NO_COLOR when empty strings. Patch by Hugo van Kemenade. From a7380986ff6ce1c2372469272efe8cc534801496 Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Date: Thu, 23 Jan 2025 16:17:24 +0200 Subject: [PATCH 3/6] Add test cases --- Lib/test/test__colorize.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Lib/test/test__colorize.py b/Lib/test/test__colorize.py index 26a0c27fca21b1..f1f6c7723bf96b 100644 --- a/Lib/test/test__colorize.py +++ b/Lib/test/test__colorize.py @@ -45,10 +45,15 @@ def test_colorized_detection_checks_for_environment_variables(self): for env_vars, expected in [ ({"TERM": "dumb"}, False), + ({"TERM": "dumb", "FORCE_COLOR": "1"}, True), + ({"PYTHON_COLORS": "true"}, True), + ({"PYTHON_COLORS": "2"}, True), ({"PYTHON_COLORS": "1"}, True), ({"PYTHON_COLORS": "0"}, False), + ({"PYTHON_COLORS": ""}, True), ({"NO_COLOR": "1"}, False), ({"NO_COLOR": "0"}, False), + ({"NO_COLOR": ""}, True), ({"NO_COLOR": "1", "PYTHON_COLORS": "1"}, True), ({"FORCE_COLOR": "1"}, True), ({"FORCE_COLOR": "1", "NO_COLOR": "1"}, False), From 621e4abbd6bb0d5420ee4036c57a1ebfcba5af13 Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Date: Sat, 25 Jan 2025 17:37:44 +0200 Subject: [PATCH 4/6] Reset tests to main --- Lib/test/test__colorize.py | 181 ++++++++++++++++++++++--------------- 1 file changed, 108 insertions(+), 73 deletions(-) diff --git a/Lib/test/test__colorize.py b/Lib/test/test__colorize.py index f1f6c7723bf96b..ff15d83cb078fe 100644 --- a/Lib/test/test__colorize.py +++ b/Lib/test/test__colorize.py @@ -1,97 +1,132 @@ import contextlib +import io import sys import unittest import unittest.mock import _colorize -from test.support import force_not_colorized, make_clean_env +from test.support.os_helper import EnvironmentVarGuard -ORIGINAL_CAN_COLORIZE = _colorize.can_colorize +@contextlib.contextmanager +def clear_env(): + with EnvironmentVarGuard() as mock_env: + for var in "FORCE_COLOR", "NO_COLOR", "PYTHON_COLORS": + mock_env.unset(var) + yield mock_env -def setUpModule(): - _colorize.can_colorize = lambda *args, **kwargs: False - -def tearDownModule(): - _colorize.can_colorize = ORIGINAL_CAN_COLORIZE +def supports_virtual_terminal(): + if sys.platform == "win32": + return unittest.mock.patch("nt._supports_virtual_terminal", return_value=True) + else: + return contextlib.nullcontext() class TestColorizeFunction(unittest.TestCase): - def setUp(self): - # Remove PYTHON* environment variables to isolate from local user - # settings and simulate running with `-E`. Such variables should be - # added to test methods later to patched os.environ. - patcher = unittest.mock.patch("os.environ", new=make_clean_env()) - self.addCleanup(patcher.stop) - patcher.start() - - @force_not_colorized def test_colorized_detection_checks_for_environment_variables(self): - flags = unittest.mock.MagicMock(ignore_environment=False) + def check(env, fallback, expected): + with (self.subTest(env=env, fallback=fallback), + clear_env() as mock_env): + mock_env.update(env) + isatty_mock.return_value = fallback + stdout_mock.isatty.return_value = fallback + self.assertEqual(_colorize.can_colorize(), expected) + with (unittest.mock.patch("os.isatty") as isatty_mock, unittest.mock.patch("sys.stdout") as stdout_mock, - unittest.mock.patch("sys.stderr") as stderr_mock, - unittest.mock.patch("sys.flags", flags), - unittest.mock.patch("_colorize.can_colorize", ORIGINAL_CAN_COLORIZE), - (unittest.mock.patch("nt._supports_virtual_terminal", return_value=False) - if sys.platform == "win32" else - contextlib.nullcontext()) as vt_mock): + supports_virtual_terminal()): + stdout_mock.fileno.return_value = 1 + for fallback in False, True: + check({}, fallback, fallback) + check({'TERM': 'dumb'}, fallback, False) + check({'TERM': 'xterm'}, fallback, fallback) + check({'TERM': ''}, fallback, fallback) + check({'FORCE_COLOR': '1'}, fallback, True) + check({'FORCE_COLOR': '0'}, fallback, True) + check({'NO_COLOR': '1'}, fallback, False) + check({'NO_COLOR': '0'}, fallback, False) + + check({'TERM': 'dumb', 'FORCE_COLOR': '1'}, False, True) + check({'FORCE_COLOR': '1', 'NO_COLOR': '1'}, True, False) + + for ignore_environment in False, True: + # Simulate running with or without `-E`. + flags = unittest.mock.MagicMock(ignore_environment=ignore_environment) + with unittest.mock.patch("sys.flags", flags): + check({'PYTHON_COLORS': '1'}, True, True) + check({'PYTHON_COLORS': '1'}, False, not ignore_environment) + check({'PYTHON_COLORS': '0'}, True, ignore_environment) + check({'PYTHON_COLORS': '0'}, False, False) + for fallback in False, True: + check({'PYTHON_COLORS': 'x'}, fallback, fallback) + check({'PYTHON_COLORS': ''}, fallback, fallback) + + check({'TERM': 'dumb', 'PYTHON_COLORS': '1'}, False, not ignore_environment) + check({'NO_COLOR': '1', 'PYTHON_COLORS': '1'}, False, not ignore_environment) + check({'FORCE_COLOR': '1', 'PYTHON_COLORS': '0'}, True, ignore_environment) + + @unittest.skipUnless(sys.platform == "win32", "requires Windows") + def test_colorized_detection_checks_on_windows(self): + with (clear_env(), + unittest.mock.patch("os.isatty") as isatty_mock, + unittest.mock.patch("sys.stdout") as stdout_mock, + supports_virtual_terminal() as vt_mock): + stdout_mock.fileno.return_value = 1 isatty_mock.return_value = True + stdout_mock.isatty.return_value = True + + vt_mock.return_value = True + self.assertEqual(_colorize.can_colorize(), True) + vt_mock.return_value = False + self.assertEqual(_colorize.can_colorize(), False) + import nt + del nt._supports_virtual_terminal + self.assertEqual(_colorize.can_colorize(), False) + + def test_colorized_detection_checks_for_std_streams(self): + with (clear_env(), + unittest.mock.patch("os.isatty") as isatty_mock, + unittest.mock.patch("sys.stdout") as stdout_mock, + unittest.mock.patch("sys.stderr") as stderr_mock, + supports_virtual_terminal()): stdout_mock.fileno.return_value = 1 + stderr_mock.fileno.side_effect = ZeroDivisionError + stderr_mock.isatty.side_effect = ZeroDivisionError + + isatty_mock.return_value = True stdout_mock.isatty.return_value = True - stderr_mock.fileno.return_value = 2 - stderr_mock.isatty.return_value = True - - for env_vars, expected in [ - ({"TERM": "dumb"}, False), - ({"TERM": "dumb", "FORCE_COLOR": "1"}, True), - ({"PYTHON_COLORS": "true"}, True), - ({"PYTHON_COLORS": "2"}, True), - ({"PYTHON_COLORS": "1"}, True), - ({"PYTHON_COLORS": "0"}, False), - ({"PYTHON_COLORS": ""}, True), - ({"NO_COLOR": "1"}, False), - ({"NO_COLOR": "0"}, False), - ({"NO_COLOR": ""}, True), - ({"NO_COLOR": "1", "PYTHON_COLORS": "1"}, True), - ({"FORCE_COLOR": "1"}, True), - ({"FORCE_COLOR": "1", "NO_COLOR": "1"}, False), - ({"FORCE_COLOR": "1", "NO_COLOR": "0"}, False), - ({"FORCE_COLOR": "1", "NO_COLOR": ""}, True), - ({"FORCE_COLOR": "0", "NO_COLOR": "1"}, False), - ({"FORCE_COLOR": "", "NO_COLOR": "1"}, False), - ({"FORCE_COLOR": "1", "PYTHON_COLORS": "0"}, False), - ({"FORCE_COLOR": "0", "PYTHON_COLORS": "0"}, False), - ]: - with self.subTest(env_vars=env_vars, expected=expected): - with unittest.mock.patch("os.environ", env_vars): - self.assertEqual(_colorize.can_colorize(), expected) - - with unittest.mock.patch("os.environ", {"NO_COLOR": ""}): - if sys.platform == "win32": - vt_mock.return_value = False - self.assertEqual(_colorize.can_colorize(), False) - - vt_mock.return_value = True - self.assertEqual(_colorize.can_colorize(), True) - else: - self.assertEqual(_colorize.can_colorize(), True) - - with unittest.mock.patch("os.environ", {}): - if sys.platform == "win32": - vt_mock.return_value = False - self.assertEqual(_colorize.can_colorize(), False) - - vt_mock.return_value = True - self.assertEqual(_colorize.can_colorize(), True) - else: - self.assertEqual(_colorize.can_colorize(), True) + self.assertEqual(_colorize.can_colorize(), True) + + isatty_mock.return_value = False + stdout_mock.isatty.return_value = False + self.assertEqual(_colorize.can_colorize(), False) + + def test_colorized_detection_checks_for_file(self): + with clear_env(), supports_virtual_terminal(): + with unittest.mock.patch("os.isatty") as isatty_mock: + file = unittest.mock.MagicMock() + file.fileno.return_value = 1 + isatty_mock.return_value = True + self.assertEqual(_colorize.can_colorize(file=file), True) isatty_mock.return_value = False - stdout_mock.isatty.return_value = False - stderr_mock.isatty.return_value = False - self.assertEqual(_colorize.can_colorize(), False) + self.assertEqual(_colorize.can_colorize(file=file), False) + + # No file.fileno. + with unittest.mock.patch("os.isatty", side_effect=ZeroDivisionError): + file = unittest.mock.MagicMock(spec=['isatty']) + file.isatty.return_value = True + self.assertEqual(_colorize.can_colorize(file=file), False) + + # file.fileno() raises io.UnsupportedOperation. + with unittest.mock.patch("os.isatty", side_effect=ZeroDivisionError): + file = unittest.mock.MagicMock() + file.fileno.side_effect = io.UnsupportedOperation + file.isatty.return_value = True + self.assertEqual(_colorize.can_colorize(file=file), True) + file.isatty.return_value = False + self.assertEqual(_colorize.can_colorize(file=file), False) if __name__ == "__main__": From 5027272232e49b98d1999ea552007f089293811d Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Date: Sat, 25 Jan 2025 18:09:02 +0200 Subject: [PATCH 5/6] Add test cases for empty values --- Lib/test/test__colorize.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Lib/test/test__colorize.py b/Lib/test/test__colorize.py index ff15d83cb078fe..1959773c4967cd 100644 --- a/Lib/test/test__colorize.py +++ b/Lib/test/test__colorize.py @@ -44,11 +44,16 @@ def check(env, fallback, expected): check({'TERM': ''}, fallback, fallback) check({'FORCE_COLOR': '1'}, fallback, True) check({'FORCE_COLOR': '0'}, fallback, True) + check({'FORCE_COLOR': ''}, fallback, fallback) check({'NO_COLOR': '1'}, fallback, False) check({'NO_COLOR': '0'}, fallback, False) + check({'NO_COLOR': ''}, fallback, fallback) check({'TERM': 'dumb', 'FORCE_COLOR': '1'}, False, True) check({'FORCE_COLOR': '1', 'NO_COLOR': '1'}, True, False) + check({'FORCE_COLOR': '1', 'NO_COLOR': ''}, True, True) + check({'FORCE_COLOR': '', 'NO_COLOR': '1'}, True, False) + check({'FORCE_COLOR': '', 'NO_COLOR': ''}, True, True) for ignore_environment in False, True: # Simulate running with or without `-E`. @@ -64,7 +69,9 @@ def check(env, fallback, expected): check({'TERM': 'dumb', 'PYTHON_COLORS': '1'}, False, not ignore_environment) check({'NO_COLOR': '1', 'PYTHON_COLORS': '1'}, False, not ignore_environment) + check({'NO_COLOR': '', 'PYTHON_COLORS': '1'}, False, not ignore_environment) check({'FORCE_COLOR': '1', 'PYTHON_COLORS': '0'}, True, ignore_environment) + check({'FORCE_COLOR': '', 'PYTHON_COLORS': '0'}, True, ignore_environment) @unittest.skipUnless(sys.platform == "win32", "requires Windows") def test_colorized_detection_checks_on_windows(self): From e86c7f7d7011137c55defa4381cb48f40df7dd73 Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Date: Mon, 27 Jan 2025 13:54:26 +0200 Subject: [PATCH 6/6] Just add the first two --- Lib/test/test__colorize.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Lib/test/test__colorize.py b/Lib/test/test__colorize.py index 1959773c4967cd..056a5306ced183 100644 --- a/Lib/test/test__colorize.py +++ b/Lib/test/test__colorize.py @@ -51,9 +51,6 @@ def check(env, fallback, expected): check({'TERM': 'dumb', 'FORCE_COLOR': '1'}, False, True) check({'FORCE_COLOR': '1', 'NO_COLOR': '1'}, True, False) - check({'FORCE_COLOR': '1', 'NO_COLOR': ''}, True, True) - check({'FORCE_COLOR': '', 'NO_COLOR': '1'}, True, False) - check({'FORCE_COLOR': '', 'NO_COLOR': ''}, True, True) for ignore_environment in False, True: # Simulate running with or without `-E`. @@ -69,9 +66,7 @@ def check(env, fallback, expected): check({'TERM': 'dumb', 'PYTHON_COLORS': '1'}, False, not ignore_environment) check({'NO_COLOR': '1', 'PYTHON_COLORS': '1'}, False, not ignore_environment) - check({'NO_COLOR': '', 'PYTHON_COLORS': '1'}, False, not ignore_environment) check({'FORCE_COLOR': '1', 'PYTHON_COLORS': '0'}, True, ignore_environment) - check({'FORCE_COLOR': '', 'PYTHON_COLORS': '0'}, True, ignore_environment) @unittest.skipUnless(sys.platform == "win32", "requires Windows") def test_colorized_detection_checks_on_windows(self):