[PATCH] Make test output coloring more intuitive

[PATCH] Make test output coloring more intuitive

[Adam Spiers]

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 <git@adamspiers.org>
---
 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

Re: [PATCH] Make test output coloring more intuitive

[Jeff King]

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.

> 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.

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

Re: [PATCH] Make test output coloring more intuitive

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> 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.

I agree with Peff's comments.

The point #3 above wants to be a separate patch; we may even want to
consider a follow-up change to add an option to make a "test that is
expected to fail did not fail" case a 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"
>  }

Also I wonder if this is still a "TODO".  "# TODO fixed known breakage",
meaning that it is something that must be looked at by whoever happened
to have fixed the known breakage by accident, might be a better wording.

Re: [PATCH] Make test output coloring more intuitive

[Adam Spiers]

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:
> 
> > 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).

> > 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.

Re: [PATCH] Make test output coloring more intuitive

[Adam Spiers]

On Mon, Sep 17, 2012 at 01:50:39PM -0700, Junio C Hamano wrote:
> Adam Spiers <git@adamspiers.org> writes:
> 
> > 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.
> 
> I agree with Peff's comments.
> 
> The point #3 above wants to be a separate patch;

OK, re-rolling this.

> we may even want to consider a follow-up change to add an option to
> make a "test that is expected to fail did not fail" case a failure.

Sounds like a nice idea.

> >  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"
> >  }
> 
> Also I wonder if this is still a "TODO".

Hah, I should trust my first instincts more; my first version of the
patch dropped the "TODO", but then I put it back in because I thought
people would object :-)

> "# TODO fixed known breakage", meaning that it is something that
> must be looked at by whoever happened to have fixed the known
> breakage by accident, might be a better wording.

