[PATCH] Make test output coloring more intuitive

1. Change the color of individual known breakages from bold green to
   bold yellow.  This seems more appropriate when considering the
   universal traffic lights coloring scheme, where green conveys the
   impression that everything's OK, and amber that something's not
   quite right.

2. Likewise, change the color of the summarized total number of known
   breakages from bold red to bold yellow to be less alarmist and more
   consistent with the above.

3. Change color of unexpectedly fixed known breakages to bold red.  An
   unexpectedly passing test indicates that the test is wrong or the
   semantics of the code being tested have changed.  Either way this
   is an error which is arguably as bad as a failing test, and as such
   is now counted in the totals too.

The end result of these changes is that:

  - red is _only_ used for things which have gone unexpectedly wrong:
    test failures, unexpected test passes, and failures with the
    framework,

  - yellow is _only_ used for known breakages, and

  - green is _only_ used for things which have gone to plan and
    require no further work to be done.

Signed-off-by: Adam Spiers <redacted>
---
 t/t0000-basic.sh |  7 ++++---
 t/test-lib.sh    | 13 ++++++++-----
 2 files changed, 12 insertions(+), 8 deletions(-)
 mode change 100644 => 100755 t/test-lib.sh

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index ae6a3f0..4e111b4 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh

@@ -81,9 +81,10 @@ test_expect_success 'pretend we have fixed a known breakage (run in sub test-lib
 	./passing-todo.sh >out 2>err &&
 	! test -s err &&
 	sed -e 's/^> //' >expect <<-\\EOF &&
-	> ok 1 - pretend we have fixed a known breakage # TODO known breakage
-	> # fixed 1 known breakage(s)
-	> # passed all 1 test(s)
+	> ok 1 - pretend we have fixed a known breakage # TODO known breakage vanished
+	> # fixed 1 known breakage(s); please update test(s)
+	> # still have 1 known breakage(s)
+	> # passed all remaining 0 test(s)
 	> 1..1
 	EOF
 	test_cmp expect out)

diff --git a/t/test-lib.sh b/t/test-lib.sh
old mode 100644
new mode 100755
index f8e3733..9907035
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -183,10 +183,12 @@ then
 			tput bold; tput setaf 1;; # bold red
 		skip)
 			tput bold; tput setaf 2;; # bold green
+		warn)
+			tput bold; tput setaf 3;; # bold yellow
 		pass)
 			tput setaf 2;;            # green
 		info)
-			tput setaf 3;;            # brown
+			tput setaf 3;;            # yellow/brown
 		*)
 			test -n "$quiet" && return;;
 		esac

