[PATCH] test: Better handling of stdout and stderr

[PATCH] test: Better handling of stdout and stderr

[Michal Sojka]

Git-style tests suppress stdout and stderr unless -v is given.
cworth-style tests (created by test_begin_subtest, test_expect_equal)
do not have this behavior so implement it the same.

Additionally, for both test styles, the test-lib.sh is changed so that
the content of suppressed stdout and stderr is shown in case of failed
test.

Finally a test for this functionality is added to basic tests
---
 test/basic                                 |   19 ++++++++++++++++++-
 test/test-lib.sh                           |   21 +++++++++++++++------
 test/test-verbose                          |   27 +++++++++++++++++++++++++++
 test/test.expected-output/test-verbose-no  |   20 ++++++++++++++++++++
 test/test.expected-output/test-verbose-yes |   24 ++++++++++++++++++++++++
 5 files changed, 104 insertions(+), 7 deletions(-)
 create mode 100755 test/test-verbose
 create mode 100644 test/test.expected-output/test-verbose-no
 create mode 100644 test/test.expected-output/test-verbose-yes

diff --git a/test/basic b/test/basic
index 725c753..091dda0 100755
--- a/test/basic
+++ b/test/basic
@@ -52,9 +52,26 @@ test_expect_code 2 'failure to clean up causes the test to fail' '
 # Ensure that all tests are being run
 test_begin_subtest 'Ensure that all available tests will be run by notmuch-test'
 tests_in_suite=$(grep TESTS= ../notmuch-test | sed -e "s/TESTS=\"\(.*\)\"/\1/" | tr " " "\n" | sort)
-available=$(ls -1 ../ | grep -v -E "^(aggregate-results.sh|Makefile|Makefile.local|notmuch-test|README|test-lib.sh|test-results|tmp.*|valgrind|corpus*|emacs.expected-output|smtp-dummy|smtp-dummy.c)" | sort)
+available=$(ls -1 ../ | grep -v -E "^(aggregate-results.sh|Makefile|Makefile.local|notmuch-test|README|test-lib.sh|test-results|tmp.*|valgrind|corpus*|emacs.expected-output|smtp-dummy|smtp-dummy.c|test-verbose|test.expected-output)" | sort)
 test_expect_equal "$tests_in_suite" "$available"
 
+EXPECTED=../test.expected-output
+suppress_diff_date() {
+    sed -e 's/\(.*\-\-\- test\.4\.\expected\).*/\1/' \
+	-e 's/\(.*\+\+\+ test\.4\.\output\).*/\1/'
+}
+
+test_begin_subtest "Ensure that test output is suppressed unless the test fails"
+output=$(cd ..; ./test-verbose 2>&1 | suppress_diff_date)
+expected=$(cat $EXPECTED/test-verbose-no | suppress_diff_date)
+test_expect_equal "$output" "$expected"
+
+test_begin_subtest "Ensure that -v does not suppress test output"
+output=$(cd ..; ./test-verbose -v 2>&1 | suppress_diff_date)
+expected=$(cat $EXPECTED/test-verbose-yes | suppress_diff_date)
+test_expect_equal "$output" "$expected"
+
+
 ################################################################
 # Test mail store prepared in test-lib.sh
 
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 8ecc9a0..3f69ab3 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -165,12 +165,6 @@ fi
 echo $(basename "$0"): "Testing ${test_description}"
 
 exec 5>&1
-if test "$verbose" = "t"
-then
-	exec 4>&2 3>&1
-else
-	exec 4>/dev/null 3>/dev/null
-fi
 
 test_failure=0
 test_count=0
@@ -393,6 +387,11 @@ add_email_corpus ()
 test_begin_subtest ()
 {
     test_subtest_name="$1"
+    # Remember stdout and stderr file descriptios and redirect test
+    # output to the previously prepared file descriptors 3 and 4 (see
+    # bellow)
+    if test "$verbose" != "t"; then exec 4>test.output 3>&4; fi
+    exec 6>&1 7>&2 >&3 2>&4
 }
 
 # Pass test if two arguments match
