test: Change color of skipped functional tests #24793

pull jacobpfickes wants to merge 1 commits into bitcoin:master from jacobpfickes:change_skipped_test_color changing 1 files +5 −5

jacobpfickes commented at 10:02 PM on April 6, 2022: contributor

changes the color of skipped functional tests (currently grey and can be hard to read/invisible on dark backgrounds) to yellow.

resolves #24791
DrahtBot added the label Tests on Apr 6, 2022
jacobpfickes force-pushed on Apr 6, 2022
0xB10C commented at 7:38 AM on April 7, 2022: member

Thanks Jacob for picking this up!

Reviewers should also read #24791. As mentioned there, we want to avoid having PRs changing the color every other month. Maybe removing the coloring of the skipped tests altogether is a alternative if this is too controversial or there are users for which this doesn't work.

For me, this change makes the output visible and readable.

<details> <summary> before and after screenshots on my system </summary>

before:

with this PR:

</details>
laanwj renamed this:
~~test: changes color of skipped functional tests~~
test: Change color of skipped functional tests
on Apr 7, 2022
laanwj commented at 9:48 AM on April 7, 2022: member

look at the pretty colors !!!

No, I was skeptical but have to admit this is better. It's a similar argument as with code comments, you don't want an editor style that hides them. Black/grey is always a subdued color (at least on a dark-ish background). ACK 230e945cd029640a465b7c12371d732c18d6628d
jacobpfickes force-pushed on Apr 7, 2022
theStack commented at 3:56 PM on April 7, 2022: member

Concept ACK

And warm welcome as a new contributor! 🎉
jacobpfickes force-pushed on Apr 7, 2022

changes color of skipped functional tests

Changes the color of skipped functional tests to the default text color of the terminal. This will make skipped tests easy to read on the majority of background colors rather than the original grey color (hard to read on dark backgrounds) and the proposed yellow change (hard to read on white backgrounds)

3258bad996

jacobpfickes force-pushed on Apr 7, 2022
jacobpfickes commented at 4:19 PM on April 7, 2022: contributor

As discussed in #24791 I pushed and squashed some new changes to use the default text color for skipped tests instead. The original color of grey was hard to view on dark backgrounds and the proposed change to yellow would be hard to view on light backgrounds but the default color should support most use cases.
kristapsk commented at 6:51 PM on April 7, 2022: contributor

With my terminal settings, before: After:

Personally prefer old one, but, OTOH, NBD.
brunoerg commented at 6:57 PM on April 7, 2022: member

Results on my terminal: <img width="635" alt="Screen Shot 2022-04-07 at 14 26 45" src="https://user-images.githubusercontent.com/19480819/162261813-4b05e08e-28fe-4034-9675-119c4c59a68e.png">

I think the white color stands out more than the others ones, I prefer the old one because that 'color' doesn't stand out so much compared to the other colors (which is good since these tests were skipped).
ayush933 commented at 12:36 PM on April 10, 2022: contributor

Before:

After:

With the new colour scheme the skipped tests stand out more than the passed ones on my terminal so I prefer the old settings, but NBD.

in test/functional/test_runner.py:65 in 3258bad996

  58 | @@ -59,10 +59,10 @@
  59 |          kernel32.SetConsoleMode(stderr, stderr_mode.value | ENABLE_VIRTUAL_TERMINAL_PROCESSING)
  60 |      # primitive formatting on supported
  61 |      # terminal via ANSI escape sequences:
  62 | +    DEFAULT = ('\033[0m', '\033[0m')
  63 |      BOLD = ('\033[0m', '\033[1m')
  64 |      GREEN = ('\033[0m', '\033[0;32m')
  65 |      RED = ('\033[0m', '\033[0;31m')
  66 | -    GREY = ('\033[0m', '\033[1;30m')

laanwj commented at 6:29 PM on April 13, 2022:

As said here, if we want to keep using dark grey, let's at least use \033[0;90m instead of bold black, which doesn't actually render as grey on many terminals.

laanwj commented at 11:13 AM on May 10, 2022:

Merging this because it's an improvement in at least this regard.

laanwj commented at 6:30 PM on April 13, 2022: member

I'm also fine with using the default color. That said, this is becoming a bikeshed-fest just as I expected.
theStack approved
theStack commented at 3:27 PM on April 18, 2022: member

Tested ACK 3258bad996262792ba77573d6080dafa3952929c

Personally I never had problems as on my terminal the original grey color is clearly visible, but considering that for some it isn't (as seen in some of the screenshots posted) I think it makes sense to change to default color.
jarolrod commented at 4:58 AM on April 21, 2022: member

Tested ACK https://github.com/bitcoin/bitcoin/commit/3258bad996262792ba77573d6080dafa3952929c

To me, this is an improvement. Thanks for working on this, and welcome as a new contributor :)
laanwj merged this on May 10, 2022
laanwj closed this on May 10, 2022
jacobpfickes deleted the branch on May 10, 2022
sidhujag referenced this in commit f5556dd9bd on May 10, 2022
DrahtBot locked this on May 10, 2023

Contributors

Labels

Linked (view graph)

#24791 tests: change the color of skipped functional tests