tests: change the color of skipped functional tests

0xB10C commented at 8:04 pm on April 6, 2022: member

The dark gray color used to mark skipped functional tests is somewhat unfortunate. I don’t see skipped tests at all with my terminal settings unless I mark the text. For others, dark gray/black is hard to read on a gray background.

My terminal:

And @jacobpfickes terminal from twitter which reminded me to open this good first issue.

For example, during review it’s important to see which tests were skipped. It could happen that you think you’ve run the tests for something, but you in fact haven’t and the tests were skipped, but you just don’t notice.

A solution would be to use a yellow/orange color to mark skipped tests. Green for pass, red for fail and yellow/orange for skipped. Another easy solution would be for me to change my terminal settings, however I think the gray text on gray background could affect more devs than just me.

For reference, the colored functional tests output was introduced in #10159.

Useful skills:

Python and running the functional tests.

Want to work on this issue?

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

laanwj added the label good first issue on Apr 6, 2022

laanwj commented at 9:31 pm on April 6, 2022: member

FWIW, a common way to avoid these kind of problems is to make sure all your 8/16 terminal colors have a good contrast against your background. It’s a limited palette so having any unusable colors is a waste. The default color scheme tends to be most suitable for white backgrounds (usually) but every terminal will allow you to change them.

That said, I’m not against changing the colors but it seems like a literal shed-painting issue, there’s always going to be someone with a background shade / palette combination that will have trouble seeing some color.

jacobpfickes commented at 10:10 pm on April 6, 2022: contributor

That is a good point about never being able to satisfy every user’s needs. I went ahead and made the change to yellow if the team decides it’s a better option (I don’t think it looks too bad on a white background but I understand it’s not optimal). On the other hand, maybe another color that works well on both white and black backgrounds would satisfy the majority of users and could be a better option.

MarcoFalke commented at 6:22 am on April 7, 2022: member

I am using the default settings on my OS and I am also seeing the skipped tests very mute. No opinion on whether that needs fixing or how to fix it.

0xB10C commented at 7:21 am on April 7, 2022: member

Thanks for the input! I agree that we don’t want a PR to change the color every other month. However, I do think the status quo can be improved upon.

Another solution could be to remove the coloring of the skipped tests entirely and print them as normal, uncolored text. This should bring the best readability and avoid any future shed re-painting.

MarcoFalke commented at 7:54 am on April 7, 2022: member

Another solution could be to remove the coloring of the skipped tests entirely and print them as normal, uncolored text. This should bring the best readability and avoid any future shed re-painting.

Yeah, just using the default color, but non-bold should work just fine.

laanwj commented at 9:28 am on April 7, 2022: member

Speaking of bold, we also shouldn’t be making the assumption in scripts that bold means bright. This is something the Linux terminal historically does, but many other terminals don’t (or have configurable). So black is really black in that case.

jacobpfickes commented at 4:03 pm on April 7, 2022: contributor

I really like the idea of using the default color for skipped tests. This should cover the majority of cases and various background colors. I pushed a new commit to use a blank default color.

fanquake deleted a comment on Apr 17, 2022

CryptoUser243 commented at 9:24 pm on May 2, 2022: none

TL;DR: This issue degrades the UX of the report for a minority of users. The suggest resolution #24793 slightly degrades the UX of the remaining users. Perhaps its worth exploring whether @laanwj’s suggestion of \033[0;90m would resolve the issue whilst minimising the impact on unaffected users before moving ahead with #24793?

Given that the potential for ‘shed re-painting’ has already been raised by other users, perhaps a more detailed exposition of the factors at play, written as a set of challengable assertions and assumptions, would aid any further discussion. See below if you are interested:

On the purpose of the report and how the colour of the skipped items help to achieve this

The objectives of the report in order of priority:

state whether the test run passed or failed
list failed test - to show the user which tests should be explored further to understand what went wrong
list passed test - so the user can confirm that the expected tests actually ran and passed
highlight how long each test, and the test run in total, took so that they can identify when tests are running unusually slow
list skipped test - so the user can see if they neglected to run a test they intended to

Three UI tools are used in the in are used to indicate the result:

colour (currently Green/Red/Grey)
text label (Passed/Failed/Skipped)
icon

The text label provides the clearest indication of the outcome of the test but is only ever read peripherally as this information is embedded directly onto the name of test through its colour.

The pass/fail to green/red colour mapping are a universal standard in CLI test reports and are therefore intuitive to most users.

The skipped to ‘muted standard text colour’ colour mapping is widely, but not universal, used in CLI test reports. ‘Standard body text colour’ and yellow are other colours sometimes used.

On the impact of the issue

For a minority of users it appears as illegibly close to the background colour but different enough to make it clear that there is text present.

Workaround - most user can make it legible simply by selecting the text. This is likely to be intuitive to most to CLI users.

This provides a poor UX 1) for a nice to have feature of the report and 2) the general usage of the report as the interface is jarring and strains the eyes.

On the cause of the issue:

Terminals vary in their default background colour and their interpretation of colour values. Most allow these setting to be customised. When CLIs use colour there is no guarantee that their display will meet access ability standards.

In defense of the status quo

For the majority of users the colour chosen for skipped items appears as ‘muted standard text colour’. This is most appropriate colour for 2 reasons:

it is commonly used standards for CLI test run reports
the skipped tests are the least important information to communicate so de-emphasising them improves the usability of the report

The use of \033[1;30m' as a muted colour should work across all sensibly configured terminals. If this isn’t the case then a bug should be raised with the terminal vendor.

Some users (eg. @ayush933 and @kristapsk) have suggested #24793 (using the default colour) degrades the usability of the report.

This issue only affects a minority of users assumed to be 2-5%.

Arguments for using the default text color as proposed in #24793:

Due to the variety of colour interpretation in CLIs colours should be used sparingly. The slight usability benefits of using grey are outweighed by the significant degradation for the minority of users.

Arguments for searching for an alternative

@laanwj suggestion of \033[0;90m may resolve the issue without degrading the reports usability for other users.

Recommendation

Explore whether \033[0;90m is preferable to #24793 for both affected and unaffected users. If it is not: fallback to #24793 as the severity of the usability impact on affected users (assumed to be 2-5% of the user-base) outweighs the minor usability degradation for unaffected users.

laanwj closed this on May 10, 2022

fanquake deleted a comment on May 10, 2022

laanwj commented at 11:51 am on May 10, 2022: member

Thanks @CryptoUser243. I went with merging #24793 as IMO “unreadable” complaints weigh stronger than “it stands out too much”. It gets rid of the GREY = "\033[1;30m" (which is bold black, not grey), so is more terminal-neutral.

sidhujag referenced this in commit f5556dd9bd on May 10, 2022

DrahtBot locked this on May 10, 2023

tests: change the color of skipped functional tests #24791