@@ -403,6 +402,7 @@ test_begin_subtest ()
 # name.
 test_expect_equal ()
 {
+	exec 1>&6 2>&7		# Restore stdout and stderr
 	test "$#" = 3 && { prereq=$1; shift; } || prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test_expect_equal"
@@ -498,6 +498,7 @@ test_failure_ () {
 	echo " $1"
 	shift
 	echo "$@" | sed -e 's/^/	/'
+	if test "$verbose" != "t"; then cat test.output; fi
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
 }
 
@@ -519,6 +520,7 @@ test_debug () {
 
 test_run_ () {
 	test_cleanup=:
+	if test "$verbose" != "t"; then exec 4>test.output 3>&4; fi
 	eval >&3 2>&4 "$1"
 	eval_ret=$?
 	eval >&3 2>&4 "$test_cleanup"
@@ -908,6 +910,13 @@ EOF
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$test" || error "Cannot setup test environment"
 
+if test "$verbose" = "t"
+then
+	exec 4>&2 3>&1
+else
+	exec 4>test.output 3>&4
+fi
+
 this_test=${0##*/}
 this_test=${this_test%%-*}
 for skp in $NOTMUCH_SKIP_TESTS
diff --git a/test/test-verbose b/test/test-verbose
new file mode 100755
index 0000000..ea52f93
--- /dev/null
+++ b/test/test-verbose
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+test_description='the verbosity options of the test framework itself.'
+
+. ./test-lib.sh
+
+test_expect_success 'print something in git-style test and pass' '
+  echo "hello stdout" &&
+  echo "hello stderr" >&2 &&
+  true
+'
+test_expect_success 'print something in git-style test and fail' '
+  echo "hello stdout" &&
+  echo "hello stderr" >&2 &&
+  false
+'
+test_begin_subtest 'print something in cworth-style test and pass'
+echo "hello stdout"
+echo "hello stderr" >&2
+test_expect_equal "a" "a"
+
+test_begin_subtest 'print something in cworth-style test and fail'
+echo "hello stdout"
+echo "hello stderr" >&2
+test_expect_equal "a" "b"
+
+test_done
diff --git a/test/test.expected-output/test-verbose-no b/test/test.expected-output/test-verbose-no
new file mode 100644
index 0000000..04fe71d
--- /dev/null
+++ b/test/test.expected-output/test-verbose-no
@@ -0,0 +1,20 @@
+test-verbose: Testing the verbosity options of the test framework itself.
+ PASS   print something in git-style test and pass
+ FAIL   print something in git-style test and fail
+	
+	  echo "hello stdout" &&
+	  echo "hello stderr" >&2 &&
+	  false
+	
+hello stdout
+hello stderr
+ PASS   print something in cworth-style test and pass
+ FAIL   print something in cworth-style test and fail
+	--- test.4.expected	2010-11-09 23:18:55.597013913 +0000
+	+++ test.4.output	2010-11-09 23:18:55.597013913 +0000
+	@@ -1 +1 @@
+	-b
+	+a
+hello stdout
+hello stderr
+
diff --git a/test/test.expected-output/test-verbose-yes b/test/test.expected-output/test-verbose-yes
new file mode 100644
index 0000000..a84a574
--- /dev/null
+++ b/test/test.expected-output/test-verbose-yes
@@ -0,0 +1,24 @@
+test-verbose: Testing the verbosity options of the test framework itself.
+hello stdout
+hello stderr
+ PASS   print something in git-style test and pass
+hello stdout
+hello stderr
+ FAIL   print something in git-style test and fail
+	
+	  echo "hello stdout" &&
+	  echo "hello stderr" >&2 &&
+	  false
+	
+hello stdout
+hello stderr
+ PASS   print something in cworth-style test and pass
+hello stdout
+hello stderr
+ FAIL   print something in cworth-style test and fail
+	--- test.4.expected	2010-11-09 23:19:05.756552603 +0000
+	+++ test.4.output	2010-11-09 23:19:05.756552603 +0000
+	@@ -1 +1 @@
+	-b
+	+a
+
-- 
1.7.1

Re: [PATCH] test: Better handling of stdout and stderr

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]

On Wed, 10 Nov 2010 00:40:35 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> Git-style tests suppress stdout and stderr unless -v is given.
> cworth-style tests (created by test_begin_subtest, test_expect_equal)
> do not have this behavior so implement it the same.
> 
> Additionally, for both test styles, the test-lib.sh is changed so that
> the content of suppressed stdout and stderr is shown in case of failed
> test.
> 
> Finally a test for this functionality is added to basic tests

This looks like some great stuff, Michal! I especially like the way you
are testing the test-suite framework itself[*].

I did have one local commit to test-lib.sh that broke this slightly, (I
changed it so that you will now get intermediate files like
test-verbose.4.out rather than test.4.out). That would have been easy
enough to fix, but I also noticed output like the following from "make
test":

...
	 PASS   Search by to: (address)
	 PASS   Search by to: (name)
	 PASS   Search by subject: (phrase)
	json: Testing --format=json output
	 PASS   Show message: json
	 PASS   Search message: json
	 PASS   Search by subject (utf-8):
	 PASS   Show message: json, utf-8
	 PASS   Search message: json, utf-8
         
	thread-naming: Testing naming of threads with changing subject
	raw: Testing notmuch show --format=raw
	 PASS   Generate some messages
	 PASS   Attempt to show multiple raw messages
...

At first I thought the change just caused some newlines to be dropped,
(such as the missing newline just before the "json" line). But then I
noticed that all output from all of the thread-naming tests is entirely
missing.

Do you want to look into what's going on there?

Meanwhile, I've just now pushed out my little test-lib.sh change, so you
can update the patch for that as well.

Thanks,

-Carl

[*] The current git test suite tries to do some things like this, but
not as cleverly. So it actually reports a FAIL and a BROKEN test when in
fact it's only trying to exercise the parts of the test suite that
report FAIL and BROKEN. I found that objectionable, so simply removed
those tests. But calling to a subordinate test that would report FAIL
and then reporting PASS after checking that would be just fine. That's a
change we could implement and even contribute upstream to the git
project itself.

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] test: Better handling of stdout and stderr

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 863 bytes --]

On Wed, 10 Nov 2010 13:15:45 -0800, Carl Worth <cworth@cworth.org> wrote:
> Meanwhile, I've just now pushed out my little test-lib.sh change, so you
> can update the patch for that as well.

Oh, and one other thing I forgot to mention about the patch. It
currently talks about things along the lines of "git-style tests" and
"cworth-style tests".

You and I might understand perfectly well what that means now, but we
need the test suite to be independently comprehensible. (I'd like to be
able to understand it myself when I look at it again in the future,
having forgotten the current history.)

So if you could describe these instead as something like "tests using
test_expect_success" vs. "tests using
test_begin_subtest/test_expect_equal" or something like that, that would
be great.

Thanks again,

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] test: Better handling of stdout and stderr

