[PATCHES] New function 'failure_' for testsuite hard errors

[Stefano Lattarini]

These three patches introduces a better way to declare hard errors and
generally unexpected or internal failures in the test scripts.

The first patch (for maint) introduces for this purpose a new 'failure_'
function in `tests/defs', ported from a recent addition to Gnulib's
`tests/init.sh'.

The second patch (for testsuite-work) introduces minmal checks for
the new function, using the 'self-check-report.test' pre-existing test.

The third patch (for testsuite-work) introduces few uses of this new
function in the test scripts, and some related changes.

I'll wait 48 hours before pushing, to allow for comments and objections.

Regards,
  Stefano

From 5b7c6b4ce170477dc3a25a5ca669d117bfb40aa8 Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
From: Stefano Lattarini <address@hidden>
Date: Tue, 7 Jun 2011 15:24:11 +0200
Subject: [PATCH] test defs: new function 'fatal_', for hard errors

Before this patch, the only way offered by tests/defs to
properly signal a hard error was the `framework_failure_'
function.  But the error message issued by that function,
as its name would suggest, refers to a set-up failure in the
testsuite, while hard errors can obviously also be due to
other reasons.  The best way to fix this inconsistency is to
introduce a new function with a more general error message.

Inspired by a recent similar change to Gnulib's tests/init.sh.

* tests/defs.in (fatal_): New function.
* tests/README (Section "Writing test cases" subsection "Do"):
Suggest the use of `fatal_', not of `framework_failure_', for
generic hard errors.  The latter should be reserved for "real"
set-up failures.
---
 ChangeLog     |   17 +++++++++++++++++
 tests/README  |    6 ++++--
 tests/defs.in |    1 +
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index cb9918f..5b3053d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2011-06-08  Stefano Lattarini  <address@hidden>
+
+       test defs: new function 'fatal_', for hard errors
+       Before this patch, the only way offered by tests/defs to
+       properly signal a hard error was the `framework_failure_'
+       function.  But the error message issued by that function,
+       as its name would suggest, refers to a set-up failure in the
+       testsuite, while hard errors can obviously also be due to
+       other reasons.  The best way to fix this inconsistency is to
+       introduce a new function with a more general error message.
+       Inspired by a recent similar change to Gnulib's tests/init.sh.
+       * tests/defs.in (fatal_): New function.
+       * tests/README (Section "Writing test cases" subsection "Do"):
+       Suggest the use of `fatal_', not of `framework_failure_', for
+       generic hard errors.  The latter should be reserved for "real"
+       set-up failures.
+
 2011-06-02  Stefano Lattarini  <address@hidden>
 
        maintcheck: fix some failures, extend some checks
diff --git a/tests/README b/tests/README
index 2b5b5af..57b2ddb 100644
--- a/tests/README
+++ b/tests/README
@@ -102,8 +102,10 @@ Do
 
   Use the `skip_' function to skip tests, with a meaningful message if
   possible.  Where convenient, use the `warn_' function to print generic
-  warnings, and the `fail_' function for test failures.  Finally, you may
-  use the `framework_fail_' function for hard errors.
+  warnings, the `fail_' function for test failures, and the `fatal_'
+  function for hard errors.  In case a hard error is due to a failed
+  set-up of a test scenario, you can use the `framework_fail_' function
+  instead.
 
   For tests that use the `parallel-tests' Automake option, set the shell
   variable `parallel_tests' to "yes" before including ./defs.  Also,
diff --git a/tests/defs.in b/tests/defs.in
index 87a350d..5849fbd 100644
--- a/tests/defs.in
+++ b/tests/defs.in
@@ -148,6 +148,7 @@ Exit ()
 warn_ () { echo "$@" 1>&$stderr_fileno_; }
 fail_ () { warn_ "$me: failed test: $@"; Exit 1; }
 skip_ () { warn_ "$me: skipped test: $@"; Exit 77; }
+fatal_ () { warn_ "$me: hard error: $@"; Exit 99; }
 framework_failure_ () { warn_ "$me: set-up failure: $@"; Exit 99; }
 
 # cross_compiling
-- 
1.7.2.3

From 8ddf97c5d707e64b13d25fe008078b1fafbf55b6 Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
From: Stefano Lattarini <address@hidden>
Date: Tue, 7 Jun 2011 16:00:31 +0200
Subject: [PATCH 1/2] self tests: check new 'fatal_' function

* tests/self-check-exit.test: Also check the new 'fatal_'
function.
---
 ChangeLog                    |    6 ++++++
 tests/self-check-report.test |    4 ++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e1fc269..1df76ef 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2011-06-08  Stefano Lattarini  <address@hidden>
+
+       self tests: check new 'fatal_' function
+       * tests/self-check-exit.test: Also check the new 'fatal_'
+       function.
+
 2011-06-07  Stefano Lattarini  <address@hidden>
 
        tests: `lib/' shell scripts transparently tested also with $SHELL
