Re: [PATCH v2] Overhauled and modularized tests in `instspc.test'.

[Ralf Wildenhues]

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  <address@hidden>
> +2010-09-16  Stefano Lattarini  <address@hidden>

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 $@ address@hidden
> +     $(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"; \
> +       } > address@hidden
> +     $(AM_V_at)chmod a+rx address@hidden && mv -f address@hidden $@
[...]

> --- /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?

[...]