[PATCH v6 0/7] make test output coloring more intuitive

[PATCH v6 0/7] 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,

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

  - blue is only used for skipped tests, and

  - cyan is used for other informational messages.

Since unexpected test passes are no longer treated as passes, the
summary lines displayed at the end of a test run have enough different
possible outputs to warrant them being covered in the test framework's
self-tests.  Therefore this series also refactors and extends the
self-tests.

Adam Spiers (7):
  tests: test number comes first in 'not ok $count - $message'
  tests: paint known breakages in bold yellow
  tests: paint skipped tests in bold blue
  tests: change info messages from yellow/brown to bold cyan
  tests: refactor mechanics of testing in a sub test-lib
  tests: test the test framework more thoroughly
  tests: paint unexpectedly fixed known breakages in bold red

 t/t0000-basic.sh | 211 ++++++++++++++++++++++++++++++++++++++++++-------------
 t/test-lib.sh    |  25 ++++---
 2 files changed, 180 insertions(+), 56 deletions(-)

-- 
1.7.12.1.396.g53b3ea9

[PATCH v6 1/7] tests: test number comes first in 'not ok $count - $message'

[Adam Spiers]

The old output to say "not ok - 1 messsage" was working by accident
only because the test numbers are optional in TAP.

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

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 562cf41..46ccda3 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -189,13 +189,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
index f50f834..d0b236f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -298,7 +298,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.1.396.g53b3ea9

[PATCH v6 2/7] tests: paint known breakages in bold yellow

[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 d0b236f..710f051 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -213,6 +213,8 @@ then
 			tput bold; tput setaf 1;; # bold red
 		skip)
 			tput bold; tput setaf 2;; # bold green
+		warn)
+			tput bold; tput setaf 3;; # bold brown/yellow
 		pass)
 			tput setaf 2;;            # green
 		info)
@@ -311,7 +313,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 () {
@@ -408,7 +410,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.1.396.g53b3ea9

[PATCH v6 3/7] tests: paint skipped tests in bold blue

[Adam Spiers]

Skipped tests indicate incomplete test coverage.  Whilst this is not a
test failure or other error, it's still not a complete success.

Other testsuite related software like automake, autotest and prove
seem to use blue for skipped tests, so let's follow suit.

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 710f051..220b172 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -212,7 +212,7 @@ 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 brown/yellow
 		pass)
-- 
1.7.12.1.396.g53b3ea9

[PATCH v6 4/7] tests: change info messages from yellow/brown to bold cyan

[Adam Spiers]

Now that we've adopted a "traffic lights" coloring scheme, yellow is
used for warning messages, so we need to re-color info messages to
something less alarmist.  Blue is a universal color for informational
messages; however we are using that for skipped tests in order to
align with the color schemes of other test suites.  Therefore we use
bold cyan which is also blue-ish, but visually distinct from bold
blue.  This was suggested on the list a while ago and no-one raised
any objections:

http://thread.gmane.org/gmane.comp.version-control.git/205675/focus=205966

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 220b172..5d9d0fc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -218,7 +218,7 @@ then
 		pass)
 			tput setaf 2;;            # green
 		info)
-			tput setaf 3;;            # brown
+			tput bold; tput setaf 6;; # bold cyan
 		*)
 			test -n "$quiet" && return;;
 		esac
-- 
1.7.12.1.396.g53b3ea9

[PATCH v6 5/7] tests: 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 | 85 ++++++++++++++++++++++++++------------------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 46ccda3..fc5200f 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -45,39 +45,53 @@ 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 &&
-	#!$SHELL_PATH
-
-	test_description='A passing TODO test
-
-	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
+run_sub_test_lib_test () {
+	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-lib.sh
+		EOF
+		cat >>$name.sh &&
+		chmod +x $name.sh &&
+		export TEST_DIRECTORY &&
+		./$name.sh >out 2>err
+	)
+}
 
-	test_expect_failure 'pretend we have fixed a known breakage' '
-		:
-	'
+check_sub_test_lib_test () {
+	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' "
+	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' '
@@ -159,19 +173,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 &&
@@ -181,14 +184,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 &&
@@ -202,8 +199,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.1.396.g53b3ea9

[PATCH v6 6/7] tests: 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 to avoid disrupting 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 fc5200f..5c1dde0 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -79,6 +79,55 @@ check_sub_test_lib_test () {
 	)
 }
 
