Improve Test to use colors to highlight failures and diffs

[fingolfin]

... and some other test code, to make it easier to see and understand diffs in there.

Consider this test file:

gap> 1+1;
2 
gap> 2+2;
4

This will actually cause a test failure, due to whitespace, which makes it annoying to discover. Currently, Test will produce something like this:

With this PR, you'll get this:

This change is IMHO useful beyond better understanding whitespace diffs. For example when I run the GAP test suite and there are multiple failures, it can be challenging to visually parse what is being displayed. With this PR, I find it much easier to understand what is going on.

Here is a more complex example:

gap> 1+1;
2 
gap> 2+2;
4
# comment in a bad place

gap> 3+3;
6
gap> Print("1\n2\n3\n");
1
2
3
gap> true
true

Which currently produces this, which I find hard to make sense out (basically, I resorted to trying to fix the first one and then hope I can repeat this; or I use the rewriteFile:=true option):

With this PR (the output could still be nicer, but it's a start):

[ChrisJefferson]

Does this make the output hard to read in less, and similar programs, by filling them with colour codes? (does GAPDoc already handle this by dropping them based on where stdout is going?)

[fingolfin]

I've never ever tried to send the output of Test() into a pager. But in general, less can handle color escape codes just fine with the right settings (namely -r or -R).

So I just tried ./gap -c 'Test("simple.tst"); QUIT;' | less -r and it worked fine. But without the -r, less indeed prints the escape sequences. But that's the normal thing for less, isn't it?

[wilfwilson]

I do like the idea of colours in the output. However, I use a terminal with a white background, and the yellow text on a white background is unpleasant to read.

Could you find a solution for the currently-yellow text that has high contrast with both light and dark terminal backgrounds? Or perhaps only colour the ####### bit (which doesn't actually need to be read), and leave the other yellow text as it was?

[fingolfin]

Yeah colors could be tweaked; I also wonder about people who are red-green blind.

[olexandr-konovalov]

Can you deliberately break some more tests for testing this PR, so that we can see how GitHub actions will render colours.

[fingolfin]

@wilfwilson there are some more colors we could pick from. Also, background color can be used, and of course any combination for foreground and background color could be used. But of course most terminals also allow overriding these colors, so there won't be a single "optimal" choice, ever... julia has a --color=yes/no command line option to be able to disable or enable use of colors. In GAP, we have ColorPrompt and I guess we could also add a test option to force disabling/enabling colors.

@alex-konovalov GitHub Actions renders the colors just fine, see e.g

[wilfwilson]

Actually I'm happy enough with it as it is.

[fingolfin]

@alex-konovalov GH Actions handles the colors just fine. But I introduced a failure so you can look for yourself, e.g. at https://github.com/gap-system/gap/pull/4170/checks?check_run_id=1427795100

@wilfwilson well, I am really open to change the colors, I just don't know what a good compromise might be. Perhaps one could explicitly set foreground and background color to increase the chance of good contrast? Playing with the code to change the colors is relatively easy.

[wilfwilson]

I had thought about it a bit, and didn't feel that I could suggest an improvement. However I have thought about it a bit more.

I think combining text and bg colours is a good way to make sure that the text looks decent in light and dark terminal windows. And this is my preference (not strongly held opinion though):

• Text with green background should be 'black'
• Text with red background should be 'white'
• The currently yellow text should be given a black background, or this should be inverted to black text on a yellow background.

Here are those styles on white/black terminal backgrounds:

I think they're all quite clear.

[fingolfin]

That's fine by me; but of course it won't be perfect in all cases: someone might have a terminal theme with a very bright red and a super dark green... There are terminals which allow specifying 24bit RGB values... but I guess that's overkill ;-).

Anyway, I used your color suggestions (with yellow-on-black), and also made these potentially configurable via a global record DefaultReportDiffColors (undocumented for now).

[wilfwilson]

Yes, but although it might look ugly in some bizarre terminal setups, hopefully it'll all still be readable 🙂

Thanks. I'm happy with it. The record is a good idea.

[frankluebeck]

A few comments, since I have experimented a bit with colors in terminals:

• avoid yellow (in standard terminal settings), it is difficult to read on many terminals
• also avoid using background/foreground combinations, these are unpleasant to read (I think I have removed all background settings in GAPDoc)
• colors should not be used if the user preference "UseColorsInTerminal" is set to false
• you could use a user preference to enable users to change the colors (e.g., GAPDoc has the user preference "TextTheme"); any setting without yellow is probably a reasonable default for most users

[codecov]

Codecov Report

Merging #4170 (2199f52) into master (d0fb91e) will increase coverage by 1.65%.
The diff coverage is 27.90%.

@@            Coverage Diff             @@
##           master    #4170      +/-   ##
==========================================
+ Coverage   80.17%   81.83%   +1.65%     
==========================================
  Files         637      637              
  Lines      272021   272660     +639     
==========================================
+ Hits       218086   223123    +5037     
+ Misses      53935    49537    -4398     

Impacted Files	Coverage Δ
lib/test.gi	63.13% <27.90%> (-1.76%)	⬇️
src/baltree.h	0.00% <0.00%> (-97.53%)	⬇️
src/dynarray.h	0.00% <0.00%> (-94.12%)	⬇️
src/libgap-api.c	4.50% <0.00%> (-59.50%)	⬇️
lib/process.gi	26.42% <0.00%> (-43.58%)	⬇️
src/sysjmp.c	52.17% <0.00%> (-30.44%)	⬇️
src/compiler.c	68.06% <0.00%> (-20.67%)	⬇️
lib/files.gi	58.33% <0.00%> (-17.51%)	⬇️
src/error.c	74.78% <0.00%> (-14.71%)	⬇️
src/vars.c	82.22% <0.00%> (-11.64%)	⬇️
... and 158 more

[fingolfin]

A few comments, since I have experimented a bit with colors in terminals:

• avoid yellow (in standard terminal settings), it is difficult to read on many terminals

I changed it to blue now, which looks OK for me in various terminal theme choices (but of course you can always find places where any choice is problematic, e.g. if someone really loves a blue background for their terminal.... though at least in my terminal, I would then also change the "blue color" to be something else)

• also avoid using background/foreground combinations, these are unpleasant to read (I think I have removed all background settings in GAPDoc)

While I am not a fan of them, but for this purpose I think they make sense, because they make it possible to "see" whitespace issues, which often are an annoying source of test diffs.

• colors should not be used if the user preference "UseColorsInTerminal" is set to false

Good point, and added.

• you could use a user preference to enable users to change the colors (e.g., GAPDoc has the user preference "TextTheme"); any setting without yellow is probably a reasonable default for most users

I'll need to look into the userpref mechanism to see whether I can do it / whether to put it in this PR or a follow-up.

[wilfwilson]

I'm happy with this.

[fingolfin]

Here is a fresh side-by-side comparison screenshot. The output in the left is basically impossible for me to read correctly. The one on the right with colors still is nasty, but I can manage.

[fingolfin]

I'd rather not delay this for adding UserPreference: the colors already are customizable by tweaking DefaultReportDiffColors in gap.ini, and if a userpref is desired, we can add one later; but I'd like to benefit from the colors now...