threads / patch / 31608

v2, 6 partsChange the color of individual known breakages

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

## tl;dr

19 messages between Sep 19, 2012 and Sep 20, 2012. Diffs are folded; open one to read it.

replies: 18people: 3as markdown or json

Adam Spiers· Sep 19, 2012, 17:15 UTC · lore

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

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
Adam Spiers· Sep 19, 2012, 17:15 UTC · re: Adam Spiers · lore

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(-)
Show changes to t/test-lib.sh +4 −2
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
Adam Spiers· Sep 19, 2012, 17:15 UTC · re: Adam Spiers · lore

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

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
Show changes to 2 files +3 −3

t/t0000-basic.sh, 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
Jeff King· Sep 19, 2012, 17:50 UTC · re: Adam Spiers · lore

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

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
Junio C Hamano· Sep 19, 2012, 23:39 UTC · re: Adam Spiers · lore

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

Adam Spiers <git@adamspiers.org> writes:
Show 5 quoted lines
> 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.
Adam Spiers· Sep 19, 2012, 23:45 UTC · re: Junio C Hamano · lore

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

On Thu, Sep 20, 2012 at 12:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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!

Adam Spiers· Sep 19, 2012, 17:15 UTC · re: Adam Spiers · lore

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

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(-)
Show changes to t/test-lib.sh +2 −2
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
Adam Spiers· Sep 19, 2012, 17:15 UTC · re: Adam Spiers · lore

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

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(-)
Show changes to t/t0000-basic.sh +27 −17
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
Jeff King· Sep 19, 2012, 17:56 UTC · re: Adam Spiers · lore

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

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:

Show 9 quoted lines
> @@ -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
Adam Spiers· Sep 19, 2012, 18:44 UTC · re: Jeff King · lore

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

On Wed, Sep 19, 2012 at 01:56:55PM -0400, Jeff King wrote:
Show 20 quoted lines
> 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.

Adam Spiers· Sep 19, 2012, 18:49 UTC · re: Adam Spiers · lore

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

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(-)
Show changes to t/t0000-basic.sh +29 −38
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
Adam Spiers· Sep 19, 2012, 18:49 UTC · re: Adam Spiers · lore

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

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(+)
Show changes to t/t0000-basic.sh +104 −0
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
Adam Spiers· Sep 19, 2012, 18:49 UTC · re: Adam Spiers · lore

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

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(-)
Show changes to 2 files +33 −10

t/t0000-basic.sh, t/test-lib.sh

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
Junio C Hamano· Sep 20, 2012, 21:13 UTC · re: Adam Spiers · lore

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

Adam Spiers <git@adamspiers.org> writes:
Show 44 quoted lines
> 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(-)
Show changes to t/t0000-basic.sh +27 −23
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
Jeff King· Sep 19, 2012, 19:37 UTC · re: Adam Spiers · lore

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

On Wed, Sep 19, 2012 at 07:44:06PM +0100, Adam Spiers wrote:
Show 8 quoted lines
> > 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
Adam Spiers· Sep 19, 2012, 20:15 UTC · re: Jeff King · lore

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

On Wed, Sep 19, 2012 at 03:37:08PM -0400, Jeff King wrote:
Show 7 quoted lines
> 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 ;-)
Adam Spiers· Sep 19, 2012, 17:15 UTC · re: Adam Spiers · lore

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

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(+)
Show changes to t/t0000-basic.sh +110 −0
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
Adam Spiers· Sep 19, 2012, 17:15 UTC · re: Adam Spiers · lore

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

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(-)
Show changes to 2 files +32 −10

t/t0000-basic.sh, t/test-lib.sh

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
Jeff King· Sep 19, 2012, 18:05 UTC · re: Adam Spiers · lore

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

On Wed, Sep 19, 2012 at 06:15:09PM +0100, Adam Spiers wrote:
Show 11 quoted lines
> 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

← back to recent threads