+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'
@@ -92,6 +141,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.1.396.g53b3ea9

[PATCH v6 7/7] tests: paint unexpectedly fixed known breakages in bold red

[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 5c1dde0..bd6127f 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -134,13 +134,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 &&
@@ -188,10 +206,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 5d9d0fc..b1acdfc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -308,7 +308,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_ () {
@@ -406,13 +406,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
@@ -426,7 +431,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.1.396.g53b3ea9

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> This series of commits attempts to make test output coloring
> more intuitive,...

Thanks; I understand that this is to replace the previous one
b465316 (tests: paint unexpectedly fixed known breakages in bold
red, 2012-09-19)---am I correct?

>   - 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,
>
>   - green is only used for things which have gone to plan and
>     require no further work to be done,
>
>   - blue is only used for skipped tests, and
>
>   - cyan is used for other informational messages.

OK.

> Since unexpected test passes are no longer treated as passes, the
> summary lines displayed at the end of a test run have enough different
> possible outputs to warrant them being covered in the test framework's
> self-tests.  Therefore this series also refactors and extends the
> self-tests.
>
> Adam Spiers (7):
>   tests: test number comes first in 'not ok $count - $message'
>   tests: paint known breakages in bold yellow
>   tests: paint skipped tests in bold blue
>   tests: change info messages from yellow/brown to bold cyan
>   tests: refactor mechanics of testing in a sub test-lib
>   tests: test the test framework more thoroughly
>   tests: paint unexpectedly fixed known breakages in bold red
>
>  t/t0000-basic.sh | 211 ++++++++++++++++++++++++++++++++++++++++++-------------
>  t/test-lib.sh    |  25 ++++---
>  2 files changed, 180 insertions(+), 56 deletions(-)

Will take a look; thanks.

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Adam Spiers]

On Sun, Dec 16, 2012 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
>> This series of commits attempts to make test output coloring
>> more intuitive,...
>
> Thanks; I understand that this is to replace the previous one
> b465316 (tests: paint unexpectedly fixed known breakages in bold
> red, 2012-09-19)---am I correct?

Correct.  AFAICS I have incorporated all feedback raised in previous
reviews.

> Will take a look; thanks.

Thanks.  Sorry again for the delay.  I'm now (finally) resuming work
on as/check-ignore.

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Junio C Hamano]

Adam Spiers <git@adamspiers.org> writes:

> On Sun, Dec 16, 2012 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Adam Spiers <git@adamspiers.org> writes:
>>
>>> This series of commits attempts to make test output coloring
>>> more intuitive,...
>>
>> Thanks; I understand that this is to replace the previous one
>> b465316 (tests: paint unexpectedly fixed known breakages in bold
>> red, 2012-09-19)---am I correct?
>
> Correct.  AFAICS I have incorporated all feedback raised in previous
> reviews.

Seemed clean from a cursory look.  Will replace.  Thanks.

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Jeff King]

On Sun, Dec 16, 2012 at 07:01:56PM +0000, Adam Spiers wrote:

> On Sun, Dec 16, 2012 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Adam Spiers <git@adamspiers.org> writes:
> >
> >> This series of commits attempts to make test output coloring
> >> more intuitive,...
> >
> > Thanks; I understand that this is to replace the previous one
> > b465316 (tests: paint unexpectedly fixed known breakages in bold
> > red, 2012-09-19)---am I correct?
> 
> Correct.  AFAICS I have incorporated all feedback raised in previous
> reviews.
> 
> > Will take a look; thanks.
> 
> Thanks.  Sorry again for the delay.  I'm now (finally) resuming work
> on as/check-ignore.

I eyeballed the test output of "pu". I do think this resolves all of the
issues brought up before, and I really hate to bikeshed on the colors at
this point, but I find that bold cyan a bit hard on the eyes when
running with "-v" (where most of the output is in that color, as it
dumps the shell for each test).  Is there any reason not to tone it down
a bit like:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 256f1c6..31f59af 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -227,7 +227,7 @@ then
 		pass)
 			tput setaf 2;;            # green
 		info)
