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
