[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] {tests-init} Tests defs: improve messages for skipped tests.
From: |
Ralf Wildenhues |
Subject: |
Re: [PATCH] {tests-init} Tests defs: improve messages for skipped tests. |
Date: |
Thu, 11 Nov 2010 21:21:42 +0100 |
User-agent: |
Mutt/1.5.20 (2010-08-04) |
* 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