Re: [PATCH] {tests-init} Tests defs: improve messages for skipped tests.

[Stefano Lattarini]

On Thursday 11 November 2010, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Thu, Nov 11, 2010 at 09:10:39PM CET:
> > On Thursday 11 November 2010, Ralf Wildenhues wrote:
> > > * Stefano Lattarini wrote on Thu, Nov 11, 2010 at 02:52:06PM CET:
> 
> > > > @@ -228,11 +229,16 @@ do
> > > >        (echo foo >> $priv_check_temp) >/dev/null 2>&1
> > > >        overwrite_status=$?
> > > >        rm -f $priv_check_temp
> > > > -      test $overwrite_status = 0 && exit 77
> > > > +      if test $overwrite_status = 0; then
> > > 
> > > -eq seems more appropriate than = (more instances below).
> > OK.
> 
> Sorry for making you fix bugs that were there before BTW.
No problem at all (even if I wouldn't define those as bugs).
 
> > > > +        echo "$me: this test shouldn't be run as root"
> > > 
> > > Please >&2, several instances.
> > I used stdout for "consistency" with messages like:
> >   echo "$me: running $CC --version"
> >   echo "$me: running python -V"
> 
> But those aren't errors.  I know it's a close call for the above
> messages, but for error messages, stderr is definitely right.
> 
> > But I have no strong feelings on this matter, so I went along with
> > the ">&2" redirections throughout.
> 
> Thanks.
> 
> > > >      perl-threads)
> > > > -      # Skip with Devel::Cover: it cannot cope with threads.
> > > > -      test "$WANT_NO_THREADS" = yes && exit 77
> > > > +      if test x"$WANT_NO_THREADS" = x"yes"; then
> > > 
> > > no need to quote `yes', and in practice, no need for x prefixing either,
> > > I would guess.  Do you think we need to take care of malicious users?
> > No, it's just for consistency.  Because sometimes the idiom
> >   test x"$foo" = x"bar"
> > is indeed required, I prefer to use it everywhere, instead of asking myself
> > at every turn "do I need to care for white spaces in $foo here?" or "do I
> > need to care for leading hypens in $foo here?".  That's all.
> 
> Well, the quotes around bar are never required (except in Libtool and
> there only because out testsuite is too dumb to not flag the false
> positives).
True, but they make "stick out" what is supposed to be compared with $foo.
A moot point here anyway, since I've reverted this change and the other
similar ones.

> > Do toy want me to use:
> >   test $WANT_NO_THREADS = yes
> > anyway?
> 
> No.  You need to put double quotes around "$WANT_NO_THREADS", because it
> can be empty.
OK.
 
> The rest is not a big deal either way, but if you want to avoid asking
> yourself at every turn, then I just suggest not changing existing code
> at that turn unless you see a good reason.  ;-)
> 
> > > >      tex)
> > > >        # No all versions of Tex support `--version', so we use
> > > >        # a configure check.
> > > > -      test -n "$TEX" || exit 77
> > > > +      if test x"$TEX" = x; then
> > > 
> > > test -n is portable here and more concise, $TEX will never start with
> > > a '-' character or be equal to '=' for any sane user.  So let's please
> > > keep that.
> > Well, we should use `test -z' at least, since the semantic of the check
> > has been reversed.  Also, `test -z' can sometimes be problematic too, and
> > I tend to avoid it altogheter for the same "consistency" resons stated
> > above.
> > 
> > Do you want me to use `tezt -z' anyway here?
> 
> Sure.
Done (sweating and cringing throughout ;-)

> > > > @@ -285,6 +298,37 @@ do
> > > >    esac
> > > >  done
> > > 
> > > The rest of the patch from here on below seems to only transpose testing
> > > of $required and testing of some other variable.  In essence for the
> > > default case it turns one case statement into three (thus a minor
> > > slowdown), little code into more code, and I fail to see the advantage
> > > of the new ordering.  What is the intention here?
> > Giving precise resons about why the test was skipped.
> 
> Ah; that's a good reason.  Thanks.
Glad you're ok with this change.

> > > > -    case $testsrcdir,$testbuilddir in
> > > > -      *\ * | *\        *) exit 77;;
> > > > +      *' libtool '*|*' libtoolize '*)
> > > > +        if test x"$libtool_found" != x"yes"; then
> > > 
> > > The old code was perfectly well quoted: in
> > >   test $libtool_found = yes 
> > > you would reliably and helpfully get a shell error if $libtool_found was
> > > erroneously unset.  Also, we ensure that it could not start with '-'.
> > True, but see the "consistency" reasons stated above.  Do you want me to
> > revert to the  older (lack of) quoting anyway?
> 
> Yes, please.  Think of it as a check of your other changes in the code.
> ;-)
Done.

> Thanks, and please commit the patch when items are addressed,
Done, and merged to master.

Attached is the final version of the patch, JFTR.

Regards,
  Stefano

From f3a5d05517c8f60cc515c835cc15cd86e2b39abd Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Thu, 11 Nov 2010 14:49:39 +0100
Subject: [PATCH] Tests defs: improve messages for skipped tests.

* tests/defs: Give meaningful messages about the reasons of a
test skip; this is especially useful as this file is run without
verbose xtraces on.  Related reorderings in the code and new
comments.
---
 ChangeLog  |    8 ++++++
 tests/defs |   80 ++++++++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 72 insertions(+), 16 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 7d439bb..634a3ed 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2010-11-11  Stefano Lattarini  <address@hidden>
+
+       Tests defs: improve messages for skipped tests.
+       * tests/defs: Give meaningful messages about the reasons of a
+       test skip; this is especially useful as this file is run without
+       verbose xtraces on.  Related reorderings in the code and new
+       comments.
+
 2010-11-10  Stefano Lattarini  <address@hidden>
 
        Tests defs: move static definitions in a new file `defs-static'.