-			tput bold; tput setaf 6;; # bold cyan
+			tput setaf 6;; # cyan
 		*)
 			test -n "$quiet" && return;;
 		esac

-Peff

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Adam Spiers]

On Thu, Dec 20, 2012 at 3:34 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Dec 16, 2012 at 07:01:56PM +0000, Adam Spiers wrote:
>> On Sun, Dec 16, 2012 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Adam Spiers <git@adamspiers.org> writes:
>> >> This series of commits attempts to make test output coloring
>> >> more intuitive,...
>> >
>> > Thanks; I understand that this is to replace the previous one
>> > b465316 (tests: paint unexpectedly fixed known breakages in bold
>> > red, 2012-09-19)---am I correct?
>>
>> Correct.  AFAICS I have incorporated all feedback raised in previous
>> reviews.
>>
>> > Will take a look; thanks.
>>
>> Thanks.  Sorry again for the delay.  I'm now (finally) resuming work
>> on as/check-ignore.
>
> I eyeballed the test output of "pu". I do think this resolves all of the
> issues brought up before, and I really hate to bikeshed on the colors at
> this point, but I find that bold cyan a bit hard on the eyes when
> running with "-v" (where most of the output is in that color, as it
> dumps the shell for each test).  Is there any reason not to tone it down
> a bit like:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 256f1c6..31f59af 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -227,7 +227,7 @@ then
>                 pass)
>                         tput setaf 2;;            # green
>                 info)
> -                       tput bold; tput setaf 6;; # bold cyan
> +                       tput setaf 6;; # cyan
>                 *)
>                         test -n "$quiet" && return;;
>                 esac
>
> -Peff

Good point, I forgot to check what it looked like with -v.  Since this
series is already on v6, is there a more lightweight way of addressing
this tiny tweak than sending v7?

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Jeff King]

On Thu, Dec 20, 2012 at 03:44:53PM +0000, Adam Spiers wrote:

> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 256f1c6..31f59af 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -227,7 +227,7 @@ then
> >                 pass)
> >                         tput setaf 2;;            # green
> >                 info)
> > -                       tput bold; tput setaf 6;; # bold cyan
> > +                       tput setaf 6;; # cyan
> >                 *)
> >                         test -n "$quiet" && return;;
> >                 esac
> >
> 
> Good point, I forgot to check what it looked like with -v.  Since this
> series is already on v6, is there a more lightweight way of addressing
> this tiny tweak than sending v7?

It is ultimately up to Junio, but I suspect he would be OK if you just
reposted patch 4/7 with the above squashed. Or even just said "I like
this, please squash it into patch 4 (change info messages from
yellow/brown to bold cyan).

As an aside, it made me wonder how hard/useful it would be to color the
snippets even more. Doing this:

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index f9ccbf2..3d44a94 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -364,7 +364,12 @@ test_expect_success () {
 	export test_prereq
 	if ! test_skip "$@"
 	then
-		say >&3 "expecting success: $2"
+		if test -z "$GIT_TEST_HIGHLIGHT"; then
+			say >&3 "expecting success: $2"
+		else
+			say >&3 "expecting success:"
+			echo "$2" | eval "$GIT_TEST_HIGHLIGHT"
+		fi
 		if test_run_ "$2"
 		then
 			test_ok_ "$1"

produces highlighted snippets with:

  GIT_TEST_HIGHLIGHT='highlight -S sh -O ansi'

or

  GIT_TEST_HIGHLIGHT='pygmentize -l sh'

depending on what you have installed on your system. I'm not convinced
it actually adds anything, but it's a fun toy. A real patch would
probably turn it off in non-verbose mode, as invoking the highlighter
(especially pygmentize) repeatedly is somewhat expensive.

-Peff

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Adam Spiers]

