Re: [PATCH] {master} tests: add testcases sanity-checking the testsuite

[Stefano Lattarini]

On Thursday 24 February 2011, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> * Stefano Lattarini wrote on Wed, Feb 23, 2011 at 10:20:03PM CET:
> > Helper subroutines, variables and other pieces of code defined
> > in the `tests/defs' and used by many testcases are non-obvious,
> > and tricky to get to work portably; but until now, they weren't
> > tested at all in a clear and self-contained way.
> > This change should remedy to the situation.
> 
> This patch is fine with me, with some nits below.  I expect some
> fallout on some systems.
>
Me too, but that seems acceptable, as it won't introduce spurious
failures in the "real" tetcases.

> Pretty nifty stuff in this patch by the way.
> 
Thanks.

> What does the S_ prefix stand for?  Sanity?
>
Yes (or also "self-check").  It also ensures (when LC_COLLATE=C or
equivalent) that the new testcases are listed earlier by e.g. ls or
shell wildcarding, and that are run early, (mostly) before all the
other testcases.

> Well, we can keep (or lose) our sanity over all kinds of things.  ;-)
> How about 'defs-' as prefix, or maybe 'suite-'?
>
Weren't you the one fond of shorter test names? ;-)

Kidding aside, I'd rather keep the new testcases standing out more
clearly (as they do with the current `S_' prefix).  So for the moment
I haven't renamed them.  But feel free to advance other proposals, or
to override my decision if the current naming really bothers you.

> Thank you,
> Ralf
> 
> > * tests/S_cleanup.test: New test, check removal of temporary
> > test working directory by `./defs'.
> > * tests/S_dir.test: New test, check that tests using `./defs'
> > create a proper temporary directory, and run in it.
> > * tests/S_exit.test: New test, check that, in case of failing
> > commands, the correct exit status is passed to the exit trap
> > installed by the `./defs' script.
> > * tests/S_is_newest.test: New test, checking the `is_newest'
> > subroutine.
> > * tests/S_me.test: New test, checking that $me gets defined
> > automatically by `tests/defs' if not set, and that it can be
> > overridden from either the shell or the environment.
> > * tests/S_sanity.test: New test, check that the sanity checks
> > performed by the `tests/defs' script works correctly.
> > * tests/S_unindent.test: New test, checking the `unindent'
> > subroutine.
> > * tests/Makefile.am (TESTS): Update.


> > --- /dev/null
> > +++ b/tests/S_cleanup.test
> 
> > +# Sanity check for the automake testsuite.
> > +# Check creation/removal of temporary test working directory by `./defs'.
> > +
> > +. ./defs || Exit 1
> > +
> > +# We still need a little hack to make ./defs work outside automake's
> > +# tree `tests' subdirectory.  Not a big deal.
> > +sed "s|^testbuilddir=.*|testbuilddir='`pwd`'|" ../defs-static >defs-static
> > +cp ../defs .
> > +
> > +have_simlinks=false
> 
> typo symlinks here and in some but not all places below
> 
Oops *blush*.  Fixed.

> > +ln -s defs foo && test -h foo && have_simlinks=:
> 
> I'm not sure if this test works that way.
> MSYS emulates ln -s with cp -p, might also emulate test -h.
> You can find out for sure by something like this:
>   echo foo > a
>   ln -s a b
>   echo bar > a
>   grep bar b
> 
> But then again, I wonder, why you even care: on systems that emulate
> symlinks, you can just go along with that.  So I guess all you'd need
> to test here is whether ln -s apparently succeeds.
>
Agreed.

> > +export have_simlinks # Is used also by spawned shells.
>
> [CUT]
>
> > +# Check that pre-test cleanup do not unduly change the permissions of
> 
> s/do/does/
>
Fixed, sorry for the noise.

> > +# files to which symlinks in the temporary test directory point to.
> > +if $have_simlinks; then
>
> [CUT]
>
> > +
> > +fi # $have_symlinks
> > +
> > +# Check that the cleanup trap does not remove the temporary
> > +# test directory in case of test failure, skip, hard-error,
> > +# or upon the receiving of a signal.
> 
> or when receiving a signal.
>
OK.

> > +for bailout_command in \
> > +  'Exit 1' \
> > +  'Exit 2' \
> > +  'Exit 10' \
> > +  'Exit 77' \
> > +  'Exit 99' \
> > +  'Exit 126' \
> > +  'Exit 127' \
> > +  'Exit 255' \
> > +  'kill -1 $$' \
> > +  'kill -2 $$' \
> > +  'kill -9 $$' \
> > +  'kill -13 $$' \
> > +  'kill -15 $$' \
> > +; do
> > +  $SHELL -c  ". ./defs; : > foo; $bailout_command" dummy.test && Exit 1
> > +  test -f dummy.dir/foo
> > +done