diff --git a/tests/self-check-report.test b/tests/self-check-report.test
index e04c88e..5434d5a 100755
--- a/tests/self-check-report.test
+++ b/tests/self-check-report.test
@@ -31,6 +31,8 @@ exec 5>&1
 (fail_ foo) 2>&1 1>&5 | grep "^$me: failed test: foo"  || Exit 1
 (skip_ foo); test $? -eq 77                            || Exit 1
 (skip_ foo) 2>&1 1>&5 | grep "^$me: skipped test: foo" || Exit 1
+(fatal_ foo); test $? -eq 99                           || Exit 1
+(fatal_ foo) 2>&1 1>&5 | grep "^$me: hard error: foo"  || Exit 1
 (framework_failure_ foo); test $? -eq 99               || Exit 1
 (framework_failure_ foo) 2>&1 1>&5 \
   | grep "^$me: set-up failure: foo"                   || Exit 1
@@ -42,6 +44,8 @@ stderr_fileno_=6
 (fail_ foo) 6>&1 1>&5 | grep "^$me: failed test: foo"  || Exit 1
 (skip_ foo); test $? -eq 77                            || Exit 1
 (skip_ foo) 6>&1 1>&5 | grep "^$me: skipped test: foo" || Exit 1
+(fatal_ foo); test $? -eq 99                           || Exit 1
+(fatal_ foo) 6>&1 1>&5 | grep "^$me: hard error: foo"  || Exit 1
 (framework_failure_ foo); test $? -eq 99               || Exit 1
 (framework_failure_ foo) 6>&1 1>&5 \
   | grep "^$me: set-up failure: foo"                   || Exit 1
-- 
1.7.2.3

From f351507a0a2020ef506b29ea8b4b0fb96acbf96e Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
In-Reply-To: <address@hidden>
References: <address@hidden>
From: Stefano Lattarini <address@hidden>
Date: Tue, 7 Jun 2011 16:36:40 +0200
Subject: [PATCH 2/2] testsuite: use 'fatal_' and 'framework_failure_' for hard 
errors

* tests/remake-gnulib-remove-header.test: Prefer using `fatal_'
with a proper error message over a direct call to `Exit 99'.
* tests/pr8365-remake-timing.test: Likewise.
* tests/cygnus-imply-foreign.test: Likewise.
* tests/missing6.test: Likewise.
* tests/cond8.test: Likewise.
* tests/cond33.test: Likewise.
* tests/python-virtualenv.test: Prefer using `framework_failure_'
with a proper error message over a direct call to `Exit 99'.
* tests/instspc-tests.sh: Prefer using `framework_failure_' and
`fatal_' over direct calls to `Exit 99'.
(fatal_): Define this (which is a simplified version of the one
in `tests/defs') for early uses (i.e., before `tests/defs'
gets sourced).
* tests/depmode-tests.sh: Likewise.  Also, simplify the
'get_depmodes' function and calls to it accordingly.
---
 ChangeLog                              |   20 +++++++++++++++++
 tests/cond33.test                      |    2 +-
 tests/cond8.test                       |    2 +-
 tests/cygnus-imply-foreign.test        |    3 +-
 tests/depmod-tests.sh                  |   35 +++++++++++++++--------------
 tests/instspc-tests.sh                 |   37 ++++++++++++++++---------------
 tests/missing6.test                    |    2 +-
 tests/pr8365-remake-timing.test        |    3 +-
 tests/python-virtualenv.test           |    4 ++-
 tests/remake-gnulib-remove-header.test |    4 +-
 10 files changed, 69 insertions(+), 43 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1df76ef..b361df3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,25 @@
 2011-06-08  Stefano Lattarini  <address@hidden>
 
