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 02:52:06PM CET:
> > 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.
> 
> I have some nits below, and a couple of questions.
> 
> Thanks,
> Ralf
> 
> > --- 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 = 0; then
> 
> -eq seems more appropriate than = (more instances below).
OK.

> 
> > +        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 I have no strong feelings on this matter, so I went along with
the ">&2" redirections throughout.

> 
> Also, the message could be more precise, as it will also trigger on
> systems without unix style permissions.  How about the following:
>   $me: cannot drop file write permissions
> 
> and similar for ro-dir below?
Agreed.

> 
> > +        exit 77
> > +      fi
> >        ;;
> >      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.

Do toy want me to use:
  test $WANT_NO_THREADS = yes
anyway?

> 
> > +        echo "$me: skip with Devel::Cover: it cannot cope with threads."
> 
> no final period (several more instances below).
All fixed.

> I'd drop the 'it'.
> 
> > +        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 = 0; then
> > +        echo "$me: support of read-only directories is required"
> > +        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"
> >          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 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?

> > +        echo "$me: TeX is required, but it wasn't found by configure"
> > +        exit 77
> > +      fi
> >        ;;
> >      texi2dvi-o)
> >        # Texi2dvi supports `-o' since Texinfo 4.1.
> > @@ -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.
 
> Sorry for sounding grumpy, I may just be overlooking something here.
> 
> > +# 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"
> > +        echo "$me: with spaces in the build tree."
> > +        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"
> > +        echo "$me: in the source tree for libtool/gettext tests."
> > +        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 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?
 
> > +          echo "$me: libtool/libtoolize is required, but libtool.m4 wasn't"
> > +          echo "$me: found in directories $aclocaldir $extra_includes"
> > +          exit 77
> > +        fi
> > +        ;;
> > +      *' gettext '*)
> > +        if test x"$gettext_found" != x"yes"; then
> > +          echo "$me: gettext is required, but gettext.m4 wasn't found"
> > +          echo "$me: in directories $aclocaldir $extra_includes"
> > +          exit 77
> > +        fi
> > +        ;;
> >      esac
> >      ACLOCAL="$ACLOCAL -Wno-syntax -I $top_testsrcdir/m4 $extra_includes -I 
> > $aclocaldir"
> >      ;;

Regards,
   Stefano