[Michal Sojka]

On Thu, 11 Nov 2010, Carl Worth wrote:
> On Wed, 10 Nov 2010 13:15:45 -0800, Carl Worth <cworth@cworth.org> wrote:
> > Meanwhile, I've just now pushed out my little test-lib.sh change, so you
> > can update the patch for that as well.
> 
> Oh, and one other thing I forgot to mention about the patch. It
> currently talks about things along the lines of "git-style tests" and
> "cworth-style tests".
> 
> You and I might understand perfectly well what that means now, but we
> need the test suite to be independently comprehensible. (I'd like to be
> able to understand it myself when I look at it again in the future,
> having forgotten the current history.)
> 
> So if you could describe these instead as something like "tests using
> test_expect_success" vs. "tests using
> test_begin_subtest/test_expect_equal" or something like that, that would
> be great.

I'll look at this tomorrow.

-Michal

Re: [PATCH] test: Better handling of stdout and stderr

[Michal Sojka]

On Thu, 11 Nov 2010, Carl Worth wrote:
> Oh, and one other thing I forgot to mention about the patch. It
> currently talks about things along the lines of "git-style tests" and
> "cworth-style tests".
> 
> You and I might understand perfectly well what that means now, but we
> need the test suite to be independently comprehensible. (I'd like to be
> able to understand it myself when I look at it again in the future,
> having forgotten the current history.)
> 
> So if you could describe these instead as something like "tests using
> test_expect_success" vs. "tests using
> test_begin_subtest/test_expect_equal" or something like that, that would
> be great.

Hi Carl,

I'm sending the fixes to the test suite as responses to this mail.

-Michal

[PATCH 1/5] test: Better handling of stdout and stderr

[Michal Sojka]

Git-style tests (test_expect_success etc.) suppress stdout and stderr
unless -v is given. Notmuch-style tests (created by test_begin_subtest
and test_expect_equal) do not have this behavior so implement it the
same.

