Re: [PATCH 0/3] Enable `errexit' a.k.a. `set -e' shell flag in all test scripts.

[Stefano Lattarini]

At Sunday 04 April 2010, Ralf Wildenhues <address@hidden> 
wrote:
> Hello Stefano,
> 
> * Stefano Lattarini wrote on Wed, Mar 31, 2010 at 01:24:05PM CEST:
> > I'll soon post a couple of patches enabling the `errexit' a.k.a.
> > `set -e' shell flag unconditionally in all test scripts.  This
> > helps making the behaviour of different test scripts globally
> > more consistent, and helps catching potential bugs that could
> > lead to false negatives (which are very bad in a testsuite IMO).
> 
> errexit handling is quite inconsistent between different
>  proprietary shells, with many bugs lurking, also in some corner
>  cases.  The Autoconf manual has some anectodes.  These changes
>  would require me to recheck all kinds of such systems and shells
>  for regressions, and it would require in-depth inspection of the
>  testsuite logs for false negatives. Checking Solaris and heirloom
>  sh is not enough, I'm afraid.
> 
More thoughts about this issue: do you think it would be OK to add a 
`set -e' call to *some* of the tests currently not setting the 
`errexit' flag, provided that:
 1. they are simple enough (i.e. are quite short, and use no
    complex composite commands nor complex command substituitions
    nor shell functions), and:
 2. I explain step-by-step why the changes introduced by `set -e'
    should be harmless (or even useful), especially by making
    comparisons with existing tests that already set the `errexit'
    flag, and:
 3. I modify one single test at time, to make patch review easier
    and more focused?

Obviously, after the thorough considerations reported in your answer, 
I'd not be upset if you decide to reject such changes, but I'd like to 
try to do them anyway if there's any hope they'll be considered.

To give a better idea of what I'm proposing, I wrote and attached a 
very simple patch where I make a tiny improvement to the test 
`aclocal3.test', enabling the `errexit' flag while I was at it.

Regards,
    Stefano

From 589df37ff3977267a5cfd4b9d9a2710d075a4ba9 Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Mon, 5 Apr 2010 18:42:02 +0200
Subject: [PATCH 1/2] Make test `aclocal3.test' stricter.

* tests/aclocal3.test: Fail if $ACLOCAL succeds unexpectedly.
---
 ChangeLog           |    5 +++++
 tests/aclocal3.test |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a5b5426..99b3b43 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2010-04-08  Stefano Lattarini  <address@hidden>
+
+       Make test `aclocal3.test' stricter.
+       * tests/aclocal3.test: Fail if $ACLOCAL succeds unexpectedly.
+
 2010-04-04  Stefano Lattarini  <address@hidden>
 
        Generated tests are now just a thin layer around other tests.
diff --git a/tests/aclocal3.test b/tests/aclocal3.test
index a550e50..8e76e6d 100755
--- a/tests/aclocal3.test
+++ b/tests/aclocal3.test
@@ -27,6 +27,6 @@ AC_DEFUN([GNOME_X_CHECKS], [
 ])
 END
 
-$ACLOCAL -I macros 2>stderr
+$ACLOCAL -I macros 2>stderr && { cat stderr >&2; Exit 1; }
 cat stderr
 grep 'macros/gnome.m4:2:.*AM_PATH_GTK.*not found' stderr
-- 
1.6.5

From f9e78a64db4c7c0d30df95faf9a8b9d6029d62f3 Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Mon, 5 Apr 2010 19:07:40 +0200
Subject: [PATCH 2/2] Use `set -e' in test aclocal3.test.

* tests/aclocal3.test: Add call to `set -e'.
The only potentially problematic construct is:
  $ACLOCAL -I macros 2>stderr && { cat stderr; Exit 1; }
but similar constructs are already used in various other tests, e.g.
in `aclocal.test':
  $ACLOCAL --output 2>stderr && { cat stderr >&2; Exit 1; }
  $ACLOCAL --unknown-option 2>stderr && { cat stderr >&2; Exit 1; }
  ...
in `check4.test' and `check8.test':
  $MAKE check >stdout && { cat stdout; Exit 1; }
  ...
etc.
---
 ChangeLog           |    3 +++
 tests/aclocal3.test |    2 ++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 99b3b43..cd76409 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2010-04-08  Stefano Lattarini  <address@hidden>
 
+       Use `set -e' in test aclocal3.test.
+       * tests/aclocal3.test: Add call to `set -e'.
+
        Make test `aclocal3.test' stricter.
        * tests/aclocal3.test: Fail if $ACLOCAL succeds unexpectedly.
 
diff --git a/tests/aclocal3.test b/tests/aclocal3.test
index 8e76e6d..b6a2fc2 100755
--- a/tests/aclocal3.test
+++ b/tests/aclocal3.test
@@ -17,6 +17,8 @@
 # Test to make sure include of include detects missing macros
 . ./defs || Exit 1
 
+set -e
+
 echo GNOME_X_CHECKS >> configure.in
 
 mkdir macros
-- 
1.6.5