> > --- /dev/null
> > +++ b/tests/S_exit.test
> 
> > +# Sanity check for the automake testsuite.
> > +# Check that. in case of failing commands, the correct exit status is
> > +# passed to the exit trap installed by the `./defs' script.
> > +# Also check that the `errexit' shell flag is active.
> > +
> > +for st in 1 2 3 4 5 77 99 126 127 128 129 130 255; do
> > +
> > +  echo "* Try: Exit $st"
> 
> ramping up output markup again?  ;->
>
This time it seems pretty innocent, and it helps to clearly distinguish
logs/traces from different subshell runs, IMHO without cluttering the
script's own log.  Excerpt:

 $ sh S_exit.test 
 * Try: Exit 1
 ~/src/automake/tests:...:/usr/sbin:/bin:/sbin
 ++ set -e
 ++ pwd
 /home/stefano/src/automake/tests/bash.dir
 + Exit 1
 + set +e
 + exit 1
 + exit 1
 + exit_status=1
 + set +e
 + cd /home/stefano/src/automake/tests
 + case $exit_status,$keep_testdirs in
 + test 0 '!=' 0
 + echo 'bash: exit 1'
 bash: exit 1
 + exit 1
 * rc=1

 * Try: sh -c 'exit 1'
 ~/src/automake/tests:...:/usr/sbin:/bin:/sbin
 ++ set -e
 ++ pwd
 /home/stefano/src/automake/tests/bash.dir
 + sh -c 'exit 1'
 + exit_status=1
 + set +e
 + cd /home/stefano/src/automake/tests
 + case $exit_status,$keep_testdirs in
 + test 0 '!=' 0
 + echo 'bash: exit 1'
 bash: exit 1
 + exit 1
 * rc=1

 * Try: Exit 2
 ~/src/automake/tests:...:/usr/sbin:/bin:/sbin
 ++ set -e
 ++ pwd
 /home/stefano/src/automake/tests/bash.dir
 + Exit 2
 + set +e
 + exit 2
 + exit 2
 + exit_status=2
 + set +e
 + cd /home/stefano/src/automake/tests
 + case $exit_status,$keep_testdirs in
 + test 0 '!=' 0
 + echo 'bash: exit 2'
 bash: exit 2
 + exit 2
 * rc=2

 ...

> [CUT]

 
> > --- /dev/null
> > +++ b/tests/S_is_newest.test
> 
> > +# Sanity check for the automake testsuite.
> > +# Check the `is_newest' subroutine.
> > +
> > +. ./defs || Exit 1
> > +
> > +: > a
> > +$sleep
> > +: > b
> > +: > c
> > +
> > +stat a b c || : # for debugging
> 
> stat is not portable.  You knew that already, I guess.
>
Yes, but on the other hand ls does not always have a useful timestamp
granularity; so having the above working on at least Linux and FreeBSD
is better than nothing.

> > +is_newest c a
> > +is_newest b a
> > +is_newest a b && Exit 1
> > +is_newest c b
> > +is_newest c c
> > +is_newest c a b c
> > +
> > +touch -r c d || Exit 77
> 
> Why the || Exit 77 here?
>
In case we encounter a non POSIX-compliant; but I agree it would be
better to handle that situation only if it really arises.  So I've
removed the `|| Exit 77'.

> > +stat c d || : # for debugging
> > +
> > +is_newest c d
> > +is_newest d c
> 
> You cannot be sure about the latter one.  See 'info Autoconf --index
> touch'.
>
Ouch (I didn't know that BTW, thanks for the pointer).  Latest line
removed.


> > --- /dev/null
> > +++ b/tests/S_sanity.test
> 
> > +# Sanity check for the automake testsuite.
> > +# Test the sanity checks performed by the `defs' script.  Also check that
> > +# it can be used by duplicating some of infrastructure of the automake's
> > +# tree `tests' subdirectory.
> 
> I have a hard time understanding this sentence.  How about
> 
>   Also check that we can use `defs' elsewhere, when we duplicate some
>   of the infrastructure from the automake/tests subdirectory.
>
Agreed.

> > +. ./defs || Exit 1
> > +
> > +# Avoid to confuse traces from children processed with our own traces.
> 
> Do not confuse traces from child processes ...
>
Sigh, again :-(  Fixed now.

> > +show_stderr()
> 
> space before paren
>
Oops.  Fixed, and sorry for the noise.

> > +{
> > +  sed 's/^/ | /' stderr >&2
> > +}
> > +
>
> [CUT]
>

I'll push in 72 hours if there are no further inputs or objections.

Thanks,
  Stefano