Re: [PATCH] {maint} tests: new subroutines for test skipping/failing

[Stefano Lattarini]

Hello automakers.  Resuming an old thread ...

Reference:
  <http://lists.gnu.org/archive/html/automake-patches/2011-01/msg00192.html>

On Monday 17 January 2011, Ralf Wildenhues wrote:
> [ Cc:ing Jim because of ideas at the end ]
>
And CC:ing him again because I'd like an ACK from him (see below).

> * Stefano Lattarini wrote on Sun, Jan 16, 2011 at 03:56:07PM CET:
> > This patch stemmed from this discussion:
> >  <http://lists.gnu.org/archive/html/automake-patches/2011-01/msg00144.html>
> > 
> > I'd like to have the patch applied to maint, to make eventual integration
> > of new tests easier.  But the follow-up patches converting the testsuite
> > to the use of skip_() and providing more informative messages for skipped
> > tests should go to master only, to avoid unecessary "merging churn".
> > 
> > OK?
> 
> Unfortunately not, for a couple of reasons.  I'm sorry I didn't think
> about it in more depth before, but there are several things that I think
> could be better with this patch.
> 
> First off, a legal one, quoting maintain.info:
> 
>   When you copy legally significant code from another free software
>   package with a GPL-compatible license, you should look in the package's
>   records to find out the authors of the part you are copying, and list
>   them as the contributors of the code that you copied.  If all you did
>   was copy it, not write it, then for copyright purposes you are _not_
>   one of the contributors of _this_ code.
> 
> So, if we copy the code, then Jim is the author of it.
>
OK, I've added Jim as the primary author of this patch (both in the
ChangeLog and with "git --author").  I'll wait his ACK on this attribution
before pushing.

> Then, there are issues that the code exposes that I think we should
> address:
> 
> > tests: new subroutines for test skipping/failing.
> > 
> > * tests/defs.in (Exit): Move definition of this function earlier.
> > (warn_, skip_, fail_, framework_failure_): New functions, inspired
> > to the homonyms in gnulib's tests/init.sh.
>
> > ($stderr_fileno_): New global variable, used by the new functions
> > above.
> > * tests/README: Updated.
> 
> > --- a/tests/defs.in
> > +++ b/tests/defs.in
> 
> > @@ -88,6 +88,31 @@ echo "$PATH"
> >  # (See note about `export' in the Autoconf manual.)
> >  export PATH
> >  
> > +# Print warnings (e.g., about skipped and failed tests) to this file 
> > number.
> > +# Override by putting, say:
> > +#   stderr_fileno_=9; export stderr_fileno_; exec 9>&2; $(SHELL)
> > +# in the definition of TESTS_ENVIRONMENT.
> 
> That may be good advice in the context of gnulib.  However, it describes
> what is essentially a bug, or at least usability issue, in Automake:
> that the test author cannot write to the original stderr with the
> parallel-tests driver any more, and has to use a workaround which breaks
> user overrides of TESTS_ENVIRONMENT.
> 
> I think this should be addressed in the driver(s) ...
>
> [CUT]
>
> I further wonder whether we should finally introduce $(AM_TESTS_ENVIRONMENT)
> and reserve the non-AM_ variable for environment settings for the user.
> 
> What do you think?
>
JFTR: we agreed on this, and the change went in (into master) with the
commits  `v1.11-349-g12f48fa' and `v1.11-348-g95bbdf1' (with some more
churn than expected, due to some excessive haste on my part).

> > +# This is useful when using automake's parallel tests mode, to print
> > +# the reason for skip/failure to console, rather than to the .log files.
> > +: ${stderr_fileno_=2}
> > +
> > +warn_() { echo "$@" 1>&$stderr_fileno_; }
> > +fail_() { warn_ "$me: failed test: $@"; Exit 1; }
> > +skip_() { warn_ "$me: skipped test: $@"; Exit 77; }
> > +framework_failure_() { warn_ "$me: set-up failure: $@"; Exit 99; }
> 
> space before ()
>
Done (both here and in the gnulib repository).