+       testsuite: use 'fatal_' and 'framework_failure_' for hard errors
+       * tests/remake-gnulib-remove-header.test: Prefer using `fatal_'
+       with a proper error message over a direct call to `Exit 99'.
+       * tests/pr8365-remake-timing.test: Likewise.
+       * tests/cygnus-imply-foreign.test: Likewise.
+       * tests/missing6.test: Likewise.
+       * tests/cond8.test: Likewise.
+       * tests/cond33.test: Likewise.
+       * tests/python-virtualenv.test: Prefer using `framework_failure_'
+       with a proper error message over a direct call to `Exit 99'.
+       * tests/instspc-tests.sh: Prefer using `framework_failure_' and
+       `fatal_' over direct calls to `Exit 99'.
+       (fatal_): Define this (which is a simplified version of the one
+       in `tests/defs') for early uses (i.e., before `tests/defs'
+       gets sourced).
+       * tests/depmode-tests.sh: Likewise.  Also, simplify the
+       'get_depmodes' function and calls to it accordingly.
+
+2011-06-08  Stefano Lattarini  <address@hidden>
+
        self tests: check new 'fatal_' function
        * tests/self-check-exit.test: Also check the new 'fatal_'
        function.
diff --git a/tests/cond33.test b/tests/cond33.test
index 126a4c1..149795d 100755
--- a/tests/cond33.test
+++ b/tests/cond33.test
@@ -47,7 +47,7 @@ $ACLOCAL
 $AUTOCONF
 $AUTOMAKE
 
-cwd=`pwd` || Exit 99
+cwd=`pwd` || fatal_ "cannot get current directory"
 mkdir nowhere
 chmod a-w nowhere
 
diff --git a/tests/cond8.test b/tests/cond8.test
index 3fe7b5c..07bf2d0 100755
--- a/tests/cond8.test
+++ b/tests/cond8.test
@@ -59,7 +59,7 @@ END
 
 cp x.c y.c
 
-instdir=`pwd`/_inst || Exit 99
+instdir=`pwd`/_inst || fatal_ "cannot get current directory"
 
 # Skip the rest of the test in case of e.g. missing C compiler.
 ./configure --prefix="$instdir" x=yes || Exit $?
diff --git a/tests/cygnus-imply-foreign.test b/tests/cygnus-imply-foreign.test
index 9a20f21..1a8d7d3 100755
--- a/tests/cygnus-imply-foreign.test
+++ b/tests/cygnus-imply-foreign.test
@@ -51,7 +51,8 @@ mv -f Makefile.sav Makefile.am
 # Try again, this time enabling cygnus mode from configure.in.
 cp configure.in configure.sav
 sed 's/^AM_INIT_AUTOMAKE$/&([gnits cygnus])/' configure.sav >configure.in
-cmp configure.in configure.sav && Exit 99 # sanity check
+cmp configure.in configure.sav && fatal_ 'failed to edit configure.in'
+
 $ACLOCAL --force
 $AUTOMAKE -Werror
 mv -f configure.sav configure.in
diff --git a/tests/depmod-tests.sh b/tests/depmod-tests.sh
index 36b5719..a21f618 100755
--- a/tests/depmod-tests.sh
+++ b/tests/depmod-tests.sh
@@ -62,10 +62,14 @@ fi
 
 set -e
 
+# We need this early.  It will be overridden when we source ./defs below,
+# which will offer a more proper implementation.
+fatal_ () { echo "$0: $*" >&2; exit 99; }
+
 case $# in
-  0) echo "$0: missing argument" >&2; exit 99;;
+  0) fatal_ "missing argument";;
   1) ;;
-  *) echo "$0: too many arguments" >&2; exit 99;;
+  *) fatal_ "too many arguments";;
 esac
 
 case $1 in
@@ -77,8 +81,7 @@ case $1 in
     depmode=`expr /"$1" : '.*/depcomp-\(.*\)\.depmod'`
     ;;
   *)
-    echo "$0: invalid argument '$1'" >&2
-    exit 99
+    fatal_ "invalid argument '$1'"
     ;;
 esac
 
@@ -136,7 +139,7 @@ END
   : > success
 }
 