On Thu, Dec 20, 2012 at 4:11 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 20, 2012 at 03:44:53PM +0000, Adam Spiers wrote:
>> > diff --git a/t/test-lib.sh b/t/test-lib.sh
>> > index 256f1c6..31f59af 100644
>> > --- a/t/test-lib.sh
>> > +++ b/t/test-lib.sh
>> > @@ -227,7 +227,7 @@ then
>> >                 pass)
>> >                         tput setaf 2;;            # green
>> >                 info)
>> > -                       tput bold; tput setaf 6;; # bold cyan
>> > +                       tput setaf 6;; # cyan
>> >                 *)
>> >                         test -n "$quiet" && return;;
>> >                 esac
>> >
>>
>> Good point, I forgot to check what it looked like with -v.  Since this
>> series is already on v6, is there a more lightweight way of addressing
>> this tiny tweak than sending v7?
>
> It is ultimately up to Junio, but I suspect he would be OK if you just
> reposted patch 4/7 with the above squashed.

I'll do that if Junio is OK with that.

> Or even just said "I like
> this, please squash it into patch 4 (change info messages from
> yellow/brown to bold cyan).

Yes, I'm OK with this way too :)  Of course "bold" would need to be dropped
from the commit message.

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Good point, I forgot to check what it looked like with -v.  Since this
>> series is already on v6, is there a more lightweight way of addressing
>> this tiny tweak than sending v7?
>
> It is ultimately up to Junio, but I suspect he would be OK if you just
> reposted patch 4/7 with the above squashed. Or even just said "I like
> this, please squash it into patch 4 (change info messages from
> yellow/brown to bold cyan).

Surely; as long as the series is not in 'next', the change to be
squashed is not too big and it is not too much work (and in this
case it certainly is not).

I actually wonder if "skipped test in bold blue" and "known breakage
in bold yellow" should also lose the boldness.  Errors and warnings
in bold are good, but I would say the degree of need for attention
are more like this:

	error (failed tests - you should look into it)
        skip (skipped - perhaps you need more packages?)
        warn (expected failure - you may want to look into fixing it someday)
	info
        pass

The "expected_failure" cases painted in "warn" are all long-known
failures; I do not think reminding about them in "bold" over and
over will help encouraging the developers take a look at them.

The "skipped" cases fall into two categories.  Either you already
know you choose to not to care (e.g. I do not expect to use git-p4
and decided not to install p4 anywhere, so I may have t98?? on
GIT_SKIP_TESTS environment) or you haven't reached that point on a
new system and haven't realized that you didn't install a package
needed to run tests you care about (e.g. cvsserver tests would not
run without Perl interface to SQLite).  For the former, the bold
output is merely distracting; for the latter, bold _might_ help in
this case.

At least, I think

	GIT_SKIP_TESTS=t98?? sh t9800-git-p4-basic.sh -v

should paint "skipping test t9800 altogether" (emitted with "-v) and
the last line "1..0 # SKIP skip all tests in t9800" both in the same
"info" color.

How about going further to reduce "bold" a bit more, like this?

diff --git a/t/test-lib.sh b/t/test-lib.sh
index aaf013e..2bbb81d 100644
--- 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 4;; # bold blue
+			tput setaf 4;; # bold blue
 		warn)
-			tput bold; tput setaf 3;; # bold brown/yellow
+			tput setaf 3;; # bold brown/yellow
 		pass)
 			tput setaf 2;;            # green
 		info)
-			tput bold; tput setaf 6;; # bold cyan
+			tput setaf 6;; # bold cyan
 		*)
 			test -n "$quiet" && return;;
 		esac
@@ -589,7 +589,7 @@ for skp in $GIT_SKIP_TESTS
 do
 	case "$this_test" in
 	$skp)
-		say_color skip >&3 "skipping test $this_test altogether"
+		say_color info >&3 "skipping test $this_test altogether"
 		skip_all="skip all tests in $this_test"
 		test_done
 	esac

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Jeff King]

On Thu, Dec 20, 2012 at 11:21:09AM -0800, Junio C Hamano wrote:

> The "expected_failure" cases painted in "warn" are all long-known
> failures; I do not think reminding about them in "bold" over and
> over will help encouraging the developers take a look at them.
> 
> The "skipped" cases fall into two categories.  Either you already
> know you choose to not to care (e.g. I do not expect to use git-p4
> and decided not to install p4 anywhere, so I may have t98?? on
> GIT_SKIP_TESTS environment) or you haven't reached that point on a
> new system and haven't realized that you didn't install a package
> needed to run tests you care about (e.g. cvsserver tests would not
> run without Perl interface to SQLite).  For the former, the bold
> output is merely distracting; for the latter, bold _might_ help in
> this case.
> 
> At least, I think
> 
> 	GIT_SKIP_TESTS=t98?? sh t9800-git-p4-basic.sh -v
> 
> should paint "skipping test t9800 altogether" (emitted with "-v) and
> the last line "1..0 # SKIP skip all tests in t9800" both in the same
> "info" color.
> 
> How about going further to reduce "bold" a bit more, like this?

Yeah, I think it is a little easier on the eyes while maintaining the
intended color scheme.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index aaf013e..2bbb81d 100644
> --- 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 4;; # bold blue
> +			tput setaf 4;; # bold blue

On my xterm, at least, this is actually the difference between light
blue" and dark blue, not bold and not-bold. I think it is OK, though to
be honest, having seen the "skip all" messages in cyan (e.g., running
t9800), I think just printing skip messages in cyan looks best. But it
is not that big a deal to me, and we are well into bikeshed territory, I
think, so that will be my last word on the subject.

-Peff

Re: [PATCH v6 0/7] make test output coloring more intuitive

[Adam Spiers]

On Thu, Dec 20, 2012 at 7:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>>> Good point, I forgot to check what it looked like with -v.  Since this
>>> series is already on v6, is there a more lightweight way of addressing
>>> this tiny tweak than sending v7?
>>
>> It is ultimately up to Junio, but I suspect he would be OK if you just
>> reposted patch 4/7 with the above squashed. Or even just said "I like
>> this, please squash it into patch 4 (change info messages from
>> yellow/brown to bold cyan).
>
> Surely; as long as the series is not in 'next', the change to be
> squashed is not too big and it is not too much work (and in this
> case it certainly is not).

OK.

> I actually wonder if "skipped test in bold blue" and "known breakage
> in bold yellow" should also lose the boldness.  Errors and warnings
> in bold are good, but I would say the degree of need for attention
> are more like this:
>
>         error (failed tests - you should look into it)
>         skip (skipped - perhaps you need more packages?)
>         warn (expected failure - you may want to look into fixing it someday)
>         info
>         pass
>
> The "expected_failure" cases painted in "warn" are all long-known
> failures; I do not think reminding about them in "bold" over and
> over will help encouraging the developers take a look at them.

As Peff already noted, on many (most?) X terminals "bold" colours are
just brighter colours, rather than a heavier typeface.  How bold they
look is therefore dependent on the colour scheme used by that
terminal.

> The "skipped" cases fall into two categories.  Either you already
> know you choose to not to care (e.g. I do not expect to use git-p4
> and decided not to install p4 anywhere, so I may have t98?? on
> GIT_SKIP_TESTS environment) or you haven't reached that point on a
> new system and haven't realized that you didn't install a package
> needed to run tests you care about (e.g. cvsserver tests would not
> run without Perl interface to SQLite).  For the former, the bold
> output is merely distracting; for the latter, bold _might_ help in
> this case.

Very good point.

> At least, I think
>
>         GIT_SKIP_TESTS=t98?? sh t9800-git-p4-basic.sh -v
>
> should paint "skipping test t9800 altogether" (emitted with "-v) and
> the last line "1..0 # SKIP skip all tests in t9800" both in the same
> "info" color.
>
> How about going further to reduce "bold" a bit more, like this?
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index aaf013e..2bbb81d 100644
> --- 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 4;; # bold blue
> +                       tput setaf 4;; # bold blue

The comment still says "bold".

>                 warn)
> -                       tput bold; tput setaf 3;; # bold brown/yellow
> +                       tput setaf 3;; # bold brown/yellow

Ditto here ...

>                 pass)
>                         tput setaf 2;;            # green
>                 info)
> -                       tput bold; tput setaf 6;; # bold cyan
> +                       tput setaf 6;; # bold cyan

... and here.

