From: Michal Sojka <sojkam1@fel.cvut.cz>
To: Carl Worth <cworth@cworth.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/5] test: Break on test script (or other) error
In-Reply-To: <878w015fb5.fsf@yoom.home.cworth.org>
References: <87hbfjr2sr.fsf@steelpick.2x.cz>
	<1289771672-9204-3-git-send-email-sojkam1@fel.cvut.cz>
	<878w015fb5.fsf@yoom.home.cworth.org>
User-Agent: Notmuch/0.5-11-g48b5e00 (http://notmuchmail.org) Emacs/23.2.1
	(x86_64-pc-linux-gnu)
Date: Wed, 08 Dec 2010 15:28:39 +0100
Message-ID: <877hfk72zs.fsf@steelpick.2x.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Tue, 07 Dec 2010, Carl Worth wrote:
> On Sun, 14 Nov 2010 22:54:30 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> > Break notmuch-test whenever a test script returns non-zero status.
> > This happens either when some test from the script fails or when there
> > is an error in the script.
> > 
> > This is especially useful in the latter case since the error may not
> > appear in the final aggregated results.
>
> I'm reverting this patch now, with the following commit message:
> 
>     Revert "test: Break on test script (or other) error"
>     
>     This reverts commit f22a7ec1e28d1264cf9d67d78796b8ab22e09a35.
>     
>     Interrupting the test suite due to an actual bug in a test script
>     would be just fine, but interrupting the run of the entire test suite
>     at the first test failure is unacceptable.
> 
> As I say there, if we could detect an actual bug in the test script, and
> only interrupt then, then that would be great, (perhaps using some value
> other than 1 for the test-failure indication in test_done?).
> 
> As it is, though, I'm implementing a fix for a BROKEN test case in one
> script, and that's causing a failure in an earlier test script. And the
> interruption is causing me pain since the script I *really* want to run
> isn't even getting run.
> 
> Let me know if you've got some good ideas for a better fix here.

I do not have any better idea. What about the following patch?

8<------
>From 2f1efaf5b0caafdeefc3a0ff373cc7c57b5f98dc Mon Sep 17 00:00:00 2001
From: Michal Sojka <sojkam1@fel.cvut.cz>
Date: Wed, 8 Dec 2010 15:13:40 +0100
Subject: [PATCH] test: Break on test script errors

Break notmuch-test whenever a test script exits with status other than
zero or one.

This is intended to break when a bug in the test script is detected in
which case the test script exits with status of 2. Exit status of
one means that some tests failed, but this does not break notmuch-test
unless -i (immediate) options is given.
---
 test/notmuch-test |   19 +++++++++++++++++--
 test/test-lib.sh  |   15 +++++++++------
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/test/notmuch-test b/test/notmuch-test
index 4889e49..5d105e7 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -14,16 +14,29 @@ if [ ${BASH_VERSINFO[0]} -lt 4 ]; then
     exit 1
 fi
 
+set -e
+
+die () {
+    echo >&2 "Unexpected failure"
+}
+
+trap 'die' 0
+
 cd $(dirname "$0")
 
 TESTS="basic new search search-output json thread-naming raw reply dump-restore uuencode thread-order author-order from-guessing long-id encoding emacs maildir-sync"
 
 # Clean up any results from a previous run
-rm -r test-results >/dev/null 2>/dev/null
+rm -rf test-results >/dev/null 2>/dev/null
 
 # Run the tests
 for test in $TESTS; do
-	./$test "$@"
+    ./$test "$@" || (
+	ret=$?
+	if [ "$ret" != 1 ]; then
+	    exit $ret
+	fi
+    )
 done
 
 # Report results
@@ -31,3 +44,5 @@ done
 
 # Clean up
 rm -r test-results corpus.mail
+
+trap '' 0
diff --git a/test/test-lib.sh b/test/test-lib.sh
index a197827..7450fc8 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -20,7 +20,7 @@ if [ ${BASH_VERSINFO[0]} -lt 4 ]; then
     echo "due to use of associative arrays within the test suite."
     echo "Please try again with a newer bash (or help us fix the"
     echo "test suite to be more portable). Thanks."
-    exit 1
+    exit 2
 fi
 
 # if --tee was passed, write the output not only to the terminal, but
@@ -112,7 +112,7 @@ do
 		root=$(expr "z$1" : 'z[^=]*=\(.*\)')
 		shift ;;
 	*)
-		echo "error: unknown test option '$1'" >&2; exit 1 ;;
+		echo "error: unknown test option '$1'" >&2; exit 2 ;;
 	esac
 done
 
@@ -146,7 +146,7 @@ fi
 error () {
 	say_color error "error: $*\n"
 	GIT_EXIT_OK=t
-	exit 1
+	exit 2
 }
 
 say () {
@@ -174,12 +174,15 @@ test_success=0
 
 die () {
 	code=$?
+	# Exit codes: 0 - success
+	#      	      1 - one or more test cases failed
+	#	      2 - serious error in test script
 	if test -n "$GIT_EXIT_OK"
 	then
 		exit $code
 	else
 		echo >&5 "FATAL: Unexpected exit with code $code"
-		exit 1
+		exit 2
 	fi
 }
 
@@ -520,7 +523,7 @@ test_failure_ () {
 	shift
 	echo "$@" | sed -e 's/^/	/'
 	if test "$verbose" != "t"; then cat test.output; fi
-	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 2; }
 }
 
 test_known_broken_ok_ () {
@@ -909,7 +912,7 @@ test ! -z "$debug" || remove_tmp=$TMP_DIRECTORY
 rm -fr "$test" || {
 	GIT_EXIT_OK=t
 	echo >&5 "FATAL: Cannot prepare test area"
-	exit 1
+	exit 2
 }
 
 MAIL_DIR="${TMP_DIRECTORY}/mail"
-- 
1.7.2.3