-# Usage: get_depmodes DEPCOMP-FILE PROGRAM-NAME
+# Usage: get_depmodes DEPCOMP-FILE
 get_depmodes ()
 {
   # Keep this in sync with the contents of depend.m4.
@@ -144,10 +147,8 @@ get_depmodes ()
                   | grep -v '^none$'` \
     && : Turn newlines and tabs into spaces, and strip extra whitespace. \
     && all_depmodes=`echo $all_depmodes` \
-    && test -n "$all_depmodes" || {
-      echo "$2: failed to extract list of valid depmodes from '$1'" >&2
-      exit 99
-    }
+    && test -n "$all_depmodes" \
+    || fatal_ "can't extract list of valid depmodes from '$1'"
 }
 
 if test x"$action" = x"generate-makefile"; then
@@ -182,8 +183,9 @@ set -e
 # the files we need.  So remove the other files created by ./defs.  And
 # check we really are in a temporary `*.dir' directory in the build tree,
 # since the last thing we want is to remove some random user files!
-test -f ../defs-static && test -f ../defs || Exit 99
-case `pwd` in *.dir);; *) Exit 99;; esac
+test -f ../defs-static && test -f ../defs \
+  && case `pwd` in *.dir) :;; *) false;; esac \
+  || fatal_ "running from the wrong directory"
 rm -f *
 
 if test x"$action" = x"generate-data"; then
@@ -194,18 +196,17 @@ if test x"$action" = x"generate-data"; then
   Exit 0
 fi
 
-get_depmodes "$top_testsrcdir/lib/depcomp" "$me"
+get_depmodes "$top_testsrcdir/lib/depcomp"
 case " $all_depmodes " in
   *" $depmode "*) ;;
-  *) echo "$me: invalid depmode '$depmode'" >&2; exit 99;;
+  *) fatal_ "invalid depmode '$depmode'";;
 esac
 
 ###  If we are still here, we have to run a test ...
 
-test -f ../depmod-data.dir/success || {
-  echo "$me: setup by depmod-data.test failed" >&2
-  Exit 99
-}
+if test ! -f ../depmod-data.dir/success; then
+  framework_failure_ "depmod-data.test failure"
+fi
 
 ../depmod-data.dir/configure am_cv_CC_dependencies_compiler_type=$depmode
 
diff --git a/tests/instspc-tests.sh b/tests/instspc-tests.sh
index c57efb7..9c6c968 100755
--- a/tests/instspc-tests.sh
+++ b/tests/instspc-tests.sh
@@ -44,11 +44,15 @@ else
 fi
 
 set -e
+ 
+# We need this early.  It will be overridden when we source ./defs below,
+# which will offer a more proper implementation.
+fatal_ () { echo "$0: $*" >&2; exit 99; }
 
 case $# in
-  0) echo "$0: missing argument" >&2; exit 99;;
+  0) fatal_ "missing argument";;
   1) ;;
-  *) echo "$0: too many arguments" >&2; exit 99;;
+  *) fatal_ "too many arguments";;
 esac
 
 case $1 in
@@ -64,8 +68,7 @@ case $1 in
     instspc_test_name=`expr /"$1" : '.*/install-\(.*\)\.instspc'`
     ;;
   *)
-    echo "$0: invalid argument '$1'" >&2
-    exit 99
+    fatal_ "invalid argument '$1'"
     ;;
 esac
 
@@ -75,7 +78,8 @@ define_problematic_string ()
 {
   tst=$1
   shift
-  eval "instspc__$tst=\$1" || exit 99
+  eval "instspc__$tst=\$1" \
+    || fatal_ "define_problematic_string: bad argument: '$tst'"
   shift
   instspc_names_list="$instspc_names_list $tst"
   # Some of the "problematic" characters cannot be used in the name of
@@ -284,8 +288,9 @@ set -e
 # the files we need.  So remove the other files created by ./defs.  And
 # check we really are in a temporary `*.dir' directory in the build tree,
 # since the last thing we want is to remove some random user files!
-test -f ../defs-static && test -f ../defs || Exit 99
-case `pwd` in *.dir);; *) Exit 99;; esac
+test -f ../defs-static && test -f ../defs \
+  && case `pwd` in *.dir) :;; *) false;; esac \
+  || fatal_ "running from the wrong directory"
 rm -f *
 
 if test x"$instspc_action" = x"generate-data"; then
@@ -298,16 +303,13 @@ fi
 
 ###  If we are still here, we have to run a test ...
 