>                 *)
>                         test -n "$quiet" && return;;
>                 esac
> @@ -589,7 +589,7 @@ for skp in $GIT_SKIP_TESTS
>  do
>         case "$this_test" in
>         $skp)
> -               say_color skip >&3 "skipping test $this_test altogether"
> +               say_color info >&3 "skipping test $this_test altogether"
>                 skip_all="skip all tests in $this_test"
>                 test_done
>         esac

Yes, I like this last hunk especially.

I have no objection in principle to a reduction in boldness.

However, I am beginning to get disheartened that at this rate, this
series will never land.  I already submitted v4 of the series which
already had non-bold blue.  I then received feedback indicating that
bold blue would be more suitable, so despite alarm bells beginning to
ring in my head, I submitted v5 with bold blue, declaring that that
would be my last version:

  http://article.gmane.org/gmane.comp.version-control.git/206042

A further concern about "info" messages not being blue prompted me
to attempt to canvass more opinions:

  http://article.gmane.org/gmane.comp.version-control.git/209321

I received none, so submitted v6 based on my best judgement.  Now we
are talking about a potential v7 going *back* to non-bold blue.  I can
submit v7 if you think it's worth it, but would that really be the end
of the discussion?  It's clear from the above that colour scheme
design by committee is about as good an idea as asking a bunch of kids
to reach consensus on their favourite colour ;-)

So if possible I'd be very happy for Junio to simply make an executive
decision (I don't care which way, as long as it fits the traffic
lights scheme and uses distinct hues of blue/cyan for the different
categories of skip/info messages), tweak the latest v6 series
accordingly, and then push so that we can all go back to more pressing
things ;-)

Hopefully that is a reasonable way forward?

Thanks,
Adam

[PATCH v7 0/7] coloring test output after traffic signal

[Junio C Hamano]

To conclude the bikeshedding discussion we had today, here is what I
queued by squashing stuff into relevant patches, so that people can
eyeball the result for the last time.

Adam Spiers (7):
  tests: test number comes first in 'not ok $count - $message'
  tests: paint known breakages in yellow
  tests: paint skipped tests in blue
  tests: change info messages from yellow/brown to cyan
  tests: refactor mechanics of testing in a sub test-lib
  tests: test the test framework more thoroughly
  tests: paint unexpectedly fixed known breakages in bold red

 t/t0000-basic.sh | 214 ++++++++++++++++++++++++++++++++++++++++++-------------
 t/test-lib.sh    |  29 +++++---
 2 files changed, 184 insertions(+), 59 deletions(-)

-- 
1.8.1.rc2.225.g8d36ab4

[PATCH v7 1/7] tests: test number comes first in 'not ok $count - $message'

[Junio C Hamano]

From: Adam Spiers <git@adamspiers.org>

The old output to say "not ok - 1 messsage" was working by accident
only because the test numbers are optional in TAP.

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

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
index f8e3733..03b86b8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -268,7 +268,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.8.1.rc2.225.g8d36ab4

[PATCH v7 2/7] tests: paint known breakages in yellow

[Junio C Hamano]

From: Adam Spiers <git@adamspiers.org>

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 the same yellow to be less alarmist and
more consistent with the above.

An earlier version of this patch used bold yellow but because these
are all long-known failures, reminding them to developers in bold
over and over does not help encouraging them to take a look at them
very much.  This iteration paints them in plain yellow instead to
make them less distracting.

Signed-off-by: Adam Spiers <git@adamspiers.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 03b86b8..72aafd0 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 setaf 3;; # brown/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.8.1.rc2.225.g8d36ab4

[PATCH v7 3/7] tests: paint skipped tests in blue

[Junio C Hamano]

From: Adam Spiers <git@adamspiers.org>

Skipped tests indicate incomplete test coverage.  Whilst this is not a
test failure or other error, it's still not a complete success.

Other testsuite related software like automake, autotest and prove
seem to use blue for skipped tests, so let's follow suit.

Signed-off-by: Adam Spiers <git@adamspiers.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 72aafd0..f32df80 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -182,7 +182,7 @@ then
 		error)
 			tput bold; tput setaf 1;; # bold red
 		skip)