Additionally, for both test styles, the test-lib.sh is changed so that
the content of suppressed stdout and stderr is shown in case of failed
test.

Finally a test for this functionality is added to basic tests.
---
 test/basic                                 |   21 ++++++++++++++++++++-
 test/test-lib.sh                           |   21 +++++++++++++++------
 test/test-verbose                          |   27 +++++++++++++++++++++++++++
 test/test.expected-output/test-verbose-no  |   20 ++++++++++++++++++++
 test/test.expected-output/test-verbose-yes |   24 ++++++++++++++++++++++++
 5 files changed, 106 insertions(+), 7 deletions(-)
 create mode 100755 test/test-verbose
 create mode 100644 test/test.expected-output/test-verbose-no
 create mode 100644 test/test.expected-output/test-verbose-yes

diff --git a/test/basic b/test/basic
index 725c753..309779c 100755
--- a/test/basic
+++ b/test/basic
@@ -52,9 +52,28 @@ test_expect_code 2 'failure to clean up causes the test to fail' '
 # Ensure that all tests are being run
 test_begin_subtest 'Ensure that all available tests will be run by notmuch-test'
 tests_in_suite=$(grep TESTS= ../notmuch-test | sed -e "s/TESTS=\"\(.*\)\"/\1/" | tr " " "\n" | sort)
-available=$(ls -1 ../ | grep -v -E "^(aggregate-results.sh|Makefile|Makefile.local|notmuch-test|README|test-lib.sh|test-results|tmp.*|valgrind|corpus*|emacs.expected-output|smtp-dummy|smtp-dummy.c)" | sort)
+available=$(ls -1 ../ | grep -v -E "^(aggregate-results.sh|Makefile|Makefile.local|notmuch-test|README|test-lib.sh|test-results|tmp.*|valgrind|corpus*|emacs.expected-output|smtp-dummy|smtp-dummy.c|test-verbose|test.expected-output)" | sort)
 test_expect_equal "$tests_in_suite" "$available"
 
+EXPECTED=../test.expected-output
+suppress_diff_date() {
+    sed -e 's/\(.*\-\-\- test-verbose\.4\.\expected\).*/\1/' \
+	-e 's/\(.*\+\+\+ test-verbose\.4\.\output\).*/\1/'
+}
+
+test_begin_subtest "Ensure that test output is suppressed unless the test fails"
+output=$(cd ..; ./test-verbose 2>&1 | suppress_diff_date)
+expected=$(cat $EXPECTED/test-verbose-no | suppress_diff_date)
+test_expect_equal "$output" "$expected"
+
+test_begin_subtest "Ensure that -v does not suppress test output"
+output=$(cd ..; ./test-verbose -v 2>&1 | suppress_diff_date)
+expected=$(cat $EXPECTED/test-verbose-yes | suppress_diff_date)
+# Do not include the results of test-verbose in totals
+rm $TEST_DIRECTORY/test-results/test-verbose-*
+test_expect_equal "$output" "$expected"
+
+
 ################################################################
 # Test mail store prepared in test-lib.sh
 
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 418eaa7..68c9cf8 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -165,12 +165,6 @@ fi
 echo $(basename "$0"): "Testing ${test_description}"
 
 exec 5>&1
-if test "$verbose" = "t"
-then
-	exec 4>&2 3>&1
-else
-	exec 4>/dev/null 3>/dev/null
-fi
 
 test_failure=0
 test_count=0
@@ -403,6 +397,11 @@ add_email_corpus ()
 test_begin_subtest ()
 {
     test_subtest_name="$1"
+    # Remember stdout and stderr file descriptios and redirect test
+    # output to the previously prepared file descriptors 3 and 4 (see
+    # bellow)
+    if test "$verbose" != "t"; then exec 4>test.output 3>&4; fi
+    exec 6>&1 7>&2 >&3 2>&4
 }
 
 # Pass test if two arguments match
