Re: [PATCH] {tests-init} Tests defs: improve messages for skipped tests. (was: Re: [PATCH 5/5] Tests defs: improve messages for skipped tests.)
* 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 ma
[PATCH] {tests-init} Tests defs: improve messages for skipped tests. (was: Re: [PATCH 5/5] Tests defs: improve messages for skipped tests.)
On Thursday 11 November 2010, Stefano Lattarini wrote: > On Wednesday 10 November 2010, Ralf Wildenhues wrote: > > * Stefano Lattarini wrote on Wed, Sep 08, 2010 at 08:57:19PM CEST: > > > * tests/defs.in: 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. > > > > What happened to this patch in the new, rebased patch series? > Nothing; I just left it out, because my main concern in the new patch > series was to arrive quickly at the separation of defs/defs-static > which I need to write the patch optimizing the `instspc*.test' tests. > > > Was it already subsumed in some other series? > No; I'm going to rebase it, and repropose it as a stand-alone patch > soonish (after the present patch series is done with). And here it is. Regards, Stefano -*-*-*- 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(-) From 25a64f7f2065a45ceccaf4b51cadea08c0534fdd Mon Sep 17 00:00:00 2001 From: Stefano Lattarini 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 + + 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 Tests defs: move static definitions in a new file `defs-static'. diff --git a/tests/defs b/tests/defs index 50d074e..5ad18aa 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 = 0; then +echo "$me: this test shouldn't be run as root" +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 +echo "$me: skip with Devel::Cover: it cannot cope with threads." +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 +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 +# 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 '*