The updated patch is attached; when I got Jim ACK, I'll push it to maint,
to make eventual integration of new testcases using the new subroutines
easier (and because the patch is really non-invasive).  I'll follow-up
soonish with patches converting the testsuite to the use of skip_() and
of more informative skip messages; this patches will be applied to a new
temporary public branch "testsuite-work", meant to be finally merged into
master (once it's stabilized).

Regards,
  Stefano

From 2c8b68396a48f70de2856f0747968225f4906abf Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 16 Jan 2011 15:36:07 +0100
Subject: [PATCH] tests: new subroutines for test skipping/failing.

* tests/defs.in (Exit): Move definition of this function earlier.
(warn_, skip_, fail_, framework_failure_): New functions, inspired
to the homonyms in gnulib's tests/init.sh.
($stderr_fileno_): New global variable, used by the new functions
above.
* tests/README: Updated.

From a suggestion by Ralf Wildenhues.
---
 ChangeLog     |   12 ++++++++++++
 tests/README  |    5 +++++
 tests/defs.in |   38 ++++++++++++++++++++++++++------------
 3 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4417d9b..3887852 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2011-04-23  Jim Meyering  <address@hidden>
+           Stefano Lattarini  <address@hidden>
+
+       test defs: new subroutines for test skipping/failing
+       * tests/defs.in (Exit): Move definition of this function earlier.
+       (warn_, skip_, fail_, framework_failure_): New functions, inspired
+       to the homonyms in gnulib's tests/init.sh.
+       ($stderr_fileno_): New global variable, used by the new functions
+       above.
+       * tests/README: Updated.
+       From a suggestion by Ralf Wildenhues.
+
 2011-04-22  Stefano Lattarini  <address@hidden>
 
        testsuite: more environment sanitization
diff --git a/tests/README b/tests/README
index 93f9cbf..26ce3ff 100644
--- a/tests/README
+++ b/tests/README
@@ -100,6 +100,11 @@ Do
   Include ./defs in every test script (see existing tests for examples
   of how to do this).
 
+  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.
+
   For tests that use the `parallel-tests' Automake option, set the shell
   variable `parallel_tests' to "yes" before including ./defs.  Also,
   use for them a name that ends in `-p.test' and does not clash with any
diff --git a/tests/defs.in b/tests/defs.in
index ef35d9f..4e0db16 100644
--- a/tests/defs.in
+++ b/tests/defs.in
@@ -122,6 +122,32 @@ echo "$PATH"
 # (See note about `export' in the Autoconf manual.)
 export PATH
 
+# We use a trap below for cleanup.  This requires us to go through
+# hoops to get the right exit status transported through the signal.
+# So use `Exit STATUS' instead of `exit STATUS' inside of the tests.
+# Turn off errexit here so that we don't trip the bug with OSF1/Tru64
+# sh inside this function.
+Exit ()
+{
+  set +e
+  (exit $1)
+  exit $1
+}
+
+# Print warnings (e.g., about skipped and failed tests) to this file
+# number.  Override by putting, say:
+#   stderr_fileno_=9; export stderr_fileno_; exec 9>&2;
+# in the definition of AM_TESTS_ENVIRONMENT.
+# This is useful when using automake's parallel tests mode, to print the
+# reason for skip/failure to console, rather than to the *.log files.
+: ${stderr_fileno_=2}
+
+# Copied from Gnulib's `tests/init.sh'.
+warn_ () { echo "$@" 1>&$stderr_fileno_; }
+fail_ () { warn_ "$me: failed test: $@"; Exit 1; }
+skip_ () { warn_ "$me: skipped test: $@"; Exit 77; }
+framework_failure_ () { warn_ "$me: set-up failure: $@"; Exit 99; }
+
 for tool in : $required
 do
   # Check that each required tool is present.
@@ -278,18 +304,6 @@ case "$srcdir" in
     ;;
 esac
 
-# We use a trap below for cleanup.  This requires us to go through
-# hoops to get the right exit status transported through the signal.
-# So use `Exit STATUS' instead of `exit STATUS' inside of the tests.
-# Turn off errexit here so that we don't trip the bug with OSF1/Tru64
-# sh inside this function.
-Exit ()
-{
-  set +e
-  (exit $1)
-  exit $1
-}
-
 curdir=`pwd`
 testSubDir=$me.dir
 test ! -d $testSubDir || {
-- 
1.7.2.3