@@ -276,12 +278,13 @@ test_failure_ () {
 
 test_known_broken_ok_ () {
 	test_fixed=$(($test_fixed+1))
-	say_color "" "ok $test_count - $@ # TODO known breakage"
+	test_broken=$(($test_broken+1))
+	say_color error "ok $test_count - $@ # TODO known breakage vanished"
 }
 
 test_known_broken_failure_ () {
 	test_broken=$(($test_broken+1))
-	say_color skip "not ok $test_count - $@ # TODO known breakage"
+	say_color warn "not ok $test_count - $@ # TODO known breakage"
 }
 
 test_debug () {

@@ -371,11 +374,11 @@ test_done () {
 
 	if test "$test_fixed" != 0
 	then
-		say_color pass "# fixed $test_fixed known breakage(s)"
+		say_color error "# fixed $test_fixed known breakage(s); please update test(s)"
 	fi
 	if test "$test_broken" != 0
 	then
-		say_color error "# still have $test_broken known breakage(s)"
+		say_color warn "# still have $test_broken known breakage(s)"
 		msg="remaining $(($test_count-$test_broken)) test(s)"
 	else
 		msg="$test_count test(s)"

-- 
1.7.12.147.g6d168f4

On Mon, Sep 17, 2012 at 12:50:37PM +0100, Adam Spiers wrote:

The end result of these changes is that:

  - red is _only_ used for things which have gone unexpectedly wrong:
    test failures, unexpected test passes, and failures with the
    framework,

  - yellow is _only_ used for known breakages, and

  - green is _only_ used for things which have gone to plan and
    require no further work to be done.

Sounds reasonable, and I think the new output looks nice. I notice that
skipped tests are still in green. I wonder if they should be in yellow,
too. They may or may not be a problem, but you are failing to run some
portion of the test suite.

This hunk is surprising after reading the commit message. It looks like
you are also breaking down expect_fail tests by fixed and not fixed.

I think that is probably an OK thing to do (although it might make more
sense in a separate patch), but are your numbers right?  It looks from
that count as if there are 2 tests expecting failure (one fixed and one
still broken).

-Peff

On Mon, Sep 17, 2012 at 04:11:19PM -0400, Jeff King wrote:

On Mon, Sep 17, 2012 at 12:50:37PM +0100, Adam Spiers wrote:

quoted

The end result of these changes is that:

  - red is _only_ used for things which have gone unexpectedly wrong:
    test failures, unexpected test passes, and failures with the
    framework,

  - yellow is _only_ used for known breakages, and

  - green is _only_ used for things which have gone to plan and
    require no further work to be done.

Sounds reasonable, and I think the new output looks nice. I notice that
skipped tests are still in green. I wonder if they should be in yellow,
too. They may or may not be a problem, but you are failing to run some
portion of the test suite.

Fair point, I'll reroll the series and change skipped tests to yellow
(non-bold, to distinguish from known breakages which are bold yellow).

quoted

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index ae6a3f0..4e111b4 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh

@@ -81,9 +81,10 @@ test_expect_success 'pretend we have fixed a known breakage (run in sub test-lib
 	./passing-todo.sh >out 2>err &&
 	! test -s err &&
 	sed -e 's/^> //' >expect <<-\\EOF &&
-	> ok 1 - pretend we have fixed a known breakage # TODO known breakage
-	> # fixed 1 known breakage(s)
-	> # passed all 1 test(s)
+	> ok 1 - pretend we have fixed a known breakage # TODO known breakage vanished
+	> # fixed 1 known breakage(s); please update test(s)
+	> # still have 1 known breakage(s)
+	> # passed all remaining 0 test(s)
 	> 1..1
 	EOF
 	test_cmp expect out)

This hunk is surprising after reading the commit message. It looks like
you are also breaking down expect_fail tests by fixed and not fixed.

Correct.

I think that is probably an OK thing to do (although it might make more
sense in a separate patch), but are your numbers right?

They are right (at least as I intended), but I agree it's a bit
confusing.

It looks from that count as if there are 2 tests expecting failure
(one fixed and one still broken).

It's actually one test which is both fixed *and* in some sense broken.
The confusion arises from the ambiguity of the word "broken", which
could mean either "failed as expected" or "expected to fail but
didn't".  Previously it was just the former, but my patch changed it
to encompass both cases.  The motivation behind this was to avoid the

    # passed all $count test(s)

summary message which is overly comforting when one or more tests were
expected to fail but didn't.  However perhaps it's cleaner to keep the
counter buckets separated.  I'll try to come up with a better
solution.

On 09/17/2012 10:11 PM, Jeff King wrote:

On Mon, Sep 17, 2012 at 12:50:37PM +0100, Adam Spiers wrote:

quoted

The end result of these changes is that:

  - red is _only_ used for things which have gone unexpectedly wrong:
    test failures, unexpected test passes, and failures with the
    framework,

  - yellow is _only_ used for known breakages, and

  - green is _only_ used for things which have gone to plan and
    require no further work to be done.

Sounds reasonable, and I think the new output looks nice. I notice that
skipped tests are still in green. I wonder if they should be in yellow,
too.

What about blue instead?   This would keep the colouring scheme more
consistent with the one used by prove:
  <http://search.cpan.org/~ovid/Test-Harness/bin/prove>
by autotest:
  <http://www.gnu.org/software/autoconf/manual/autoconf.html#Using-Autotest>
and by the Automake-generated test harness:
  <http://www.gnu.org/software/automake/manual/automake.html#Scripts_002dbased-Testsuites>

Just my 2 cents,
  Stefano

On Wed, Sep 19, 2012 at 10:02:52PM +0200, Stefano Lattarini wrote:

On 09/17/2012 10:11 PM, Jeff King wrote:

quoted

On Mon, Sep 17, 2012 at 12:50:37PM +0100, Adam Spiers wrote:

quoted

The end result of these changes is that:

  - red is _only_ used for things which have gone unexpectedly wrong:
    test failures, unexpected test passes, and failures with the
    framework,

  - yellow is _only_ used for known breakages, and

  - green is _only_ used for things which have gone to plan and
    require no further work to be done.

Sounds reasonable, and I think the new output looks nice. I notice that
skipped tests are still in green. I wonder if they should be in yellow,
too.

What about blue instead?   This would keep the colouring scheme more
consistent with the one used by prove:
  <http://search.cpan.org/~ovid/Test-Harness/bin/prove>
by autotest:
  <http://www.gnu.org/software/autoconf/manual/autoconf.html#Using-Autotest>
and by the Automake-generated test harness:
  <http://www.gnu.org/software/automake/manual/automake.html#Scripts_002dbased-Testsuites>

Sounds good to me!  Blue is the conventional color for informational
signs, so seems like a natural fit for skipped tests.  Not sure
whether it should be bold or not though?  I'll wait for a more solid
group consensus before re-rolling yet another patch :-)

On Wed, Sep 19, 2012 at 09:12:01PM +0100, Adam Spiers wrote:

quoted

quoted

Sounds reasonable, and I think the new output looks nice. I notice that
skipped tests are still in green. I wonder if they should be in yellow,
too.

What about blue instead?   This would keep the colouring scheme more
consistent with the one used by prove:
  <http://search.cpan.org/~ovid/Test-Harness/bin/prove>
by autotest:
  <http://www.gnu.org/software/autoconf/manual/autoconf.html#Using-Autotest>
and by the Automake-generated test harness:
  <http://www.gnu.org/software/automake/manual/automake.html#Scripts_002dbased-Testsuites>

Sounds good to me!  Blue is the conventional color for informational
signs, so seems like a natural fit for skipped tests.  Not sure
whether it should be bold or not though?  I'll wait for a more solid
group consensus before re-rolling yet another patch :-)