I would challenge the assumption that the test passed because someone
deliberately fixed the code it was testing.  Whilst this is a probable
scenario (e.g. if they forgot to adjust the test to expect success
rather than failure), it's not the only one.  Tests can be brittle and
depend on external factors which can change unexpectedly.  For
example, I noticed that the final 'general options plus command' test
in t9902-completion.sh is currently dependent on the contents of the
current $PATH.  This is because I have a shell-script called
`git-check-email' on mine, so `check-email' becomes an extra
subcommand completion possibility for the prefix `check'.  Almost
every other developer running this test would not encounter the same
failure.  It's not a huge stretch to imagine a similar corner case
which could unexpectedly cause a test to pass when failure was
expected.

Therefore I would suggest a wording which avoids implying that
something was deliberately fixed.  That was my reasoning behind
introducing the word "vanished" to the output.

Re: [PATCH] Make test output coloring more intuitive

[Jeff King]

On Tue, Sep 18, 2012 at 10:36:17PM +0100, Adam Spiers wrote:

> > > +	say_color error "ok $test_count - $@ # TODO known breakage vanished"
> > >  }
> > 
> > Also I wonder if this is still a "TODO".
> 
> Hah, I should trust my first instincts more; my first version of the
> patch dropped the "TODO", but then I put it back in because I thought
> people would object :-)

TODO is a special token[1] respected by TAP harnesses like "prove". I'm
not sure what practical impact it has, but it should probably remain.

-Peff

[1] http://testanything.org/wiki/index.php/TAP_specification#TODO_tests

Re: [PATCH] Make test output coloring more intuitive

[Adam Spiers]

On Tue, Sep 18, 2012 at 10:59 PM, Jeff King <peff@peff.net> wrote:
> TODO is a special token[1] respected by TAP harnesses like "prove". I'm
> not sure what practical impact it has, but it should probably remain.
>
> -Peff
>
> [1] http://testanything.org/wiki/index.php/TAP_specification#TODO_tests

Thanks, I didn't know that.  For reasons I just stated elsewhere in
this thread, I don't _completely_ agree with the specification where
it says:

    "Should a todo test point begin succeeding, the harness should
    report it as a bonus.  This indicates that whatever you were
    supposed to do has been done and you should promote this to a
    normal test point."

However those semantics are well-defined enough to warrant keeping the
"TODO" as you suggest.

[PATCH v2 0/6] make test output coloring more intuitive

[Adam Spiers]

This series of commits attempts to make test output coloring
more intuitive, so 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 skipped tests, and

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

Adam Spiers (6):
  Change the color of individual known breakages
  Make 'not ok $count - $message' consistent with 'ok $count -
    $message'
  Color skipped tests the same as informational messages
  Refactor mechanics of testing in a sub test-lib
  Test the test framework more thoroughly
  Treat unexpectedly fixed known breakages more seriously

 t/t0000-basic.sh | 179 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 t/test-lib.sh    |  25 +++++---
 2 files changed, 174 insertions(+), 30 deletions(-)
 mode change 100644 => 100755 t/test-lib.sh

-- 
1.7.12.147.g6d168f4

[PATCH v2 1/6] Change the color of individual known breakages

[Adam Spiers]

Bold yellow seems a more appropriate color than bold green 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.

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.

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f8e3733..426820e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -183,6 +183,8 @@ 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)
@@ -281,7 +283,7 @@ test_known_broken_ok_ () {
 
 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 () {
@@ -375,7 +377,7 @@ test_done () {
 	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

[PATCH v2 2/6] Make 'not ok $count - $message' consistent with 'ok $count - $message'

[Adam Spiers]

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 t/t0000-basic.sh | 4 ++--
 t/test-lib.sh    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
 mode change 100644 => 100755 t/test-lib.sh

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index ae6a3f0..c6b42de 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -167,13 +167,13 @@ test_expect_success 'tests clean up even on failures' "
 	! test -s err &&
 	! test -f \"trash directory.failing-cleanup/clean-after-failure\" &&
 	sed -e 's/Z$//' -e 's/^> //' >expect <<-\\EOF &&
-	> not ok - 1 tests clean up even after a failure
+	> not ok 1 - tests clean up even after a failure
 	> #	Z
 	> #	touch clean-after-failure &&
 	> #	test_when_finished rm clean-after-failure &&
 	> #	(exit 1)
 	> #	Z
-	> not ok - 2 failure to clean up causes the test to fail
+	> not ok 2 - failure to clean up causes the test to fail
 	> #	Z
 	> #	test_when_finished \"(exit 2)\"
 	> #	Z
diff --git a/t/test-lib.sh b/t/test-lib.sh
old mode 100644
new mode 100755
index 426820e..5293830
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -270,7 +270,7 @@ test_ok_ () {
 
 test_failure_ () {
 	test_failure=$(($test_failure + 1))
-	say_color error "not ok - $test_count $1"
+	say_color error "not ok $test_count - $1"
 	shift
 	echo "$@" | sed -e 's/^/#	/'
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
-- 
1.7.12.147.g6d168f4

[PATCH v2 3/6] Color skipped tests the same as informational messages

[Adam Spiers]

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 we leave it non-bold
in order to contrast with warnings.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 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..7028ba8 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 3;;            # yellow/brown
 		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

[PATCH v2 4/6] Refactor mechanics of testing in a sub test-lib

[Adam Spiers]

This will allow us to test the test framework more thoroughly
without disrupting the top-level test metrics.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 t/t0000-basic.sh | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index c6b42de..662cd2f 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -55,39 +55,49 @@ test_expect_failure 'pretend we have a known breakage' '
 	false
 '
 
-test_expect_success 'pretend we have fixed a known breakage (run in sub test-lib)' "
-	mkdir passing-todo &&
-	(cd passing-todo &&
-	cat >passing-todo.sh <<-EOF &&
+run_sub_test_lib_test () {
+	name="$1" descr="$2" # stdin is body of test code
+	mkdir $name &&
+	(cd $name &&
+	cat >$name.sh <<-EOF &&
 	#!$SHELL_PATH
 
-	test_description='A passing TODO test
+	test_description='$descr (run in sub test-lib)
 
 	This is run in a sub test-lib so that we do not get incorrect
 	passing metrics
 	'
 
 	# Point to the t/test-lib.sh, which isn't in ../ as usual
-	TEST_DIRECTORY=\"$TEST_DIRECTORY\"
-	. \"\$TEST_DIRECTORY\"/test-lib.sh
-
-	test_expect_failure 'pretend we have fixed a known breakage' '
-		:
-	'
+	TEST_DIRECTORY="$TEST_DIRECTORY"
+	. "\$TEST_DIRECTORY"/test-lib.sh
+	EOF
+	cat >>$name.sh &&
+	chmod +x $name.sh &&
+	./$name.sh >out 2>err)
+}
+
+check_sub_test_lib_test () {
+	name="$1" # stdin is test's expected stdout
+	(cd $name &&
+	! test -s err &&
+	sed -e 's/^> //' >expect &&
+	test_cmp expect out)
+}
 
+test_expect_success 'pretend we have fixed a known breakage' "
+	run_sub_test_lib_test passing-todo 'A passing TODO test' <<-EOF &&
+	test_expect_failure 'pretend we have fixed a known breakage' 'true'
 	test_done
 	EOF
-	chmod +x passing-todo.sh &&
-	./passing-todo.sh >out 2>err &&
-	! test -s err &&
-	sed -e 's/^> //' >expect <<-\\EOF &&
+	check_sub_test_lib_test passing-todo <<-EOF
 	> ok 1 - pretend we have fixed a known breakage # TODO known breakage
 	> # fixed 1 known breakage(s)
 	> # passed all 1 test(s)
 	> 1..1
 	EOF
-	test_cmp expect out)
 "
+
 test_set_prereq HAVEIT
 haveit=no
 test_expect_success HAVEIT 'test runs if prerequisite is satisfied' '
@@ -166,7 +176,7 @@ test_expect_success 'tests clean up even on failures' "
 	test_must_fail ./failing-cleanup.sh >out 2>err &&
 	! test -s err &&
 	! test -f \"trash directory.failing-cleanup/clean-after-failure\" &&
-	sed -e 's/Z$//' -e 's/^> //' >expect <<-\\EOF &&
+	sed -e 's/Z$//' -e 's/^> //' >expect <<-EOF &&
 	> not ok 1 - tests clean up even after a failure
 	> #	Z
 	> #	touch clean-after-failure &&
-- 
1.7.12.147.g6d168f4

[PATCH v2 5/6] Test the test framework more thoroughly

[Adam Spiers]

Add 5 new full test suite runs each with a different number of
passing/failing/broken/fixed tests, in order to ensure that the
correct exit code and output are generated in each case.  As before,
these are run in a subdirectory in order to disrupt the metrics for
the parent tests.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 t/t0000-basic.sh | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 662cd2f..644cc2c 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -77,6 +77,15 @@ run_sub_test_lib_test () {
 	./$name.sh >out 2>err)
 }
 
+run_sub_test_lib_test_expecting_failures () {
+	if run_sub_test_lib_test "$@"; then
+		echo 'sub test-lib run should have failed'
+		return 1
+	else
+		return 0
+	fi
+}
+
 check_sub_test_lib_test () {
 	name="$1" # stdin is test's expected stdout
 	(cd $name &&
@@ -85,6 +94,54 @@ check_sub_test_lib_test () {
 	test_cmp expect out)
 }
 
+test_expect_success 'pretend we have a fully passing test suite' "
+	run_sub_test_lib_test full-pass '3 passing tests' <<-EOF &&
+	for i in 1 2 3; do
+		test_expect_success \"passing test #\\\$i\" 'true'
+	done
+	test_done
+	EOF
+	check_sub_test_lib_test full-pass <<-EOF
+	> ok 1 - passing test #1
+	> ok 2 - passing test #2
+	> ok 3 - passing test #3
+	> # passed all 3 test(s)
+	> 1..3
+	EOF
+"
+
+test_expect_success 'pretend we have a partially passing test suite' "
+	run_sub_test_lib_test_expecting_failures partial-pass '2/3 tests passing' <<-EOF &&
+	test_expect_success 'passing test #1' 'true'
+	test_expect_success 'failing test #2' 'false'
+	test_expect_success 'passing test #3' 'true'
+	test_done
+	EOF
+	check_sub_test_lib_test partial-pass <<-EOF
+	> ok 1 - passing test #1
+	> not ok 2 - failing test #2
+	#	false
+	> ok 3 - passing test #3
+	> # failed 1 among 3 test(s)
+	> 1..3
+	EOF
+"
+
+test_expect_success 'pretend we have a known breakage' "
+	run_sub_test_lib_test failing-todo 'A failing TODO test' <<-EOF &&
+	test_expect_success 'passing test' 'true'
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_done
+	EOF
+	check_sub_test_lib_test failing-todo <<-EOF
+	> ok 1 - passing test
+	> not ok 2 - pretend we have a known breakage # TODO known breakage
+	> # still have 1 known breakage(s)
+	> # passed all remaining 1 test(s)
+	> 1..2
+	EOF
+"
+
 test_expect_success 'pretend we have fixed a known breakage' "
 	run_sub_test_lib_test passing-todo 'A passing TODO test' <<-EOF &&
 	test_expect_failure 'pretend we have fixed a known breakage' 'true'
@@ -98,6 +155,59 @@ test_expect_success 'pretend we have fixed a known breakage' "
 	EOF
 "
 
+test_expect_success 'pretend we have a pass, fail, and known breakage' "
+	run_sub_test_lib_test_expecting_failures mixed-results1 'mixed results #1' <<-EOF &&
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'failing test' 'false'
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_done
+	EOF
+	check_sub_test_lib_test mixed-results1 <<-EOF
+	> ok 1 - passing test
+	> not ok 2 - failing test
+	> #	false
+	> not ok 3 - pretend we have a known breakage # TODO known breakage
+	> # still have 1 known breakage(s)
+	> # failed 1 among remaining 2 test(s)
+	> 1..3
+	EOF
+"
+
+test_expect_success 'pretend we have a mix of all possible results' "
+	run_sub_test_lib_test_expecting_failures mixed-results2 'mixed results #2' <<-EOF &&
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'failing test' 'false'
+	test_expect_success 'failing test' 'false'
+	test_expect_success 'failing test' 'false'
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_expect_failure 'pretend we have fixed a known breakage' 'true'
+	test_done
+	EOF
+	check_sub_test_lib_test mixed-results2 <<-EOF
+	> ok 1 - passing test
+	> ok 2 - passing test
+	> ok 3 - passing test
+	> ok 4 - passing test
+	> not ok 5 - failing test
+	> #	false
+	> not ok 6 - failing test
+	> #	false
+	> not ok 7 - failing test
+	> #	false
+	> not ok 8 - pretend we have a known breakage # TODO known breakage
+	> not ok 9 - pretend we have a known breakage # TODO known breakage
+	> ok 10 - pretend we have fixed a known breakage # TODO known breakage
+	> # fixed 1 known breakage(s)
+	> # still have 2 known breakage(s)
+	> # failed 3 among remaining 8 test(s)
+	> 1..10
+	EOF
+"
+
 test_set_prereq HAVEIT
 haveit=no
 test_expect_success HAVEIT 'test runs if prerequisite is satisfied' '
-- 
1.7.12.147.g6d168f4

[PATCH v2 6/6] Treat unexpectedly fixed known breakages more seriously

[Adam Spiers]

Change color of unexpectedly fixed known breakages to bold red.  An
unexpectedly passing test indicates that the test code is somehow
broken or out of sync with the code it is testing.  Either way this
is an error which is potentially as bad as a failing test, and as
such is no longer portrayed as a pass in the output.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 t/t0000-basic.sh | 29 +++++++++++++++++++++++------
 t/test-lib.sh    | 13 +++++++++----
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 644cc2c..459d0c7 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -148,13 +148,30 @@ test_expect_success 'pretend we have fixed a known breakage' "
 	test_done
 	EOF
 	check_sub_test_lib_test passing-todo <<-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
+	> # 1 known breakage(s) vanished; please update test(s)
 	> 1..1
 	EOF
 "
 
+test_expect_success 'pretend we have fixed one of two known breakages (run in sub test-lib)' "
+	run_sub_test_lib_test partially-passing-todos '2 TODO tests, one passing' <<-EOF &&
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_expect_success 'pretend we have a passing test' 'true'
+	test_expect_failure 'pretend we have fixed another known breakage' 'true'
+	test_done
+	EOF
+	check_sub_test_lib_test partially-passing-todos <<-EOF
+	> not ok 1 - pretend we have a known breakage # TODO known breakage
+	> ok 2 - pretend we have a passing test
+	> ok 3 - pretend we have fixed another known breakage # TODO known breakage vanished
+	> # 1 known breakage(s) vanished; please update test(s)
+	> # still have 1 known breakage(s)
+	> # passed all remaining 1 test(s)
+	> 1..3
+	EOF
+"
+
 test_expect_success 'pretend we have a pass, fail, and known breakage' "
 	run_sub_test_lib_test_expecting_failures mixed-results1 'mixed results #1' <<-EOF &&
 	test_expect_success 'passing test' 'true'
@@ -200,10 +217,10 @@ test_expect_success 'pretend we have a mix of all possible results' "
 	> #	false
 	> not ok 8 - pretend we have a known breakage # TODO known breakage
 	> not ok 9 - pretend we have a known breakage # TODO known breakage
-	> ok 10 - pretend we have fixed a known breakage # TODO known breakage
-	> # fixed 1 known breakage(s)
+	> ok 10 - pretend we have fixed a known breakage # TODO known breakage vanished
+	> # 1 known breakage(s) vanished; please update test(s)
 	> # still have 2 known breakage(s)
-	> # failed 3 among remaining 8 test(s)
+	> # failed 3 among remaining 7 test(s)
 	> 1..10
 	EOF
 "
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7028ba8..b403e85 100755
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -278,7 +278,7 @@ test_failure_ () {
 
 test_known_broken_ok_ () {
 	test_fixed=$(($test_fixed+1))
-	say_color "" "ok $test_count - $@ # TODO known breakage"
+	say_color error "ok $test_count - $@ # TODO known breakage vanished"
 }
 
 test_known_broken_failure_ () {
@@ -373,13 +373,18 @@ test_done () {
 
 	if test "$test_fixed" != 0
 	then
-		say_color pass "# fixed $test_fixed known breakage(s)"
+		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
 	fi
 	if test "$test_broken" != 0
 	then
 		say_color warn "# still have $test_broken known breakage(s)"
-		msg="remaining $(($test_count-$test_broken)) test(s)"
+	fi
+	if test "$test_broken" != 0 || test "$test_fixed" != 0
+	then
+		test_remaining=$(( $test_count - $test_broken - $test_fixed ))
+		msg="remaining $test_remaining test(s)"
 	else
+		test_remaining=$test_count
 		msg="$test_count test(s)"
 	fi
 	case "$test_failure" in
@@ -393,7 +398,7 @@ test_done () {
 
 		if test $test_external_has_tap -eq 0
 		then
-			if test $test_count -gt 0
+			if test $test_remaining -gt 0
 			then
 				say_color pass "# passed all $msg"
 			fi
-- 
1.7.12.147.g6d168f4

Re: [PATCH v2 2/6] Make 'not ok $count - $message' consistent with 'ok $count - $message'

[Jeff King]

On Wed, Sep 19, 2012 at 06:15:11PM +0100, Adam Spiers wrote:

>  test_failure_ () {
>  	test_failure=$(($test_failure + 1))
> -	say_color error "not ok - $test_count $1"
> +	say_color error "not ok $test_count - $1"

Interesting. I wondered what TAP had to say about this, and in fact we
were doing it wrong before. However, since the test numbers are optional
in TAP, a harness is supposed to keep its own counter when we fail to
provide a number.

So it happened to work, but this change in fact makes us more
TAP-compliant. Good.

-Peff

Re: [PATCH v2 4/6] Refactor mechanics of testing in a sub test-lib

[Jeff King]

On Wed, Sep 19, 2012 at 06:15:13PM +0100, Adam Spiers wrote:

> This will allow us to test the test framework more thoroughly
> without disrupting the top-level test metrics.

I see this is prep for the next patch, and the parts pulling out the
test-runs into functions make sense. But this hunk confuses me:

> @@ -166,7 +176,7 @@ test_expect_success 'tests clean up even on failures' "
>  	test_must_fail ./failing-cleanup.sh >out 2>err &&
>  	! test -s err &&
>  	! test -f \"trash directory.failing-cleanup/clean-after-failure\" &&
> -	sed -e 's/Z$//' -e 's/^> //' >expect <<-\\EOF &&
> +	sed -e 's/Z$//' -e 's/^> //' >expect <<-EOF &&
>  	> not ok 1 - tests clean up even after a failure
>  	> #	Z
>  	> #	touch clean-after-failure &&

Is it just that you are dropping the '\' in all of the here-docs because
they are not needed? I think our usual style is not to interpolate, and
to do so only when we explicitly want it, which can prevent accidental
errors due to missing quoting.

Also, why is this one not converted into a check_sub... invocation?

-Peff

Re: [PATCH v2 0/6] make test output coloring more intuitive

[Jeff King]

On Wed, Sep 19, 2012 at 06:15:09PM +0100, Adam Spiers wrote:

> This series of commits attempts to make test output coloring
> more intuitive, so 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 skipped tests, and
> 
>   - green is _only_ used for things which have gone to plan and
>     require no further work to be done.

Thanks, I like this much better than the original (and it's much easier
to review broken apart like this).

I raised a few minor questions in the refactoring patch, but other than
that (and assuming your answers are what I expect, I do not care enough
about them to block the series), it looks very good.

The new "a passing expect_failure is a breakage" is a good thing. When
it's unexpected, it will help call attention to it and let us figure out
early what changed. And when it is expected (because you are fixing the
breakage), it is an easy way to remind you to update the tests. :)

Thanks for working on it.

-Peff

Re: [PATCH v2 4/6] Refactor mechanics of testing in a sub test-lib

[Adam Spiers]

On Wed, Sep 19, 2012 at 01:56:55PM -0400, Jeff King wrote:
> On Wed, Sep 19, 2012 at 06:15:13PM +0100, Adam Spiers wrote:
> 
> > This will allow us to test the test framework more thoroughly
> > without disrupting the top-level test metrics.
> 
> I see this is prep for the next patch, and the parts pulling out the
> test-runs into functions make sense. But this hunk confuses me:
> 
> > @@ -166,7 +176,7 @@ test_expect_success 'tests clean up even on failures' "
> >  	test_must_fail ./failing-cleanup.sh >out 2>err &&
> >  	! test -s err &&
> >  	! test -f \"trash directory.failing-cleanup/clean-after-failure\" &&
> > -	sed -e 's/Z$//' -e 's/^> //' >expect <<-\\EOF &&
> > +	sed -e 's/Z$//' -e 's/^> //' >expect <<-EOF &&
> >  	> not ok 1 - tests clean up even after a failure
> >  	> #	Z
> >  	> #	touch clean-after-failure &&
> 
> Is it just that you are dropping the '\' in all of the here-docs because
> they are not needed?

Hmm, I think I previously misunderstood the point of the \\ due to
never seeing that syntax before (since my Perl background taught me to
write <<'EOF' instead).  I noticed that the tests all passed without
it, and mistakenly assumed it had become unnecessary due to the
refactoring.

> I think our usual style is not to interpolate, and
> to do so only when we explicitly want it, which can prevent accidental
> errors due to missing quoting.

Right, that makes sense.  I'd vote to put it back in then.

> Also, why is this one not converted into a check_sub... invocation?

Because it was much further down in that file so I didn't notice it
during the refactoring ;-)  I've also noticed I can use test_must_fail
instead of introducing run_sub_test_lib_test_expecting_failures.

So I'll have to re-roll 4--6 again.  Presumably I can just reply to
[PATCH v2 4/6] with modified v3 versions without having to resend
the first three in the series, which haven't changed.

[PATCH v3 4/6] Refactor mechanics of testing in a sub test-lib

[Adam Spiers]

This will allow us to test the test framework more thoroughly
without disrupting the top-level test metrics.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 t/t0000-basic.sh | 67 ++++++++++++++++++++++++--------------------------------
 1 file changed, 29 insertions(+), 38 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index c6b42de..029e3bd 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -55,39 +55,49 @@ test_expect_failure 'pretend we have a known breakage' '
 	false
 '
 
-test_expect_success 'pretend we have fixed a known breakage (run in sub test-lib)' "
-	mkdir passing-todo &&
-	(cd passing-todo &&
-	cat >passing-todo.sh <<-EOF &&
+run_sub_test_lib_test () {
+	name="$1" descr="$2" # stdin is body of test code
+	mkdir $name &&
+	(cd $name &&
+	cat >$name.sh <<-EOF &&
 	#!$SHELL_PATH
 
-	test_description='A passing TODO test
+	test_description='$descr (run in sub test-lib)
 
 	This is run in a sub test-lib so that we do not get incorrect
 	passing metrics
 	'
 
 	# Point to the t/test-lib.sh, which isn't in ../ as usual
-	TEST_DIRECTORY=\"$TEST_DIRECTORY\"
-	. \"\$TEST_DIRECTORY\"/test-lib.sh
-
-	test_expect_failure 'pretend we have fixed a known breakage' '
-		:
-	'
+	TEST_DIRECTORY="$TEST_DIRECTORY"
+	. "\$TEST_DIRECTORY"/test-lib.sh
+	EOF
+	cat >>$name.sh &&
+	chmod +x $name.sh &&
+	./$name.sh >out 2>err)
+}
+
+check_sub_test_lib_test () {
+	name="$1" # stdin is test's expected stdout
+	(cd $name &&
+	! test -s err &&
+	sed -e 's/^> //' -e 's/Z$//' >expect &&
+	test_cmp expect out)
+}
 
+test_expect_success 'pretend we have fixed a known breakage' "
+	run_sub_test_lib_test passing-todo 'A passing TODO test' <<-\\EOF &&
+	test_expect_failure 'pretend we have fixed a known breakage' 'true'
 	test_done
 	EOF
-	chmod +x passing-todo.sh &&
-	./passing-todo.sh >out 2>err &&
-	! test -s err &&
-	sed -e 's/^> //' >expect <<-\\EOF &&
+	check_sub_test_lib_test passing-todo <<-\\EOF
 	> ok 1 - pretend we have fixed a known breakage # TODO known breakage
 	> # fixed 1 known breakage(s)
 	> # passed all 1 test(s)
 	> 1..1
 	EOF
-	test_cmp expect out)
 "
+
 test_set_prereq HAVEIT
 haveit=no
 test_expect_success HAVEIT 'test runs if prerequisite is satisfied' '
@@ -137,19 +147,8 @@ then
 fi
 
 test_expect_success 'tests clean up even on failures' "
-	mkdir failing-cleanup &&
-	(
-	cd failing-cleanup &&
-
-	cat >failing-cleanup.sh <<-EOF &&
-	#!$SHELL_PATH
-
-	test_description='Failing tests with cleanup commands'
-
-	# Point to the t/test-lib.sh, which isn't in ../ as usual
-	TEST_DIRECTORY=\"$TEST_DIRECTORY\"
-	. \"\$TEST_DIRECTORY\"/test-lib.sh
-
+	test_must_fail run_sub_test_lib_test \
+		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
 	test_expect_success 'tests clean up even after a failure' '
 		touch clean-after-failure &&
 		test_when_finished rm clean-after-failure &&
@@ -159,14 +158,8 @@ test_expect_success 'tests clean up even on failures' "
 		test_when_finished \"(exit 2)\"
 	'
 	test_done
-
 	EOF
-
-	chmod +x failing-cleanup.sh &&
-	test_must_fail ./failing-cleanup.sh >out 2>err &&
-	! test -s err &&
-	! test -f \"trash directory.failing-cleanup/clean-after-failure\" &&
-	sed -e 's/Z$//' -e 's/^> //' >expect <<-\\EOF &&
+	check_sub_test_lib_test failing-cleanup <<-\\EOF
 	> not ok 1 - tests clean up even after a failure
 	> #	Z
 	> #	touch clean-after-failure &&
@@ -180,8 +173,6 @@ test_expect_success 'tests clean up even on failures' "
 	> # failed 2 among 2 test(s)
 	> 1..2
 	EOF
-	test_cmp expect out
-	)
 "
 
 ################################################################
-- 
1.7.12.147.g6d168f4

[PATCH v3 5/6] Test the test framework more thoroughly

[Adam Spiers]

Add 5 new full test suite runs each with a different number of
passing/failing/broken/fixed tests, in order to ensure that the
correct exit code and output are generated in each case.  As before,
these are run in a subdirectory in order to disrupt the metrics for
the parent tests.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 t/t0000-basic.sh | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 029e3bd..65f578f 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -85,6 +85,55 @@ check_sub_test_lib_test () {
 	test_cmp expect out)
 }
 
+test_expect_success 'pretend we have a fully passing test suite' "
+	run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
+	for i in 1 2 3; do
+		test_expect_success \"passing test #\$i\" 'true'
+	done
+	test_done
+	EOF
+	check_sub_test_lib_test full-pass <<-\\EOF
+	> ok 1 - passing test #1
+	> ok 2 - passing test #2
+	> ok 3 - passing test #3
+	> # passed all 3 test(s)
+	> 1..3
+	EOF
+"
+
+test_expect_success 'pretend we have a partially passing test suite' "
+	test_must_fail run_sub_test_lib_test \
+		partial-pass '2/3 tests passing' <<-\\EOF &&
+	test_expect_success 'passing test #1' 'true'
+	test_expect_success 'failing test #2' 'false'
+	test_expect_success 'passing test #3' 'true'
+	test_done
+	EOF
+	check_sub_test_lib_test partial-pass <<-\\EOF
+	> ok 1 - passing test #1
+	> not ok 2 - failing test #2
+	#	false
+	> ok 3 - passing test #3
+	> # failed 1 among 3 test(s)
+	> 1..3
+	EOF
+"
+
+test_expect_success 'pretend we have a known breakage' "
+	run_sub_test_lib_test failing-todo 'A failing TODO test' <<-\\EOF &&
+	test_expect_success 'passing test' 'true'
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_done
+	EOF
+	check_sub_test_lib_test failing-todo <<-\\EOF
+	> ok 1 - passing test
+	> not ok 2 - pretend we have a known breakage # TODO known breakage
+	> # still have 1 known breakage(s)
+	> # passed all remaining 1 test(s)
+	> 1..2
+	EOF
+"
+
 test_expect_success 'pretend we have fixed a known breakage' "
 	run_sub_test_lib_test passing-todo 'A passing TODO test' <<-\\EOF &&
 	test_expect_failure 'pretend we have fixed a known breakage' 'true'
@@ -98,6 +147,61 @@ test_expect_success 'pretend we have fixed a known breakage' "
 	EOF
 "
 
+test_expect_success 'pretend we have a pass, fail, and known breakage' "
+	test_must_fail run_sub_test_lib_test \
+		mixed-results1 'mixed results #1' <<-\\EOF &&
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'failing test' 'false'
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_done
+	EOF
+	check_sub_test_lib_test mixed-results1 <<-\\EOF
+	> ok 1 - passing test
+	> not ok 2 - failing test
+	> #	false
+	> not ok 3 - pretend we have a known breakage # TODO known breakage
+	> # still have 1 known breakage(s)
+	> # failed 1 among remaining 2 test(s)
+	> 1..3
+	EOF
+"
+
+test_expect_success 'pretend we have a mix of all possible results' "
+	test_must_fail run_sub_test_lib_test \
+		mixed-results2 'mixed results #2' <<-\\EOF &&
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'passing test' 'true'
+	test_expect_success 'failing test' 'false'
+	test_expect_success 'failing test' 'false'
+	test_expect_success 'failing test' 'false'
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_expect_failure 'pretend we have fixed a known breakage' 'true'
+	test_done
+	EOF
+	check_sub_test_lib_test mixed-results2 <<-\\EOF
+	> ok 1 - passing test
+	> ok 2 - passing test
+	> ok 3 - passing test
+	> ok 4 - passing test
+	> not ok 5 - failing test
+	> #	false
+	> not ok 6 - failing test
+	> #	false
+	> not ok 7 - failing test
+	> #	false
+	> not ok 8 - pretend we have a known breakage # TODO known breakage
+	> not ok 9 - pretend we have a known breakage # TODO known breakage
+	> ok 10 - pretend we have fixed a known breakage # TODO known breakage
+	> # fixed 1 known breakage(s)
+	> # still have 2 known breakage(s)
+	> # failed 3 among remaining 8 test(s)
+	> 1..10
+	EOF
+"
+
 test_set_prereq HAVEIT
 haveit=no
 test_expect_success HAVEIT 'test runs if prerequisite is satisfied' '
-- 
1.7.12.147.g6d168f4

[PATCH v3 6/6] Treat unexpectedly fixed known breakages more seriously

[Adam Spiers]

Change color of unexpectedly fixed known breakages to bold red.  An
unexpectedly passing test indicates that the test code is somehow
broken or out of sync with the code it is testing.  Either way this
is an error which is potentially as bad as a failing test, and as
such is no longer portrayed as a pass in the output.

Signed-off-by: Adam Spiers <git@adamspiers.org>
---
 t/t0000-basic.sh | 30 ++++++++++++++++++++++++------
 t/test-lib.sh    | 13 +++++++++----
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 65f578f..ed44f7d 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -140,13 +140,31 @@ test_expect_success 'pretend we have fixed a known breakage' "
 	test_done
 	EOF
 	check_sub_test_lib_test passing-todo <<-\\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
+	> # 1 known breakage(s) vanished; please update test(s)
 	> 1..1
 	EOF
 "
 
+test_expect_success 'pretend we have fixed one of two known breakages (run in sub test-lib)' "
+	run_sub_test_lib_test partially-passing-todos \
+		'2 TODO tests, one passing' <<-\\EOF &&
+	test_expect_failure 'pretend we have a known breakage' 'false'
+	test_expect_success 'pretend we have a passing test' 'true'
+	test_expect_failure 'pretend we have fixed another known breakage' 'true'
+	test_done
+	EOF
+	check_sub_test_lib_test partially-passing-todos <<-\\EOF
+	> not ok 1 - pretend we have a known breakage # TODO known breakage
+	> ok 2 - pretend we have a passing test
+	> ok 3 - pretend we have fixed another known breakage # TODO known breakage vanished
+	> # 1 known breakage(s) vanished; please update test(s)
+	> # still have 1 known breakage(s)
+	> # passed all remaining 1 test(s)
+	> 1..3
+	EOF
+"
+
 test_expect_success 'pretend we have a pass, fail, and known breakage' "
 	test_must_fail run_sub_test_lib_test \
 		mixed-results1 'mixed results #1' <<-\\EOF &&
@@ -194,10 +212,10 @@ test_expect_success 'pretend we have a mix of all possible results' "
 	> #	false
 	> not ok 8 - pretend we have a known breakage # TODO known breakage
 	> not ok 9 - pretend we have a known breakage # TODO known breakage
-	> ok 10 - pretend we have fixed a known breakage # TODO known breakage
-	> # fixed 1 known breakage(s)
+	> ok 10 - pretend we have fixed a known breakage # TODO known breakage vanished
+	> # 1 known breakage(s) vanished; please update test(s)
 	> # still have 2 known breakage(s)
-	> # failed 3 among remaining 8 test(s)
+	> # failed 3 among remaining 7 test(s)
 	> 1..10
 	EOF
 "
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7028ba8..b403e85 100755
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -278,7 +278,7 @@ test_failure_ () {
 
 test_known_broken_ok_ () {
 	test_fixed=$(($test_fixed+1))
-	say_color "" "ok $test_count - $@ # TODO known breakage"
+	say_color error "ok $test_count - $@ # TODO known breakage vanished"
 }
 
 test_known_broken_failure_ () {
@@ -373,13 +373,18 @@ test_done () {
 
 	if test "$test_fixed" != 0
 	then
-		say_color pass "# fixed $test_fixed known breakage(s)"
+		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
 	fi
 	if test "$test_broken" != 0
 	then
 		say_color warn "# still have $test_broken known breakage(s)"
-		msg="remaining $(($test_count-$test_broken)) test(s)"
+	fi
+	if test "$test_broken" != 0 || test "$test_fixed" != 0
+	then
+		test_remaining=$(( $test_count - $test_broken - $test_fixed ))
+		msg="remaining $test_remaining test(s)"
 	else
+		test_remaining=$test_count
 		msg="$test_count test(s)"
 	fi
 	case "$test_failure" in
@@ -393,7 +398,7 @@ test_done () {
 
 		if test $test_external_has_tap -eq 0
 		then
-			if test $test_count -gt 0
+			if test $test_remaining -gt 0
 			then
 				say_color pass "# passed all $msg"
 			fi
-- 
1.7.12.147.g6d168f4

Re: [PATCH v2 4/6] Refactor mechanics of testing in a sub test-lib

[Jeff King]

On Wed, Sep 19, 2012 at 07:44:06PM +0100, Adam Spiers wrote:

> > Is it just that you are dropping the '\' in all of the here-docs because
> > they are not needed?
> 
> Hmm, I think I previously misunderstood the point of the \\ due to
> never seeing that syntax before (since my Perl background taught me to
> write <<'EOF' instead).  I noticed that the tests all passed without
> it, and mistakenly assumed it had become unnecessary due to the
> refactoring.

OK. You can write 'EOF' in the shell, too, but we tend not to in this
project (and you can write \EOF in perl, but I agree that it is much
less common in perl code I have seen).

Looking at it again, it is actually quite subtle what is going on. We
wrap the outer test_expect_* calls in double-quotes so that the inner
ones can use single-quotes easily. But that means that technically the
contents of the here-doc _are_ interpolated. But not at test run-time,
but rather at the call to test_expect_*. And that is why we nee to use
"\\" instead of "\". So I think anybody trying to tweak these tests
using shell metacharacters is in for a surprise either way. I'm not sure
it is worth worrying about, though, as handling it would probably make
the existing tests less readable.

> > Also, why is this one not converted into a check_sub... invocation?
> 
> Because it was much further down in that file so I didn't notice it
> during the refactoring ;-)

OK. :)

> I've also noticed I can use test_must_fail instead of introducing
> run_sub_test_lib_test_expecting_failures.

Good catch. I didn't notice that, but it definitely makes sense to reuse
it.

> 
> So I'll have to re-roll 4--6 again.  Presumably I can just reply to
> [PATCH v2 4/6] with modified v3 versions without having to resend
> the first three in the series, which haven't changed.

It all looks sane to me. Thanks again.

-Peff

Re: [PATCH] Make test output coloring more intuitive

[Stefano Lattarini]

On 09/17/2012 10:11 PM, Jeff King wrote:
> 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.
>
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

Re: [PATCH] Make test output coloring more intuitive

[Adam Spiers]

On Wed, Sep 19, 2012 at 10:02:52PM +0200, Stefano Lattarini wrote:
> On 09/17/2012 10:11 PM, Jeff King wrote:
> > 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.
> >
> 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 :-)

Re: [PATCH] Make test output coloring more intuitive

[Jeff King]

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

> > > 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

Re: [PATCH v2 4/6] Refactor mechanics of testing in a sub test-lib

[Adam Spiers]

On Wed, Sep 19, 2012 at 03:37:08PM -0400, Jeff King wrote:
> Looking at it again, it is actually quite subtle what is going on. We
> wrap the outer test_expect_* calls in double-quotes so that the inner
> ones can use single-quotes easily. But that means that technically the
> contents of the here-doc _are_ interpolated. But not at test run-time,
> but rather at the call to test_expect_*. And that is why we nee to use
> "\\" instead of "\". So I think anybody trying to tweak these tests
> using shell metacharacters is in for a surprise either way.

Actually I already did that in one place:

    test_expect_success 'pretend we have a fully passing test suite' "
            run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
            for i in 1 2 3; do
                    test_expect_success \"passing test #\$i\" 'true'
            done
            test_done
            EOF
    [...]
    "

Without the \\ preceeding the EOF, it needed to be:

                    test_expect_success \"passing test #\\\$i\" 'true'

> I'm not sure it is worth worrying about, though, as handling it
> would probably make the existing tests less readable.

Yeah, the for loop is perhaps slightly overkill :-)

> It all looks sane to me. Thanks again.

Thanks for your (unnervingly) thorough reviews ;-)

[PATCH v4 3/6] Color skipped tests blue

[Adam Spiers]

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 <git@adamspiers.org>
---
 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

Re: [PATCH v2 2/6] Make 'not ok $count - $message' consistent with 'ok $count - $message'

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  t/t0000-basic.sh | 4 ++--
>  t/test-lib.sh    | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

>  mode change 100644 => 100755 t/test-lib.sh

Peff might have already pointed out, but this is wrong.  Will queue
with an obvious tweak.

Thanks.

Re: [PATCH v2 2/6] Make 'not ok $count - $message' consistent with 'ok $count - $message'

[Adam Spiers]

On Thu, Sep 20, 2012 at 12:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
>> Signed-off-by: Adam Spiers <git@adamspiers.org>
>> ---
>>  t/t0000-basic.sh | 4 ++--
>>  t/test-lib.sh    | 2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>
>>  mode change 100644 => 100755 t/test-lib.sh
>
> Peff might have already pointed out, but this is wrong.  Will queue
> with an obvious tweak.

Oops, good catch.  That's a bug in my emacs config, I suspect.
Thanks for the tweak.  You guys have eagle eyes!

Re: [PATCH v4 3/6] Color skipped tests blue

[Johannes Sixt]

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

Re: [PATCH v4 3/6] Color skipped tests blue

[Adam Spiers]

On Thu, Sep 20, 2012 at 6:48 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> 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.

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.

[PATCH v5 3/3] Color skipped tests bold blue

[Adam Spiers]

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 <git@adamspiers.org>
---
 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

Re: [PATCH v5 3/3] Color skipped tests bold blue

[Stefano Lattarini]

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

Re: [PATCH v5 3/3] Color skipped tests bold blue

[Junio C Hamano]

Stefano Lattarini <stefano.lattarini@gmail.com> writes:

> 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.  Will tweak the proposed log message to include the above
when queuing the updated patch.

Re: [PATCH v3 4/6] Refactor mechanics of testing in a sub test-lib

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> This will allow us to test the test framework more thoroughly
> without disrupting the top-level test metrics.
>
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  t/t0000-basic.sh | 67 ++++++++++++++++++++++++--------------------------------
>  1 file changed, 29 insertions(+), 38 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index c6b42de..029e3bd 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -55,39 +55,49 @@ test_expect_failure 'pretend we have a known breakage' '
>  	false
>  '
>  
> -test_expect_success 'pretend we have fixed a known breakage (run in sub test-lib)' "
> -	mkdir passing-todo &&
> -	(cd passing-todo &&
> -	cat >passing-todo.sh <<-EOF &&
> +run_sub_test_lib_test () {
> +	name="$1" descr="$2" # stdin is body of test code
> +	mkdir $name &&
> +	(cd $name &&
> +	cat >$name.sh <<-EOF &&
>  	#!$SHELL_PATH
>  
> -	test_description='A passing TODO test
> +	test_description='$descr (run in sub test-lib)
>  
>  	This is run in a sub test-lib so that we do not get incorrect
>  	passing metrics
>  	'
>  
>  	# Point to the t/test-lib.sh, which isn't in ../ as usual
> -	TEST_DIRECTORY=\"$TEST_DIRECTORY\"
> -	. \"\$TEST_DIRECTORY\"/test-lib.sh
> -
> -	test_expect_failure 'pretend we have fixed a known breakage' '
> -		:
> -	'
> +	TEST_DIRECTORY="$TEST_DIRECTORY"
> +	. "\$TEST_DIRECTORY"/test-lib.sh
> +	EOF

The quoting of $TEST_DIRECTORY in the assignment does not look
correct (imagine a path with a double quote in it).

Removing the assignment and instead exporting TEST_DIRECTORY before
calling name.sh may be a reasonable fix, than trying to quotemeta
the value of $TEST_DIRECTORY here.

I'll re-queue this series in 'pu' with fixes and retitles; please
eyeball them before submitting a reroll.

  b465316 tests: paint unexpectedly fixed known breakages in bold red
  7214717 tests: test the test framework more thoroughly
  03c772a [SQUASH] t/t0000-basic.sh: quoting of TEST_DIRECTORY is screwed up
  99fe0af tests: refactor mechanics of testing in a sub test-lib
  6af90bf tests: paint skipped tests in bold blue
  0b87581 tests: test number comes first in 'not ok $count - $message'
  1c55079 tests: paint known breakages in bold yellow

The third one from the tip looks like the following, to re-indent to
make it readable and then minimally fix the quoting.

Thanks.

 t/t0000-basic.sh | 50 +++++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index ee78e68..c3345a9 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -56,33 +56,37 @@ test_expect_failure 'pretend we have a known breakage' '
 '
 
 run_sub_test_lib_test () {
-	name="$1" descr="$2" # stdin is body of test code
+	name="$1" descr="$2" # stdin is the body of the test code
 	mkdir $name &&
-	(cd $name &&
-	cat >$name.sh <<-EOF &&
-	#!$SHELL_PATH
-
-	test_description='$descr (run in sub test-lib)
-
-	This is run in a sub test-lib so that we do not get incorrect
-	passing metrics
-	'
-
-	# Point to the t/test-lib.sh, which isn't in ../ as usual
-	TEST_DIRECTORY="$TEST_DIRECTORY"
-	. "\$TEST_DIRECTORY"/test-lib.sh
-	EOF
-	cat >>$name.sh &&
-	chmod +x $name.sh &&
-	./$name.sh >out 2>err)
+	(
+		cd $name &&
+		cat >$name.sh <<-EOF &&
+		#!$SHELL_PATH
+
+		test_description='$descr (run in sub test-lib)
+
+		This is run in a sub test-lib so that we do not get incorrect
+		passing metrics
+		'
+
+		# Point to the t/test-lib.sh, which isn't in ../ as usual
+		. "\$TEST_DIRECTORY"/test-lib.sh
+		EOF
+		cat >>$name.sh &&
+		chmod +x $name.sh &&
+		export TEST_DIRECTORY &&
+		./$name.sh >out 2>err
+	)
 }
 
 check_sub_test_lib_test () {
-	name="$1" # stdin is expected output from the test
-	(cd $name &&
-	! test -s err &&
-	sed -e 's/^> //' -e 's/Z$//' >expect &&
-	test_cmp expect out)
+	name="$1" # stdin is the expected output from the test
+	(
+		cd $name &&
+		! test -s err &&
+		sed -e 's/^> //' -e 's/Z$//' >expect &&
+		test_cmp expect out
+	)
 }
 
 test_expect_success 'pretend we have fixed a known breakage' "
-- 
1.7.12.1.389.g3dff30b

Re: [PATCH v4 3/6] Color skipped tests blue

[Jeff King]

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

>  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?

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

Re: [PATCH v4 3/6] Color skipped tests blue

[Adam Spiers]

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:
> 
> >  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.