diff --git a/tests/defs b/tests/defs
index 50d074e..82c3b5b 100644
--- a/tests/defs
+++ b/tests/defs
@@ -192,6 +192,7 @@ do
       export GCJ
       echo "$me: running $GCJ --version"
       ( $GCJ --version ) || exit 77
+      echo "$me: running $GCJ -v"
       ( $GCJ -v ) || exit 77
       ;;
     g++)
@@ -228,11 +229,16 @@ do
       (echo foo >> $priv_check_temp) >/dev/null 2>&1
       overwrite_status=$?
       rm -f $priv_check_temp
-      test $overwrite_status = 0 && exit 77
+      if test $overwrite_status -eq 0; then
+        echo "$me: cannot drop file write permissions" >&2
+        exit 77
+      fi
       ;;
     perl-threads)
-      # Skip with Devel::Cover: it cannot cope with threads.
-      test "$WANT_NO_THREADS" = yes && exit 77
+      if test "$WANT_NO_THREADS" = "yes"; then
+        echo "$me: skip with Devel::Cover: cannot cope with threads" >&2
+        exit 77
+      fi
       ;;
     python)
       # Python doesn't support --version, it has -V
@@ -248,7 +254,10 @@ do
       (: > $ro_dir_temp/probe) >/dev/null 2>/dev/null
       create_status=$?
       rm -rf $ro_dir_temp
-      test $create_status = 0 && exit 77
+      if test $create_status -eq 0; then
+        echo "$me: cannot drop directory write permissions" >&2
+        exit 77
+      fi
       ;;
     rst2html)
       # Try the variants that are tried in check.am.
@@ -257,6 +266,7 @@ do
           echo "$me: running $r2h --version"
           $r2h --version && break 2
         done
+        echo "$me: no proper rst2html program found" >&2
         exit 77
       done
       ;;
@@ -264,13 +274,16 @@ do
       # DejaGnu's runtest program. We rely on being able to specify
       # the program on the runtest command-line. This requires
       # DejaGnu 1.4.3 or later.
-      echo "$me: running runtest --version"
+      echo "$me: running runtest SOMEPROGRAM=someprogram --version"
       (runtest SOMEPROGRAM=someprogram --version) || exit 77
       ;;
     tex)
       # No all versions of Tex support `--version', so we use
       # a configure check.
-      test -n "$TEX" || exit 77
+      if test -z "$TEX"; then
+        echo "$me: TeX is required, but it wasn't found by configure" >&2
+        exit 77
+      fi
       ;;
     texi2dvi-o)
       # Texi2dvi supports `-o' since Texinfo 4.1.
@@ -285,6 +298,37 @@ do
   esac
 done
 
+# Using just `$testbuilddir' for the check here is ok, since the
+# further temporary subdirectory where the test will be run is
+# ensured not to contain any whitespace character.
+case $testbuilddir in
+  *\ *|*\      *)
+    case " $required " in
+      *' libtool '* | *' libtoolize '* )
+        echo "$me: libtool/libtoolized cannot cope correctly" >&2
+        echo "$me: with spaces in the build tree" >&2
+        exit 77
+        ;;
+    esac
+    ;;
+esac
+
+# This test is necessary, although Automake's configure script bails out
+# when $srcdir contains spaces.  This is because $testsrcdir is in not
+# configure-time $srcdir, but is instead configure-time $abs_srcdir, and
+# that is allowed to contain spaces.
+case $testsrcdir in
+  *\ * |*\     *)
+    case " $required " in
+      *' libtool '* | *' libtoolize '* | *' gettext '* )
+        echo "$me: our testsuite setup cannot cope correctly with spaces" >&2
+        echo "$me: in the source tree for libtool/gettext tests" >&2
+        exit 77
+        ;;
+   esac
+   ;;
+esac
+
 # We might need extra macros, e.g., from Libtool or Gettext.
 # Find them on the system.
 # Use `-I $top_testsrcdir/m4' in addition to `--acdir=$top_testsrcdir/m4',
@@ -315,16 +359,20 @@ case " $required " in
       fi
     done
     case " $required " in
-      *' libtool '* | *' libtoolize '* ) test $libtool_found = yes || exit 77;;
-      *' gettext '* ) test $gettext_found = yes || exit 77;;
-    esac
-    # Libtool cannot cope with spaces in the build tree.  Our testsuite setup
-    # cannot cope with spaces in the source tree name for Libtool and gettext
-    # tests.  Using just `$testbuilddir' for the check here is ok, since the
-    # further temporary subdirectory where the test will be run is ensured not
-    # to contain any space.
-    case $testsrcdir,$testbuilddir in
-      *\ * | *\        *) exit 77;;
+      *' libtool '*|*' libtoolize '*)
+        if test $libtool_found != yes; then
+          echo "$me: libtool/libtoolize is required, but libtool.m4 wasn't" >&2
+          echo "$me: found in directories $aclocaldir $extra_includes" >&2
+          exit 77
+        fi
+        ;;
+      *' gettext '*)
+        if test $gettext_found != yes; then
+          echo "$me: gettext is required, but gettext.m4 wasn't found" >&2
+          echo "$me: in directories $aclocaldir $extra_includes" >&2
+          exit 77
+        fi
+        ;;
     esac
     ACLOCAL="$ACLOCAL -Wno-syntax -I $top_testsrcdir/m4 $extra_includes -I 
$aclocaldir"
     ;;
-- 
1.7.1