Skip to content

gh-111474: Add color and a fancy output mode to regrtest#111475

Draft
Yhg1s wants to merge 10 commits intopython:mainfrom
Yhg1s:fancy-regrtest
Draft

gh-111474: Add color and a fancy output mode to regrtest#111475
Yhg1s wants to merge 10 commits intopython:mainfrom
Yhg1s:fancy-regrtest

Conversation

@Yhg1s
Copy link
Copy Markdown
Member

@Yhg1s Yhg1s commented Oct 29, 2023

Refactor regrtest to push more decisions about formatting to the Logger class. Add color support to Logger, and add a FancyLogger subclass that uses vt100 terminal control to make the test output much more condensed and easier to visually scan.

The color support for logger uses just bold green, bold red and normal yellow to print "good", "bad" and "notable" things. This makes anomalous things and interleaved test output stand out a lot more:

Screenshot of test output in a light on black terminal screen, with test passes and low load numbers colored in bold green, the time spent in (long running) tests in yellow, and other text uncolored

Color support is enabled by passing --color to regrtest (for example by setting the EXTRATESTOPTS environment variable in the make test call.) It doesn't try to detect color support. It obeys the NO_COLOR environment variable to disable color.

The fancy logger is enabled by passing --progress_reporter=fancy (or by passing --progress_reporter=detect and having a functioning terminal with a known terminal type). It enables color by default, but it still obeys NO_COLOR. The fancy logger will keep redrawing the same lines on the screen rather than printing new lines, reporting all running tests. Test output, test failures, and output that is longer than can be printed on a single line will still appear on their own lines, without being overwritten. (The exception is still-running tests; if those are too long they will get truncated as long as they are running, but are printed in full when they finish.) An additional option, --fancy_report_skip_reason, can be passed to make the fancy reporter print test skips with full test reason on a line of their own.

Here is a short demo of the fancy reporter running:

fancy-regrtest-short-demo.mp4

Here's what it looks like when test output needs to be printed:

An image of the fancy reporter reporting the passing of a test with extra output, which appears on a line of its own, with the continually updating test progress below it

And here's what it looks like when using the --fancy_report_skip_reason option is added:

An image of the fancy reporter reporting test skips, which appear on a line of their own, with the continually updating test progress below it

The refactoring to support the fancy output logger are probably enough to also enable a Tkinter-based logger, although that's more work than I'm willing to put into it (since I would not use it myself).

With a very small exception, existing behaviour is not changed. The new options (--color and --progress_reporter) have to explicitly be passed, via the EXTRATESTOPTS environment variable or the TESTOPTS make variable, to enable them. The exception is the regrtest output when it's been waiting on running tests and doesn't have any test results to report for more than 30 seconds. Before the refactoring this looked like:

0:00:00 load avg: 0.09 Run 4 tests in parallel using 4 worker processes
0:00:00 load avg: 0.09 [1/4] test_keyword passed
0:00:00 load avg: 0.09 [2/4] test_list passed
0:00:00 load avg: 0.09 [3/4] test_dict passed
0:00:30 load avg: 0.06 running (1): test_multiprocessing_forkserver.test_threads (30.3 sec)
0:00:42 load avg: 0.05 [4/4] test_multiprocessing_forkserver.test_threads passed (42.4 sec)

but now it looks like:

0:00:00 load avg: 0.03 Run 4 tests in parallel using 4 worker processes
0:00:00 load avg: 0.03 [1/4] test_keyword passed
0:00:00 load avg: 0.03 [2/4] test_list passed
0:00:00 load avg: 0.03 [3/4] test_dict passed
0:00:30 load avg: 0.07 [3/4] running (1): test_multiprocessing_forkserver.test_threads 30.3 sec
0:00:42 load avg: 0.19 [4/4] test_multiprocessing_forkserver.test_threads passed (42.5 sec)

Changing the output back is not impossible, but to my eyes the new output makes more sense anyway.

@Yhg1s
Copy link
Copy Markdown
Member Author

Yhg1s commented Oct 29, 2023

(Making the PR a draft one because I have yet to add tests for the new code.)

@gaogaotiantian
Copy link
Copy Markdown
Member

Just curious - what does the output look like if the background is white-ish? I would assume that's a very common scenario.

@Yhg1s
Copy link
Copy Markdown
Member Author

Yhg1s commented Oct 29, 2023