Blue would be fine with me. No strong opinion on bold or not (my gut is
that most things should not be bold unless there is a good reason, but
that is just a feeling).

-Peff

Skipped tests indicate incomplete test coverage.  Whilst this is
not a test failure or other error, it's still not complete
success, so according to the universal traffic lights coloring
scheme, yellow/brown seems more suitable than green.  However,
it's more informational than cautionary, so instead we use blue
which is a universal color for information signs.

Signed-off-by: Adam Spiers <redacted>
---
 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5293830..78c88c2 100755
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -182,13 +182,13 @@ then
 		error)
 			tput bold; tput setaf 1;; # bold red
 		skip)
-			tput bold; tput setaf 2;; # bold green
+			tput setaf 4;;            # blue
 		warn)
 			tput bold; tput setaf 3;; # bold yellow
 		pass)
 			tput setaf 2;;            # green
 		info)
-			tput setaf 3;;            # brown
+			tput setaf 3;;            # yellow/brown
 		*)
 			test -n "$quiet" && return;;
 		esac

-- 
1.7.12.147.g6d168f4

Am 9/19/2012 22:24, schrieb Adam Spiers:

 		skip)
-			tput bold; tput setaf 2;; # bold green
+			tput setaf 4;;            # blue

It's unreadable on black background. Keep it bold; that works on both
black and white background.

-- Hannes

On Thu, Sep 20, 2012 at 6:48 AM, Johannes Sixt [off-list ref] wrote:

Am 9/19/2012 22:24, schrieb Adam Spiers:

quoted

              skip)
-                     tput bold; tput setaf 2;; # bold green
+                     tput setaf 4;;            # blue

It's unreadable on black background. Keep it bold; that works on both
black and white background.

Whilst my preference aligns with yours in this particular case, we are
now on a slippery slope, since these days terminal colors are
infinitely configurable, and some heretics even choose backgrounds
which are neither black nor white ;-)  I don't want to trigger a long
discussion or end up spamming the list with lots of different color
scheme patches in an attempt to please everyone.  So bold blue will be
my last version of this patch.

Skipped tests indicate incomplete test coverage.  Whilst this is
not a test failure or other error, it's still not complete
success, so according to the universal traffic lights coloring
scheme, yellow/brown seems more suitable than green.  However,
it's more informational than cautionary, so instead we use blue
which is a universal color for information signs.  Bold blue
should work better on both black and white backgrounds than
normal blue.

Signed-off-by: Adam Spiers <redacted>
---
 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5293830..5ef87d4 100755
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -182,13 +182,13 @@ then
 		error)
 			tput bold; tput setaf 1;; # bold red
 		skip)
