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

[Ralf Wildenhues]

* 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.

> > > +        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).

> 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.

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.

> > > @@ -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.

> > > -    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.
;-)

Thanks, and please commit the patch when items are addressed,
Ralf