Hi Stefano,
* Stefano Lattarini wrote on Thu, Sep 16, 2010 at 04:24:46AM CEST:
> Your objections, reasonings and suggestsions have been quite enlighting,
> and brought me to the understandanding that most of our problems were
> deriving by the juggling of "weird strings" ;-> between gen-instspc-tests,
> instspc.sh and the generated Makefile.in. Thus I've attempted a rewrite
> of the patch, where names and contents of the problematic strings are kept
> in a single, centralized place: a new "driver script" `instspc-tests.sh'.
Cool.
With nits below addressed, the patch is OK for master, but please commit
to a new branch off of maint and merge that to master (that way, I hope
we may have an easier time doing merge fixups from other testsuite
changes later). Thanks.
I have an idea for a followup patch by the way: have a prerequisite of
all the instspc-*test logs create instspc.dir/ with all the files in it,
and autotools already run. Then let each individual test create a build
directory in its own test directory, but reuse the source tree
../../instspc.dir/configure [...]
$MAKE ...
that way you can regain almost all of the newly-introduced overhead by
not running aclocal, autoconf, automake, more than once.
On my system, the overhead amounts to going from 1:20 to 3:48 (this
is just the instspc* tests), which amounts to a net increase of the
whole testsuite by, I'm guessing, 15%.
Cheers,
Ralf
> On Wednesday 15 September 2010, Ralf Wildenhues wrote:
> > * Stefano Lattarini wrote on Tue, Sep 14, 2010 at 02:15:59PM CEST:
> > > On Monday 13 September 2010, Ralf Wildenhues wrote:
[...]
> > > > > + echo '. ./defs || Exit 99 || exit 99'; \
> > > >
> > > > ;-)
> > >
> > > A similar tweaking (for all tests) is planned in my ongoing
> > > "Testsuite initialization refactoring". Do you want me to drop
> > > it in this patch and re-insert it when the refactoring branch is
> > > merged?
> >
> > No that's fine, I was just smiling at this. I don't think we need
> > the extra '|| exit 99' though: although it is more correct,
> Even if I must admit that still have to see a shell that doesn't
> abort by itself on a failed ". ./FILE" command... ;-)
Indeed. '.' is a special built-in utility, and Posix explicitly
documents that not finding the FILE causes the script to exit.
Thanks for the reminder.
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,4 +1,41 @@
> -2010-09-14 Stefano Lattarini <stefano.lattar...@gmail.com>
> +2010-09-16 Stefano Lattarini <stefano.lattar...@gmail.com>
This looks fishy, you seem to be modifying the date of a previous commit
entry, rather than adding a new one.
> + Overhauled and modularized tests in `instspc.test'.
> + The test `instspc.test' was way too big and fragile. Its running
> + time was very long. It also produced a log that was nearly
> + unreadable due to its length, making it very difficult to find
> + out the reason for failures.
> + Also, it was too much monolithic, with a single (maybe spurious)
> + failure in a corner case causing the whole test to fail (even if
> + everything worked as expected in the other 99% of cases).
> + The present change should solve these problems, by separating
> + `instspc.test' into many smaller, self-contained, auto-generated
> + tests.
> + * tests/instspc.test: Removed.
> + * tests/instspc-tests.sh: New script, fullfilling a double role:
fulfilling
> + 1. it generates a Makefile.am snippet `tests/instspc-tests.am',
> + containing the definition of a list of new tests which will take
> + over the older `instspc.test', and
> + 2. it is sourced by said generated tests with proper parameters
> + pre-set, to run the "meat" of the checks.
> + This apparent abuse is indeed required because the test generation
> + code and test execution code are inevitably interwined.
intertwined
> + * tests/Makefile.am ($(srcdir)/instspc-tests.am): Include this
> + snippet, which (among the other things) defines ...
> + (instspc_tests): ... this new macro, containing the list of the
> + newly generated `instspc*.test' tests, and ...
> + (instspc_xfail_tests): ... this new macro, containing the list
> + of the `instspc*.test' tests expected to fail.
> + ($(instspc_tests)): New rule, generates the `instspc*.test' tests.
> + ($(instspc_tests:.test=.log)): New rule, registers the dependency
> + of all `instspc*.test' tests on the `instspc-tests.sh' script.
> + (TESTS): Add `$(instspc_tests)', remove `instspc.test'.
> + (XFAIL_TESTS): Add `$(xfail_instspc_tests)'.
> + (EXTRA_DIST): Distribute instspc-tests.sh.
> + (MAINTAINERCLEANFILES): Added $(instspc_tests).
> + Other minor cosmetic changes.
> + * bootstrap: Generate instspc-tests.am.
> + * tests/.gitignore: Updated.
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> +$(instspc_tests): Makefile.am
> + $(AM_V_at)rm -f $@ $...@-t
> + $(AM_V_GEN) :; \
> + base=`expr x'$@' : x'instspc-\(.*\)\.test$$'`; \
I think $@ could contain $(srcdir) here, no? You could just kill off
all directory components to be sure.
> + name=`expr x"$$base" : x'\(.*\)-'`; \
> + action=`expr x"$$base" : x'.*-\(.*\)'`; \
> + { \
> + echo '#!/bin/sh'; \
> + echo '# DO NOT EDIT! GENERATED AUTOMATICALLY!'; \
> + echo; \
> + echo '# Ensure proper definition of $$srcdir.'; \
> + echo 'am_skip_defs=yes'; \
> + echo '. ./defs || exit 99'; \
> + echo 'test -n "$$srcdir" || exit 99 # sanity check'; \
> + echo; \
> + echo "instspc_test_name='$$name'"; \
> + echo "instspc_action='test-$$action'"; \
> + echo ". \$$srcdir/instspc-tests.sh"; \
> + } > $...@-t
> + $(AM_V_at)chmod a+rx $...@-t && mv -f $...@-t $@
[...]
> --- /dev/null
> +++ b/tests/instspc-tests.sh
> +# Driver script to generate and run tests checking that building from,
> +# or installing to, directories with shell metacharacters succeed.
> +#
> +# Original report from James Amundson about file names with spaces.
> +# Other characters added by Paul Eggert.
> +#
> +# This script fullfills a double role:
fulfills
Alternatively, you could have split the thing in three: a generator
script, a test run script, and the common parts in a scriplet sourced
by both. I'm OK with the way it is now, though.
> +# 1. It generates a Makefile.am snippet, containing the definintion
definition
> +# of proper lists of tests.
> +# 2. It is sourced by said generated tests with proper parameters
> +# pre-set, to run the "meat" of the checks.
> +# This setup might seem tricky and over-engineered abuse, but past
> +# (painful) experiences showed that it is indeed required, because
> +# the test generation code and test execution code tend to be
> +# inextricably coupled and interwined.
intertwined
> +set -e
[...]
> +elif test $# -gt 0; then
> + echo "$0: action specified and command line arguments used" >&2
> + exit 99
> +elif test x"$instspc_action" = x"generate-makefile"; then
> + :
indentation
> +else
> + case $instspc_action in
> + test-build|test-install)
> + if test x"$instspc_test_name" = x; then
> + echo "$0: test name undefined for action '$instspc_action'" >&2
> + exit 99
> + fi;;
> + *)
> + echo "$0: invalid action: '$instspc_action'"
> + exit 99;;
> + esac
> +fi
> +
> +# Helper subroutine for test data definition.
> +# Usage: define_problematic_string NAME STRING
again, this is veery nitty, and for the testsuite I don't really mind
all that much, but the GNU style comment for this function would be
something like this:
# define_problematic_string NAME STRING [FAILURE-MODE]...
# -------------------------------------------------------
# Register STRING in associative array instspc__<NAME> and NAME in
# instspc_names_list. Update instspc_xfail_builds_list and
# instspc_xfail_installs_list according to FAILURE-MODEs.
> +define_problematic_string ()
> +{
> + tst=$1
> + shift
> + eval "instspc__$tst=\$1" || exit 99
> + shift
> + instspc_names_list="$instspc_names_list $tst"
> + # Some of the "problematic" characters cannot be used in the name of
> + # a build or install directory on a POSIX host. These lists should
> + # be empty, but are not due to limitations in Autoconf, Automake, Make,
> + # M4, or the shell.
> + case " $* " in *' fail-build '*|*' build-fail '*)
> + instspc_xfail_builds_list="$instspc_xfail_builds_list $tst";;
> + esac
> + case " $* " in *' fail-install '*|*' install-fail '*)
> + instspc_xfail_installs_list="$instspc_xfail_installs_list $tst";;
> + esac
> +}
> +# Skip if this system doesn't support these characters in file names.
> +mkdir "./$instspc_test_string" || Exit 77
> +
> +mkdir sub1
> +
> +cat >> configure.in << 'EOF'
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +AC_OUTPUT
> +EOF
> +
> +mkdir sub
Collapse three mkdir calls into one, to save two (times 2 times 45)
processes?
[...]