-			tput bold; tput setaf 2;; # bold green
+			tput bold; tput setaf 4;; # bold blue
 		warn)
 			tput bold; tput setaf 3;; # bold yellow
 		pass)
 			tput setaf 2;;            # green
 		info)
-			tput setaf 3;;            # brown
+			tput setaf 3;;            # yellow/brown
 		*)
 			test -n "$quiet" && return;;
 		esac

-- 
1.7.12.147.g6d168f4

Hi Adam.

On 09/20/2012 11:08 AM, Adam Spiers wrote:

Skipped tests indicate incomplete test coverage.  Whilst this is
not a test failure or other error, it's still not complete
success, so according to the universal traffic lights coloring
scheme, yellow/brown seems more suitable than green.  However,
it's more informational than cautionary, so instead we use blue
which is a universal color for information signs.  Bold blue
should work better on both black and white backgrounds than
normal blue.

A very minor nit (feel free to ignore it): IMHO, it should be nice
to state explicitly in the commit message that blue is already used
by other testsuite-related software to highlight skipped tests; you
can report the examples of at least Automake, Autotest and prove --
and extra kudos if you find further examples ;-)

Thanks,
  Stefano

On Wed, Sep 19, 2012 at 09:24:23PM +0100, Adam Spiers wrote:

I happened to be running a test script with "-v" earlier today, and I
noticed that the "expecting success..." dump of the test contents is
also yellow. By your new rules, shouldn't it be blue?

I think it is matching the "info" type, which from the discussion should
be blue, no?

Maybe it is just my terminal. I see it is labeled as "brown" here, but
it looks very yellow (and I am using the stock xterm colors. According
to:

  https://en.wikipedia.org/wiki/ANSI_colors

It looks it really is brown on some platforms. I'm not sure if it is
worth worrying about.  I don't really want to get into configurable
colors just for the test-suite output.

-Peff

On Fri, Sep 21, 2012 at 02:13:25AM -0400, Jeff King wrote:

On Wed, Sep 19, 2012 at 09:24:23PM +0100, Adam Spiers wrote:

quoted

 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5293830..78c88c2 100755
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -182,13 +182,13 @@ then
 		error)
 			tput bold; tput setaf 1;; # bold red
 		skip)
-			tput bold; tput setaf 2;; # bold green
+			tput setaf 4;;            # blue
 		warn)
 			tput bold; tput setaf 3;; # bold yellow
 		pass)
 			tput setaf 2;;            # green
 		info)
-			tput setaf 3;;            # brown
+			tput setaf 3;;            # yellow/brown

I happened to be running a test script with "-v" earlier today, and I
noticed that the "expecting success..." dump of the test contents is
also yellow. By your new rules, shouldn't it be blue?

I think it is matching the "info" type, which from the discussion should
be blue, no?

It uses the "default" colour:

    say >&3 "expecting success: $2"

where say is defined:

    say () {
            say_color info "$*"
    }

Many other messages are output in this default colour too, and I never
proposed to change it.  The only time in the discussion where blue was
associated with "info" was in this sentence I wrote in the
commit message for the patch altering the colour of "skip" messages:

   "However, it's more informational than cautionary, so instead we
    use blue which is a universal color for information signs."

Whilst it could also be applied to "info", I don't think it would be a
good idea to have the "skip" and "info" colours *both* as bold blue.
It seems to me more important that the "skip" messages should visually
stand out more than "info", since they are rarer and a more notable
level of information than the latter (especially if --verbose is
used).  Additionally, yellow is already somewhat overloaded (yellow
for "info" and bold yellow for "warn").  Therefore I would suggest
changing "info" to perhaps bold white or bold cyan.  Or "skip" could
be magenta and "info" blue.  But now we are heading down a slippery
slope; it'll be near impossible to please everyone.  Any final
thoughts?

Maybe it is just my terminal. I see it is labeled as "brown" here, but
it looks very yellow (and I am using the stock xterm colors. According
to:

  https://en.wikipedia.org/wiki/ANSI_colors

It looks it really is brown on some platforms.

Yes, it can be.

I'm not sure if it is
worth worrying about.  I don't really want to get into configurable
colors just for the test-suite output.

Agreed.  There is no indisputably correct combination.  However, I
think that, modulo a tweak for the above, we are definitely in the
right ball park.  The main thing is that the traffic light colour
scheme is adhered to, and that different types of message are clearly
visually separated, with more important ones standing out more than
less important ones.