-			tput bold; tput setaf 2;; # bold green
+			tput setaf 4;; # blue
 		warn)
 			tput setaf 3;; # brown/yellow
 		pass)
-- 
1.8.1.rc2.225.g8d36ab4

[PATCH v7 4/7] tests: change info messages from yellow/brown to cyan

[Junio C Hamano]

From: Adam Spiers <git@adamspiers.org>

Now that we've adopted a "traffic lights" coloring scheme, yellow is
used for warning messages, so we need to re-color info messages to
something less alarmist.  Blue is a universal color for informational
messages; however we are using that for skipped tests in order to
align with the color schemes of other test suites.  Therefore we use
cyan which is also blue-ish, but visually distinct from blue.

This was suggested on the list a while ago and no-one raised any
objections:

    http://thread.gmane.org/gmane.comp.version-control.git/205675/focus=205966

An earlier iteration of this patch used bold cyan, but the point of
this change is to make them less alarming; let's drop the boldness.

Also paint the message to report skipping the whole thing via
GIT_SKIP_TESTS mechanism in the same color as the "info" color
that is used on the final summary line for the entire script.

Signed-off-by: Adam Spiers <git@adamspiers.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f32df80..8b75c9a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -186,9 +186,9 @@ then
 		warn)
 			tput setaf 3;; # brown/yellow
 		pass)
-			tput setaf 2;;            # green
+			tput setaf 2;; # green
 		info)
-			tput setaf 3;;            # brown
+			tput setaf 6;; # cyan
 		*)
 			test -n "$quiet" && return;;
 		esac
@@ -584,7 +584,7 @@ for skp in $GIT_SKIP_TESTS
 do
 	case "$this_test" in
 	$skp)
-		say_color skip >&3 "skipping test $this_test altogether"
+		say_color info >&3 "skipping test $this_test altogether"
 		skip_all="skip all tests in $this_test"
 		test_done
 	esac
-- 
1.8.1.rc2.225.g8d36ab4

[PATCH v7 5/7] tests: refactor mechanics of testing in a sub test-lib

[Junio C Hamano]

From: Adam Spiers <git@adamspiers.org>

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0000-basic.sh | 85 ++++++++++++++++++++++++++------------------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index c6b42de..d0f46e8 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -55,39 +55,53 @@ 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 &&
-	#!$SHELL_PATH
-
-	test_description='A passing TODO test
-
-	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
+run_sub_test_lib_test () {
+	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-lib.sh
+		EOF
+		cat >>"$name.sh" &&
+		chmod +x "$name.sh" &&
+		export TEST_DIRECTORY &&
+		./"$name.sh" >out 2>err
+	)
+}
 
-	test_expect_failure 'pretend we have fixed a known breakage' '
-		:
-	'
+check_sub_test_lib_test () {
+	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' "
+	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 +151,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 +162,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 +177,6 @@ test_expect_success 'tests clean up even on failures' "
 	> # failed 2 among 2 test(s)
 	> 1..2
 	EOF
-	test_cmp expect out
-	)
 "
 
 ################################################################
-- 
1.8.1.rc2.225.g8d36ab4

[PATCH v7 6/7] tests: test the test framework more thoroughly

[Junio C Hamano]

From: Adam Spiers <git@adamspiers.org>

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 to avoid disrupting the metrics for
the parent tests.

Signed-off-by: Adam Spiers <git@adamspiers.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0000-basic.sh | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index d0f46e8..384b0ae 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -89,6 +89,56 @@ check_sub_test_lib_test () {
 	)
 }
 
+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'
@@ -102,6 +152,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.8.1.rc2.225.g8d36ab4

[PATCH v7 7/7] tests: paint unexpectedly fixed known breakages in bold red

[Junio C Hamano]

From: Adam Spiers <git@adamspiers.org>

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 384b0ae..8973d2c 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -145,13 +145,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 &&
@@ -199,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 8b75c9a..30a0937 100644
--- 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.8.1.rc2.225.g8d36ab4

Re: [PATCH v7 0/7] coloring test output after traffic signal

[Jeff King]

On Thu, Dec 20, 2012 at 07:12:31PM -0800, Junio C Hamano wrote:

