Py tooling: fix Windows terminal color output and handle color flags in compare.py (fixes #642) by jdymitarai · Pull Request #2297 · google/benchmark

jdymitarai · 2026-09-10T01:49:57Z

Summary

This PR fixes issue #642 where compare.py produced literal ANSI escape characters (^[[92m, ^[[0m, etc.) on Windows consoles, and ensures that color settings (--color, --no-color, --benchmark_color=...) are properly recognized regardless of argument position.

Root Cause

Windows Console VT Processing: On Windows, ANSI escape sequences (\033[...]) require ENABLE_VIRTUAL_TERMINAL_PROCESSING (0x0004) to be set on the console output buffer. Without this flag, cmd.exe and standard console hosts print literal escape sequences.
Auto-detection & Redirects: compare.py previously defaulted color=True unconditionally, even when stdout was not a TTY (such as redirected to a file or piped to another tool), or when the standard NO_COLOR environment variable was set.
Argparse Remainder Flag Leak: Because subparsers in compare.py collect trailing options into benchmark_options (nargs=argparse.REMAINDER), placing --no-color or --benchmark_color=false after the subcommand or arguments caused the flag to be captured as benchmark runner options instead of setting args.color, while triggering a spurious warning when inputs were JSON files.
Test import issue: In tools/gbench/report.py line 1492, import util caused ModuleNotFoundError when run via python -m unittest.

Changes

Windows VT Processing: Enable ENABLE_VIRTUAL_TERMINAL_PROCESSING on Windows console handles when available, and disable color formatting if VT mode is not supported by the console.
Color Auto-Detection: Auto-detect TTY output (sys.stdout.isatty()) and respect NO_COLOR standard (https://no-color.org/).
Flag Extraction: Allow both --color and --no-color on root parser, and extract --color, --no-color, and --benchmark_color=false|true|auto from trailing benchmark_options.
Suppress False Warning: Avoid printing "WARNING: passing --benchmark_color=... has no effect since both inputs are JSON" in check_inputs.
Test Fix & Unit Tests: Fix from gbench import util in report.py, and add unit tests in compare.py covering color flag parsing, option resolution, and NO_COLOR handling.
Added contributor details to AUTHORS and CONTRIBUTORS.

Verification

All 35 Python unit tests in tools/ pass:

python -m unittest discover -s tools -p "*.py"

C++ library builds cleanly with CMake on MSVC:

cmake --build build --config Release --target benchmark

…in compare.py (fixes google#642) * Enable virtual terminal processing on Windows console handles when supported so ANSI escape codes are rendered natively instead of printing literal '^[[92m' sequences. * Fall back to disabled colors when stdout is not a TTY or when the standard NO_COLOR environment variable is present. * Support --color and --no-color on the root parser and correctly extract --color, --no-color, and --benchmark_color=false|true|auto if provided after subcommands. * Prevent false warnings in check_inputs when --benchmark_color is provided with JSON inputs. * Fix 'import util' in tools/gbench/report.py test to 'from gbench import util'. * Add unit tests covering color flag parsing, option extraction, and NO_COLOR detection.

dmah42 · 2026-09-10T09:18:47Z


-    parser.add_argument(
+    color_group = parser.add_mutually_exclusive_group()
+    color_group.add_argument(


isn't the opposite of --no-color just --color? why do we need both flags?

+1. I do not get why --color needs to exist. This is the default already.

jdymitarai · 2026-09-10T10:45:20Z

Previously, \compare.py\ only had --no-color\ (with \default=True), which meant color could only be turned off, and could never be explicitly forced on (e.g., when piping output into \less -R\ or in CI runners where stdout is not detected as an interactive TTY).

We added --color\ to allow explicitly forcing color output, while retaining --no-color\ for 100% backwards compatibility with existing user scripts and CLI habits. When neither flag is specified (\default=None), \compare.py\ auto-detects whether stdout is an interactive TTY and whether VT processing is supported (and honors the standard \NO_COLOR\ environment variable).

dmah42 · 2026-09-10T10:46:56Z

Previously, \compare.py\ only had --no-color\ (with \default=True), which meant color could only be turned off, and could never be explicitly forced on (e.g., when piping output into \less -R\ or in CI runners where stdout is not detected as an interactive TTY).

We added --color\ to allow explicitly forcing color output, while retaining --no-color\ for 100% backwards compatibility with existing user scripts and CLI habits. When neither flag is specified (\default=None), \compare.py\ auto-detects whether stdout is an interactive TTY and whether VT processing is supported (and honors the standard \NO_COLOR\ environment variable).

that explains what it does, but not why. why do we need to force color on?

LebedevRI · 2026-09-10T16:28:32Z

(This also probably needs LLM disclosure notice.)

jdymitarai · 2026-09-10T23:59:29Z

Thanks for the feedback!

Why force color on?

When compare.py runs in non-interactive environments—such as CI pipelines (GitHub Actions, GitLab CI, Buildkite) or when piping through pagers/utilities (compare.py ... | less -R or compare.py ... | tee log.txt)—sys.stdout.isatty() evaluates to False.

Without a flag to explicitly force color on, the output will unconditionally strip colors in those environments, even though modern CI dashboards and pagers like less -R fully support rendering ANSI color codes. Allowing users to pass --color mirrors standard CLI behavior across tooling (e.g., git --color=always, grep --color=always, pytest --color=yes, ls --color=always) so that colored diffs can be preserved in CI logs and terminal pipelines.

Disclosure: AI assistance (LLM) was used in formulating this patch and tests.

dmah42 · 2026-09-11T15:31:20Z

        # When both sides are JSON the only supported flag is
        # --benchmark_filter=
        for flag in util.remove_benchmark_flags("--benchmark_filter=", flags):
+            if flag.startswith("--benchmark_color="):


why is this here?

dmah42 · 2026-09-11T15:32:37Z

i guess i don't have a particularly strong opinion. it's a lot of code to add for a fairly niche use-case.

LebedevRI · 2026-09-11T17:49:49Z

I have a question. If this change itself does not enable the coloring,
and you have to change the callers (ci etc) to pass extra CLI argument to benefit,
can you not instead change CI etc to globally enable coloring in the terminal?
Why must this be specifically done in this tooling?

I don't really know much about windows terminal,
but i'd guess there is a way to do that with a simple terminal command?

jdymitarai · 2026-09-11T21:48:33Z

Hi @LebedevRI, thanks for asking! Let me clarify both points:

1. Does this enable coloring automatically?

Yes, it does enable coloring automatically out-of-the-box without requiring any extra CLI arguments.

For any interactive terminal session (Linux, macOS, and Windows):

\compare.py\ auto-detects that stdout is a TTY (\sys.stdout.isatty() == True).
On Windows, the reason issue On Windows there is formating error when using compare.py #642 occurred is that the Windows Console host (\conhost.exe\ / \cmd.exe) has \ENABLE_VIRTUAL_TERMINAL_PROCESSING\ (\

LebedevRI

To make this less contentious, i'd propose

Drop --color. The default is already always-on with --no-color opt-out.
Drop --benchmark_color= handling.
Keep enable_virtual_terminal_processing()
Adjust should_use_color() into a legality check - should auto-disable colors when coloring is not possible.

…r, keep VT enable and legality check

jdymitarai · 2026-09-11T23:27:15Z

Hi @LebedevRI,

Thank you for the constructive suggestion! I have updated the PR following your 4 points:

**Dropped --color**: Kept only the original --no-color\ flag (with \default=True).
Dropped --benchmark_color=\ handling: Removed all internal option interception.
**Kept \enable_virtual_terminal_processing()**: Enables VT mode on the Windows console handle so ANSI sequences render natively on Windows.
Adjusted \should_use_color()\ into a legality check:
- Auto-disables color when \NO_COLOR\ is present in \os.environ.
- Auto-disables color when \sys.stdout\ is not an interactive TTY (or dumb terminal).
- On Windows, auto-disables color if \enable_virtual_terminal_processing()\ fails.

The PR diff is now much smaller and focused strictly on Windows VT console support and the legality check. All tests pass.

LebedevRI · 2026-09-11T23:32:13Z

Suggested change

default=should_use_color(),

LebedevRI · 2026-09-11T23:39:17Z

+        )
+        self.assertFalse(parsed.color)
+
+    def test_benchmarks_color_default_true(self):


I think we can't test what the default is.

LebedevRI · 2026-09-11T23:40:17Z

+class TestShouldUseColor(unittest.TestCase):
+    def test_should_use_color_no_color_env(self):
+        orig = os.environ.get("NO_COLOR")
+        try:
+            os.environ["NO_COLOR"] = "1"
+            self.assertFalse(should_use_color())
+        finally:
+            if orig is None:
+                os.environ.pop("NO_COLOR", None)
+            else:
+                os.environ["NO_COLOR"] = orig


This test does not look hermetic.
Can unittest.mock.patch be used here?

…and mock test

jdymitarai · 2026-09-12T00:03:32Z

Thanks @LebedevRI for the review!

I have applied all three suggestions in commit \57ffcd9:

Set \default=should_use_color()\ directly on the --no-color\ flag in \create_parser(), eliminating redundant post-parse color checking in \main().
Removed \ est_benchmarks_color_default_true\ to avoid testing an environment-dependent dynamic default.
Updated \TestShouldUseColor\ to be hermetic using \unittest.mock.patch\ (testing both \NO_COLOR\ environment variable suppression via @patch.dict\ and non-interactive TTY via @patch('sys.stdout.isatty')).

All 16 unit tests pass.

LebedevRI · 2026-09-12T00:07:45Z

+        self.assertFalse(should_use_color())
+
+    @patch.dict(os.environ, {}, clear=True)
+    @patch("sys.stdout.isatty", return_value=False)
+    def test_should_use_color_not_a_tty(self, mock_isatty):
+        self.assertFalse(should_use_color())


I'd think this should test the actual result of parsing, not should_use_color().

LebedevRI · 2026-09-12T00:09:45Z


    def test_json_diff_report_pretty_printing(self):
-        import util
+        from gbench import util


Hm, is this to allow running tests from prebuilt package?
I'm mainly worried that it would no longer be possible to run tests in source checkout.

jdymitarai force-pushed the fix-compare-windows-color-642 branch from 8799b00 to 80784e6 Compare September 10, 2026 08:51

Merge branch 'main' into fix-compare-windows-color-642

3490429

dmah42 reviewed Sep 10, 2026

View reviewed changes

dmah42 reviewed Sep 11, 2026

View reviewed changes

LebedevRI reviewed Sep 11, 2026

View reviewed changes

Simplify color handling per review: drop --color and --benchmark_colo…

08dbbc1

…r, keep VT enable and legality check

LebedevRI reviewed Sep 11, 2026

View reviewed changes

Comment thread tools/compare.py Outdated

LebedevRI Sep 11, 2026

Copy link
Copy Markdown

Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

default=should_use_color(),

LebedevRI reviewed Sep 11, 2026

View reviewed changes

Py tooling: address review comments from @LebedevRI on default color …

57ffcd9

…and mock test

LebedevRI reviewed Sep 12, 2026

View reviewed changes

Uh oh!

Conversation

jdymitarai commented Sep 10, 2026

Summary

Root Cause

Changes

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdymitarai commented Sep 10, 2026

Uh oh!

dmah42 commented Sep 10, 2026

Uh oh!

LebedevRI commented Sep 10, 2026

Uh oh!

jdymitarai commented Sep 10, 2026

Why force color on?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmah42 commented Sep 11, 2026

Uh oh!

LebedevRI commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdymitarai commented Sep 11, 2026

1. Does this enable coloring automatically?

Uh oh!

LebedevRI left a comment

Choose a reason for hiding this comment

Uh oh!

jdymitarai commented Sep 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdymitarai commented Sep 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LebedevRI commented Sep 11, 2026 •

edited

Loading