-eval "instspc_test_string=\${instspc__$instspc_test_name}" || Exit 99
-if test x"$instspc_test_string" = x; then
-  echo "$me: invalid test name: '$instspc_test_name'" >&2
-  Exit 99
-fi
+eval "instspc_test_string=\${instspc__$instspc_test_name}" \
+  && test x"$instspc_test_string" != x \
+  || fatal_ "invalid test name: '$instspc_test_name'"
 
-test -f ../instspc-data.dir/success || {
-  echo "$me: setup by instspc-data.test failed" >&2
-  Exit 99
-}
+if test ! -f ../instspc-data.dir/success; then
+  framework_failure_ "instspc-data.test failure"
+fi
 
 # Skip if this system doesn't support these characters in file names.
 mkdir "./$instspc_test_string" || Exit 77
@@ -323,8 +325,7 @@ case $instspc_action in
     relbuilddir=..
     ;;
   *)
-    echo "$me: internal error: invalid action '$instspc_action'"
-    Exit 99
+    fatal_ "invalid action '$instspc_action'"
     ;;
 esac
 
diff --git a/tests/missing6.test b/tests/missing6.test
index 2e257f1..b6f1562 100755
--- a/tests/missing6.test
+++ b/tests/missing6.test
@@ -38,7 +38,7 @@ $AUTOMAKE
 $MAKE
 
 sed 's/^dnl!! //' < configure.ac > configure.tmp
-cmp configure.ac configure.tmp && Exit 99 # sanity check
+cmp configure.ac configure.tmp && fatal_ 'failed to edit configure.ac'
 mv -f configure.tmp configure.ac
 
 $MAKE 2>stderr || { cat stderr >&2; Exit 1; }
diff --git a/tests/pr8365-remake-timing.test b/tests/pr8365-remake-timing.test
index 5dbebc6..f549bf1 100755
--- a/tests/pr8365-remake-timing.test
+++ b/tests/pr8365-remake-timing.test
@@ -42,7 +42,8 @@ $AUTOCONF
 
 ./configure
 $MAKE Makefile
-$EGREP 'FOOBAR|zardoz' Makefile && Exit 99 # Sanity check.
+# Sanity check.
+$EGREP 'FOOBAR|zardoz' Makefile && fatal_ 'unexpected AC_SUBST in Makefile'
 
 echo 'AC_SUBST([FOOBAR])' >> configure.in
 
diff --git a/tests/python-virtualenv.test b/tests/python-virtualenv.test
index c36c2b7..5ddd8fb 100755
--- a/tests/python-virtualenv.test
+++ b/tests/python-virtualenv.test
@@ -30,7 +30,9 @@ virtualenv --verbose virtenv && test -f virtenv/bin/activate \
 # Activate the virtualenv.
 . ./virtenv/bin/activate
 # Sanity check.
-test -n "$VIRTUAL_ENV" || Exit 99
+if test -z "$VIRTUAL_ENV"; then
+  framework_failure_ "can't activate python virtual environment"
+fi
 
 cwd=`pwd`
 py_version=`python -c 'import sys; print("%u.%u" % 
tuple(sys.version_info[:2]))'`
diff --git a/tests/remake-gnulib-remove-header.test 
b/tests/remake-gnulib-remove-header.test
index de5a2fa..9e9b4e2 100755
--- a/tests/remake-gnulib-remove-header.test
+++ b/tests/remake-gnulib-remove-header.test
@@ -108,7 +108,7 @@ for vpath in : false; do
 
   $sleep
   sed -e 's/^\( *override_stdio\)=.*$/\1=false/' $srcdir/macros.m4 > t
-  diff $srcdir/macros.m4 t && Exit 99 # sanity check
+  diff $srcdir/macros.m4 t && fatal_ "failed to edit macros.m4"
   mv -f t $srcdir/macros.m4
 
   using_gmake || $MAKE Makefile
@@ -120,7 +120,7 @@ for vpath in : false; do
 
   $sleep
   sed -e 's/^\( *override_stdio\)=.*$/\1=:/' $srcdir/macros.m4 > t
-  diff $srcdir/macros.m4 t && Exit 99 # sanity check
+  diff $srcdir/macros.m4 t && fatal_ "failed to edit macros.m4"
   mv -f t $srcdir/macros.m4
 
   using_gmake || $MAKE Makefile
-- 
1.7.2.3