> To conclude the bikeshedding discussion we had today, here is what I
> queued by squashing stuff into relevant patches, so that people can
> eyeball the result for the last time.

Thanks, this looks OK to me.

And thank you, Adam, for your patience. Seven iterations of color
bikeshedding is more than should be asked of anyone. :)

-Peff

Re: [PATCH v7 2/7] tests: paint known breakages in yellow

[Stefano Lattarini]

On 12/21/2012 04:12 AM, Junio C Hamano wrote:
> From: Adam Spiers <git@adamspiers.org>
> 
> 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.
>
Here are few more details about the behaviour of other testing
tools, in case you want to squash them in the commit message for
future references:

1. Automake (at least up to 1.13) and Autotest (at least up to the
   2.69 Autoconf release) use "bold" green for reporting expected
   failures.

2. On the other hand, the 'prove' utility (as of TAP::Harness v3.23
   and Perl v5.14.2) use yellow (not bold) for the same purpose.

Regards,
  Stefano

Re: [PATCH v7 0/7] coloring test output after traffic signal

[Adam Spiers]

On Fri, Dec 21, 2012 at 8:15 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 20, 2012 at 07:12:31PM -0800, Junio C Hamano wrote:
>> To conclude the bikeshedding discussion we had today, here is what I
>> queued by squashing stuff into relevant patches, so that people can
>> eyeball the result for the last time.

Great, thanks a lot Junio.

> Thanks, this looks OK to me.

To me too.

> And thank you, Adam, for your patience. Seven iterations of color
> bikeshedding is more than should be asked of anyone. :)

;-)

Re: [PATCH v7 2/7] tests: paint known breakages in yellow

[Junio C Hamano]

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

> On 12/21/2012 04:12 AM, Junio C Hamano wrote:
>> From: Adam Spiers <git@adamspiers.org>
>> 
>> 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.
>>
> Here are few more details about the behaviour of other testing
> tools, in case you want to squash them in the commit message for
> future references:
>
> 1. Automake (at least up to 1.13) and Autotest (at least up to the
>    2.69 Autoconf release) use "bold" green for reporting expected
>    failures.
>
> 2. On the other hand, the 'prove' utility (as of TAP::Harness v3.23
>    and Perl v5.14.2) use yellow (not bold) for the same purpose.

Nice to know, thanks.

I re-read the above three times, trying to see how to add it to the
log message, but having hard time phrasing it.

The only thing the additional knowledge adds seems to be to give
rationale for the old choice of "bold green"---it was not chosen
from thin-air but can be viewed as following the automake/autotest
scheme, and other systems cannot agree on what color to pick for
this purpose.

I do not see a need to justify why we chose differently from
automake/autotest; we could say something like:

    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.  This is in line with what 'prove'
    uses, but different from 'automake/autotest' do.

but we are not in the business of choosing which is more correct
between prove and automake/autotest, and I do not see how it adds
much value to tell readers that color choices are not universally
agreed upon across various test software suites---that's kind of
known, isn't it?

So...

Re: [PATCH v7 2/7] tests: paint known breakages in yellow

[Stefano Lattarini]

On 12/21/2012 04:46 PM, Junio C Hamano wrote:
>
> [SNIP]
>
> The only thing the additional knowledge adds seems to be to give
> rationale for the old choice of "bold green"---it was not chosen
> from thin-air but can be viewed as following the automake/autotest
> scheme, and other systems cannot agree on what color to pick for
> this purpose.
> 
> I do not see a need to justify why we chose differently from
> automake/autotest; we could say something like:
> 
>     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.  This is in line with what 'prove'
>     uses, but different from 'automake/autotest' do.
> 
> but we are not in the business of choosing which is more correct
> between prove and automake/autotest, and I do not see how it adds
> much value to tell readers that color choices are not universally
> agreed upon across various test software suites---that's kind of
> known, isn't it?
> 
> So...
>
That is fine with me, I just pointed it out because I suspected not
everybody was aware of all these details.  If you decide they don't
matter, it's perfectly OK -- but at least now it's an informed
choice ;-)

Thanks,
  Stefano