@@ -413,6 +412,7 @@ test_begin_subtest ()
 # name.
 test_expect_equal ()
 {
+	exec 1>&6 2>&7		# Restore stdout and stderr
 	test "$#" = 3 && { prereq=$1; shift; } || prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test_expect_equal"
@@ -508,6 +508,7 @@ test_failure_ () {
 	echo " $1"
 	shift
 	echo "$@" | sed -e 's/^/	/'
+	if test "$verbose" != "t"; then cat test.output; fi
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
 }
 
@@ -529,6 +530,7 @@ test_debug () {
 
 test_run_ () {
 	test_cleanup=:
+	if test "$verbose" != "t"; then exec 4>test.output 3>&4; fi
 	eval >&3 2>&4 "$1"
 	eval_ret=$?
 	eval >&3 2>&4 "$test_cleanup"
@@ -918,6 +920,13 @@ EOF
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$test" || error "Cannot setup test environment"
 
+if test "$verbose" = "t"
+then
+	exec 4>&2 3>&1
+else
+	exec 4>test.output 3>&4
+fi
+
 this_test=${0##*/}
 for skp in $NOTMUCH_SKIP_TESTS
 do
diff --git a/test/test-verbose b/test/test-verbose
new file mode 100755
index 0000000..f29a9c7
--- /dev/null
+++ b/test/test-verbose
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+test_description='the verbosity options of the test framework itself.'
+
+. ./test-lib.sh
+
+test_expect_success 'print something in test_expect_success and pass' '
+  echo "hello stdout" &&
+  echo "hello stderr" >&2 &&
+  true
+'
+test_expect_success 'print something in test_expect_success and fail' '
+  echo "hello stdout" &&
+  echo "hello stderr" >&2 &&
+  false
+'
+test_begin_subtest 'print something between test_begin_subtest and test_expect_equal and pass'
+echo "hello stdout"
+echo "hello stderr" >&2
+test_expect_equal "a" "a"
+
+test_begin_subtest 'print something test_begin_subtest and test_expect_equal and fail'
+echo "hello stdout"
+echo "hello stderr" >&2
+test_expect_equal "a" "b"
+
+test_done
diff --git a/test/test.expected-output/test-verbose-no b/test/test.expected-output/test-verbose-no
new file mode 100644
index 0000000..0bca754
--- /dev/null
+++ b/test/test.expected-output/test-verbose-no
@@ -0,0 +1,20 @@
+test-verbose: Testing the verbosity options of the test framework itself.
+ PASS   print something in test_expect_success and pass
+ FAIL   print something in test_expect_success and fail
+	
+	  echo "hello stdout" &&
+	  echo "hello stderr" >&2 &&
+	  false
+	
+hello stdout
+hello stderr
+ PASS   print something between test_begin_subtest and test_expect_equal and pass
+ FAIL   print something test_begin_subtest and test_expect_equal and fail
+	--- test-verbose.4.expected	2010-11-14 21:41:12.738189710 +0000
+	+++ test-verbose.4.output	2010-11-14 21:41:12.738189710 +0000
+	@@ -1 +1 @@
+	-b
+	+a
+hello stdout
+hello stderr
+
diff --git a/test/test.expected-output/test-verbose-yes b/test/test.expected-output/test-verbose-yes
new file mode 100644
index 0000000..ebe5187
--- /dev/null
+++ b/test/test.expected-output/test-verbose-yes
@@ -0,0 +1,24 @@
+test-verbose: Testing the verbosity options of the test framework itself.
+hello stdout
+hello stderr
+ PASS   print something in test_expect_success and pass
+hello stdout
+hello stderr
+ FAIL   print something in test_expect_success and fail
+	
+	  echo "hello stdout" &&
+	  echo "hello stderr" >&2 &&
+	  false
+	
+hello stdout
+hello stderr
+ PASS   print something between test_begin_subtest and test_expect_equal and pass
+hello stdout
+hello stderr
+ FAIL   print something test_begin_subtest and test_expect_equal and fail
+	--- test-verbose.4.expected	2010-11-14 21:41:06.650023289 +0000
+	+++ test-verbose.4.output	2010-11-14 21:41:06.650023289 +0000
+	@@ -1 +1 @@
+	-b
+	+a
+
-- 
1.7.2.3

[PATCH 2/5] test: Add trailing newline to error messages

[Michal Sojka]

The newline was removed from say_color in commit 222926ab to allow
printing test status in the beginning of the line. Error messages are
never followed by other text so we add the newline to error function.
---
 test/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 68c9cf8..dce9077 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -144,7 +144,7 @@ else
 fi
 
 error () {
-	say_color error "error: $*"
+	say_color error "error: $*\n"
 	GIT_EXIT_OK=t
 	exit 1
 }
-- 
1.7.2.3

[PATCH 3/5] test: Break on test script (or other) error

[Michal Sojka]

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.
---
 test/notmuch-test |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/test/notmuch-test b/test/notmuch-test
index b51045a..055467f 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -14,12 +14,20 @@ 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 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
@@ -31,3 +39,5 @@ done
 
 # Clean up
 rm -r test-results corpus.mail
+
+trap '' 0
-- 
1.7.2.3

[PATCH 4/5] test: Detect unfinished subsets

[Michal Sojka]

When test_begin_subtest is not followed by corresponding test_expect_equal,
the output of the rest of the test script is errornously suppressed. Add
code to detect these bugs in test scripts.
---
 test/test-lib.sh |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index dce9077..04a4c14 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -396,12 +396,17 @@ add_email_corpus ()
 
 test_begin_subtest ()
 {
+    if [ -n "$inside_subtest" ]; then
+	exec 1>&6 2>&7		# Restore stdout and stderr
+	error "bug in test script: Missing test_expect_equal in ${BASH_SOURCE[1]}:${BASH_LINENO[0]}"
+    fi
     test_subtest_name="$1"
     # Remember stdout and stderr file descriptios and redirect test
     # output to the previously prepared file descriptors 3 and 4 (see
     # bellow)
     if test "$verbose" != "t"; then exec 4>test.output 3>&4; fi
     exec 6>&1 7>&2 >&3 2>&4
+    inside_subtest=t
 }
 
 # Pass test if two arguments match
@@ -413,6 +418,7 @@ test_begin_subtest ()
 test_expect_equal ()
 {
 	exec 1>&6 2>&7		# Restore stdout and stderr
+	inside_subtest=
 	test "$#" = 3 && { prereq=$1; shift; } || prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test_expect_equal"
-- 
1.7.2.3

[PATCH 5/5] test: Fix bugs detected thanks to the previous commit

[Michal Sojka]

---
 test/from-guessing |    1 +
 test/search        |   26 ++++++++++++++++++++++++--
 test/thread-naming |    1 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/test/from-guessing b/test/from-guessing
index 8de543f..6744e68 100755
--- a/test/from-guessing
+++ b/test/from-guessing
@@ -98,6 +98,7 @@ On Tue, 05 Jan 2010 15:43:56 -0000, Sender <sender@example.com> wrote:
 
 test_begin_subtest "Testing From line heuristics (with single configured address)"
 sed -i -e "s/^other_email.*//" "${NOTMUCH_CONFIG}"
+test_expect_equal '' ''
 
 test_begin_subtest "Magic from guessing (nothing to go on)"
 add_message '[from]="Sender <sender@example.com>"' \
diff --git a/test/search b/test/search
index 5939c6a..b180c7f 100755
--- a/test/search
+++ b/test/search
@@ -73,9 +73,31 @@ add_message '[subject]="this phrase should not match the subject search test"' '
 output=$(notmuch search 'subject:"subject search test (phrase)"' | notmuch_search_sanitize)
 test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; subject search test (phrase) (inbox unread)"
 
-test_begin_subtest 'Search for all messages ("*"
+test_begin_subtest 'Search for all messages ("*")'
 output=$(notmuch search '*' | notmuch_search_sanitize)
-test_expect_equal):' "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; body search (inbox unread)
+test_expect_equal "$output" "thread:XXX   2009-11-18 [1/1] Chris Wilson; [notmuch] [PATCH 1/2] Makefile: evaluate pkg-config once (inbox unread)
+thread:XXX   2009-11-18 [2/2] Alex Botero-Lowry, Carl Worth; [notmuch] [PATCH] Error out if no query is supplied to search instead of going into an infinite loop (attachment inbox unread)
+thread:XXX   2009-11-18 [2/2] Ingmar Vanhassel, Carl Worth; [notmuch] [PATCH] Typsos (inbox unread)
+thread:XXX   2009-11-18 [3/3] Adrian Perez de Castro, Keith Packard, Carl Worth; [notmuch] Introducing myself (inbox unread)
+thread:XXX   2009-11-18 [3/3] Israel Herraiz, Keith Packard, Carl Worth; [notmuch] New to the list (inbox unread)
+thread:XXX   2009-11-18 [3/3] Jan Janak, Carl Worth; [notmuch] What a great idea! (inbox unread)
+thread:XXX   2009-11-18 [2/2] Jan Janak, Carl Worth; [notmuch] [PATCH] Older versions of install do not support -C. (inbox unread)
+thread:XXX   2009-11-18 [3/3] Aron Griffis, Keith Packard, Carl Worth; [notmuch] archive (inbox unread)
+thread:XXX   2009-11-18 [2/2] Keith Packard, Carl Worth; [notmuch] [PATCH] Make notmuch-show 'X' (and 'x') commands remove inbox (and unread) tags (inbox unread)
+thread:XXX   2009-11-18 [7/7] Lars Kellogg-Stedman, Mikhail Gusarov, Keith Packard, Carl Worth; [notmuch] Working with Maildir storage? (inbox unread)
+thread:XXX   2009-11-18 [5/5] Mikhail Gusarov, Carl Worth, Keith Packard; [notmuch] [PATCH 1/2] Close message file after parsing message headers (inbox unread)
+thread:XXX   2009-11-18 [2/2] Keith Packard, Alexander Botero-Lowry; [notmuch] [PATCH] Create a default notmuch-show-hook that highlights URLs and uses word-wrap (inbox unread)
+thread:XXX   2009-11-18 [1/1] Alexander Botero-Lowry; [notmuch] request for pull (inbox unread)
+thread:XXX   2009-11-18 [4/4] Jjgod Jiang, Alexander Botero-Lowry; [notmuch] Mac OS X/Darwin compatibility issues (inbox unread)
+thread:XXX   2009-11-18 [1/1] Rolland Santimano; [notmuch] Link to mailing list archives ? (inbox unread)
+thread:XXX   2009-11-18 [1/1] Jan Janak; [notmuch] [PATCH] notmuch new: Support for conversion of spool subdirectories into tags (inbox unread)
+thread:XXX   2009-11-18 [1/1] Stewart Smith; [notmuch] [PATCH] count_files: sort directory in inode order before statting (inbox unread)
+thread:XXX   2009-11-18 [1/1] Stewart Smith; [notmuch] [PATCH 2/2] Read mail directory in inode number order (inbox unread)
+thread:XXX   2009-11-18 [1/1] Stewart Smith; [notmuch] [PATCH] Fix linking with gcc to use g++ to link in C++ libs. (inbox unread)
+thread:XXX   2009-11-18 [2/2] Lars Kellogg-Stedman; [notmuch] \"notmuch help\" outputs to stderr? (attachment inbox unread)
+thread:XXX   2009-11-17 [1/1] Mikhail Gusarov; [notmuch] [PATCH] Handle rename of message file (inbox unread)
+thread:XXX   2009-11-17 [2/2] Alex Botero-Lowry, Carl Worth; [notmuch] preliminary FreeBSD support (attachment inbox unread)
+thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; body search (inbox unread)
 thread:XXX   2000-01-01 [1/1] searchbyfrom; search by from (inbox unread)
 thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; search by to (inbox unread)
 thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; subjectsearchtest (inbox unread)
diff --git a/test/thread-naming b/test/thread-naming
index 129536e..987c3b3 100755
--- a/test/thread-naming
+++ b/test/thread-naming
@@ -17,6 +17,7 @@ add_message '[subject]="thread-naming: Final thread subject"' \
             '[date]="Mon, 08 Jan 2001 15:43:56 -0000"' \
             "[in-reply-to]=\<$parent\>"
 final=${gen_msg_id}
+test_expect_equal '' ''
 
 test_begin_subtest "Initial thread name (oldest-first search)"
 output=$(notmuch search --sort=oldest-first thread-naming and tag:inbox | notmuch_search_sanitize)
-- 
1.7.2.3

Re: [PATCH] test: Better handling of stdout and stderr

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Sun, 14 Nov 2010 22:52:20 +0100, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> I'm sending the fixes to the test suite as responses to this mail.

Excellent fixes, Michal. Thanks so much!

I've pushed these out now, with one tiny followup to remove the
hard-coded:

	test_expect_equal '' ''

which looked really odd to me.

I didn't see any other problems in my review of these patches.

Thanks again,

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/5] test: Break on test script (or other) error

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

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.

Hi Michal,

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.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/5] test: Break on test script (or other) error

[Michal Sojka]

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