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
