Skip to content

Conversation

juj
Copy link
Collaborator

@juj juj commented Oct 4, 2025

Another take at adding colors to test harness:

image

Tested to work on:

  • Windows 10 x64
  • Windows 11 ARM64
  • MacOS 15.5 Sequoia Apple M1
  • Linux Debian 13 Trixie ARM64

@juj
Copy link
Collaborator Author

juj commented Oct 4, 2025

The screenshot shows a glitch in the color output, on line with test_float_builtins + test_float_literals, but that is an unrelated long standing existing bug on Windows. E.g. on current main before this PR, there are some stray escapes getting printed in the console:

image



def with_color(color, text):
return f'\033[9{color}m{text}\033[0m'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why wouldn't this be output_color() + text + reset_color()?

Why is ok to use ansi here, even on windows, when we use different mechanism for windows above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

output_color() and reset_color() adjust the current terminal state already before printing. That works for full lines to be printed, e.g.

output_color(...)
print('asdf')
reset_color()

but not for adjusting color on an individual line.

Why is ok to use ansi here

That I actually don't know. In general Python it is not ok on Windows to do that (i.e. if you write a standalone python file with a print('\033[92m HELLO \033[0m'), it will not work on Windows.

But we have run some code somewhere else to enable the current terminal to use ANSI escape codes on Windows. Modules like colorama seem to do that, but I don't know what it is that enables that in Emscripten's case for Windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But output_color and reset_color just return strings don't they? it should be possible to construct your string here by using those function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

output_color() calls to output_color_windows(), which calls to SetConsoleTextAttribute, which is a global state for the current terminal.

Copy link
Collaborator

@sbc100 sbc100 Oct 6, 2025

Choose a reason for hiding this comment

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

OK, but given that with_color in this PR doesn't work on windows anyway why not just do:

def with_color(color, text): 
   if WINDOWS:
     ruturn text
   return output_color() + text + reset_color()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with_color() does work on both Windows 10 and 11.. the screenshot I posted in the PR description is a screenshot from Windows cmd.exe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, in that case I think we should probably just delete the current output_color_windows code.

I'm assuming more users are on windows 10 or 11 these days? Users of older versions of windows just wouldn't get color?

i.e. if we are going to assume ANSI support on windows lets just do that everywhere maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like we can detect it using ENABLE_VIRTUAL_TERMINAL_PROCESSING... (I will take a stab at it unless you are feeling like it?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds like a good idea, though I'd rather leave that for a future refactor PR, since I don't yet know the extent here.

In general for Windows, I think we can call all pre-Windows 10 versions unsupported. (i.e. XP, Vista, 7, 8, 8.1 all are obsolete). On Windows 10, we could take e.g. Win10 21H1 (build 19043, May 18, 2021) as the minimum version (not that it would matter for Emscripten I believe)

msg = f'{test} ... ERROR'
errlog(f'{self.compute_progress()}{with_color(RED, msg)}')
self.buffered_result = BufferedTestError(test, err)
self.test_result = 'errored'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this work for -j1 too? i.e. the non-parallel runner? Maybe as a followup? Maybe the default python runner already has some color support?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be nice (also to add the [35%] progress bar), though looks more difficult, since in non-parallel runner the printing is handled by the built-in TextTestResult class, which I don't know how to replace.

Another thing I'd want to do is remove the long wall of test results dump at the very end of the test run, which repeats the same results from the test run a second time.. that is a bit spammy.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 6, 2025
Windows 10 and above support ANSI coloring and there should be very few
emscripten users on older version of windows at this point (if any?).

The downside is that windows 7 will no longer have nice colored error
messages, but the upside is that we can cleanup and refactor this code
now (see emscripten-core#25495).
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 6, 2025
Windows 10 and above support ANSI coloring and there should be very few
emscripten users on older version of windows at this point (if any?).

The downside is that windows 7 will no longer have nice colored error
messages, but the upside is that we can cleanup and refactor this code
now (see emscripten-core#25495).
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 6, 2025
Windows 10 and above support ANSI coloring and there should be very few
emscripten users on older version of windows at this point (if any?).

The downside is that windows 7 will no longer have nice colored error
messages, but the upside is that we can cleanup and refactor this code
now (see emscripten-core#25495).
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 6, 2025
Windows 10 and above support ANSI coloring and there should be very few
emscripten users on older version of windows at this point (if any?).

The downside is that windows 7 will no longer have nice colored error
messages, but the upside is that we can cleanup and refactor this code
now (see emscripten-core#25495).
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 6, 2025
Windows 10 and above support ANSI coloring and there should be very few
emscripten users on older version of windows at this point (if any?).

The downside is that windows 7 will no longer have nice colored error
messages, but the upside is that we can cleanup and refactor this code
now (see emscripten-core#25495).
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 6, 2025
Windows 10 and above support ANSI coloring and there should be very few
emscripten users on older version of windows at this point (if any?).

The downside is that windows 7 will no longer have nice colored error
messages, but the upside is that we can cleanup and refactor this code
now (see emscripten-core#25495).
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 6, 2025
Windows 10 and above support ANSI coloring and there should be very few
emscripten users on older version of windows at this point (if any?).

The downside is that windows 7 will no longer have nice colored error
messages, but the upside is that we can cleanup and refactor this code
now (see emscripten-core#25495).
sbc100 added a commit that referenced this pull request Oct 6, 2025
Windows 10 and above support ANSI coloring and there should be very few
emscripten users on older version of windows at this point (if any?).

emscripten already does not support windows 7 IIUC, since its
dependencies (both python 3 and node 18) do not support windows 7

This means that only windows 8 users should be effected by this change.
The upside is that we can cleanup and refactor this code now (see
#25495).


def with_color(color, text):
return f'\033[9{color}m{text}\033[0m'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can now be output_color() + text + reset_color() right? Now that #25502 has landed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm,

def output_color(color):
  assert color_enabled
  return '\033[3%sm' % color

this generates a different code than \033[9{color}m that I was using. I wonder where that \033[3 comes from. Gives blue instead of cyan.. but green and red are intact.

But maybe blue looks nicer than cyan, I'll let that be like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I was wondering that myself, certainly worth not have two ways to output a given color (at least not without documenting).

@juj juj force-pushed the test_harness_colors branch from 9cc9962 to 8fc7709 Compare October 6, 2025 22:35
@juj
Copy link
Collaborator Author

juj commented Oct 6, 2025

CircleCI fails on

  File "C:\Users\circleci\path with spaces ()\test\parallel_testsuite.py", line 308, in compute_progress
    return with_color(CYAN, val)
  File "C:\Users\circleci\path with spaces ()\tools\diagnostics.py", line 61, in with_color
    return output_color(color) + text + reset_color()
           ~~~~~~~~~~~~^^^^^^^
  File "C:\Users\circleci\path with spaces ()\tools\diagnostics.py", line 46, in output_color
    assert color_enabled
           ^^^^^^^^^^^^^
AssertionError
"""

although locally for me on Windows 10, test run is good with colors.

@juj
Copy link
Collaborator Author

juj commented Oct 6, 2025

Demoted the code to output non-colors instead of asserting, if colors not enabled.. Guess that should be per design?

@sbc100
Copy link
Collaborator

sbc100 commented Oct 6, 2025

CircleCI fails on

  File "C:\Users\circleci\path with spaces ()\test\parallel_testsuite.py", line 308, in compute_progress
    return with_color(CYAN, val)
  File "C:\Users\circleci\path with spaces ()\tools\diagnostics.py", line 61, in with_color
    return output_color(color) + text + reset_color()
           ~~~~~~~~~~~~^^^^^^^
  File "C:\Users\circleci\path with spaces ()\tools\diagnostics.py", line 46, in output_color
    assert color_enabled
           ^^^^^^^^^^^^^
AssertionError
"""

although locally for me on Windows 10, test run is good with colors.

Ah I guess we should do if not color_enabled: return text maybe?

@juj juj enabled auto-merge (squash) October 6, 2025 23:23
@sbc100 sbc100 disabled auto-merge October 7, 2025 00:02
@sbc100 sbc100 merged commit 1309a5e into emscripten-core:main Oct 7, 2025
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants