* 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). > + echo "$me: this test shouldn't be run as root" Please >&2, several instances. 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? > + 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? > + echo "$me: skip with Devel::Cover: it cannot cope with threads." no final period (several more instances below). 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. > + 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? 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 '-'. > + 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" > ;;