It depends on the terminal's colors. I don't have a dark-on-light terminal configuration handy myself (I only ever use light-on-dark), but I would generally expect such configurations to have reasonable contrasting colors for green, yellow and red. (I could make the colors configurable, but I'd still want to stick to the basic 16 vt100 colors because of their ubiquity.)

@terryjreedy
Copy link
Copy Markdown
Member

terryjreedy commented Oct 30, 2023

On Windows, new -h output is

  --color, --no-color   use color in the progress reports
  --progress_reporter {plain,fancy,detect}
  --fancy_report_skip_reason
                        in the fancy reporter, report the skip reason

The progress_reporter set should be indented 2 more spaces. argparse bug?

I tried running on Windows since I believe CommandPrompt and/or PowerShell either do or can be
made to recognize at least a subset of VT100 sequences. In logger.FancyLogger.setup_terminal (221),
the termios import and SIGWINCH accessess fail. When I commented those out and just set
self.line/columns for my window it ran apparently fine -- except that VT100 commands are simply printed.
Ditto with PowerShell (which does color coding itself of executable names and options in command
and of error messages). Perhaps someone else (such as Eryk Sun) could say how to turn on recognition
and how to query current lines and columns and whether resizes can be signaled.

Copy link
Copy Markdown
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Just looking at annotations right now :)

The feature itself looks great! Thank you!

self.color = False
self.load_threshold = os.process_cpu_count()

def error(self, s) -> str:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def error(self, s) -> str:
def error(self, s: str) -> str:

return s
return f'{self.ERROR_COLOR}{s}{self.RESET_COLOR}'

def warning(self, s) -> str:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def warning(self, s) -> str:
def warning(self, s: str) -> str:

return f'{self.INFO_COLOR}{s}{self.RESET_COLOR}'

def log(self, line: str = '') -> None:
def good(self, s) -> str:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def good(self, s) -> str:
def good(self, s: str) -> str:

return s
return f'{self.GOOD_COLOR}{s}{self.RESET_COLOR}'

def load_color(self, load_avg: float):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def load_color(self, load_avg: float):
def load_color(self, load_avg: float) -> str:

load = self.error(load)
return load

def state_color(self, text: str, state: str | None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def state_color(self, text: str, state: str | None):
def state_color(self, text: str, state: State | str | None) -> str:

if self._quiet:
return
def display_progress(self, test_index: int, text: str,
stdout: str|None = None) -> None:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit

Suggested change
stdout: str|None = None) -> None:
stdout: str | None = None) -> None:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(and in other places)

class StatusRemovingWrapper(io.TextIOWrapper):
# Set status_size here to avoid having to override __init__.
status_size = 0
def write(self, msg):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def write(self, msg):
def write(self, msg: str) -> None:

https://github.com/python/typeshed/blob/f7aa7b709a826ed34f52b1de3f7095f90f349a9c/stdlib/io.pyi#L143

super().write(msg)


def replace_stdout():
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def replace_stdout():
def replace_stdout() -> None:

run_workers.PROGRESS_UPDATE = 5
super().__init__(results, ns)

def setup_terminal(self):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def setup_terminal(self):
def setup_terminal(self) -> None:

sys.stdout.status_size = report.count('\n') + 1 # type: ignore[attr-defined]


def detect_vt100_capability():
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
def detect_vt100_capability():
def detect_vt100_capability() -> bool:

@terryjreedy
Copy link
Copy Markdown
Member

One way to implement a tkinter alternative in a followup issue/PR would be to add a 'tkgui' (or whatever) option to the reporter options. If selected, it could initialize a GuiLogger subclass of FancyLogger with a different setup_terminal() that would initialize a TestOutput window and do a different stdout replacement. Then the VT100 commands could be removed (with REs) in that replacement and replaced with terminal actions. I believe logic in the replacement write() function could replace the status removal stuff.

@hugovk
Copy link
Copy Markdown
Member

hugovk commented Oct 30, 2023

Color support is enabled by passing --color to regrtest (for example by setting the EXTRATESTOPTS environment variable in the make test call.) It doesn't try to detect color support. It obeys the NO_COLOR environment variable to disable color.

Something to consider: many tools accept a three-state option for --color. For example, pytest accepts:

  --color=color         Color terminal output (yes/no/auto)

And pre-commit:

  --color {auto,always,never}
                        Whether to use color in output. Defaults to `auto`.

Following https://no-color.org/, these options override NO_COLOR (and FORCE_COLOR).

@github-actions
Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants