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

[Stefano Lattarini]

On Thursday 16 September 2010, Ralf Wildenhues wrote:
> Hi Stefano,
Hi Ralf.  Thanks for persisting on this.

> * 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
That's what I tried at first, but unfortunately the maint branch lacks
support for silent-rules.  However, it might be possible that $(AM_V_GEN)
and friends behaves as no-op in maint, so I might be able to do the
rebase anyway. If I succeeds, I'll post the updated patch as a "FYI".
> 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.
I was starting to develop a roughly similar idea :-)
But as you said, that's for a follow-up patch.

> 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%.
Yes, keeping the modularity without the performance penalty would
definitely be a boon.  Thinking about it, your proposal would in fact
even *increase* the modularity!

> > --- 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.
Botched amending; fixed.
 
> > +   * tests/instspc-tests.sh: New script, fullfilling a double role:
> fulfilling
Fixed.

> >  +  code and test execution code are inevitably interwined.
> intertwined
Ditto.

> > --- 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?
Why? In "True VPATH Spirit", we generate tests in the builddir, not in
the srcdir.

> You could just kill off all directory components to be sure.
I haven't done this yet ATM.  But if you still think it will be safer,
I'll follow your judgement and do the amending.
 
> [...]
> 
> > --- /dev/null
> > +++ b/tests/instspc-tests.sh
 
> > +# This script fullfills a double role:
> fulfills
Fixed.
 
> 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.
OK, thanks. I'd really like to avoid another respinning, if not
absolutely necessary.

> > +#   1. It generates a Makefile.am snippet, containing the definintion
> definition
Fixed.
 
> > +# the test generation code and test execution code tend to be
> > +# inextricably coupled and interwined.
> intertwined
Ditto.
 
> > +set -e
> 
> [...]

You missed this:
>  else
>    echo "$0: empty action and no proper command line " >&2
>    exit 99
Extra trailing space in error message.  Fixed.

> 
> > +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
Oops.

> > +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.
I'd agree with you if the function were in, say, `tests/defs.in' (once the
queue of pending patches in the "tests-init" branch is flushed, you'll see
how nicely commented my `run_command' function is ;-).  But in the present
situation, a detailed explanation of the function would be overkill IMVHO.
Besides, the code in `define_problematic_string' is so ad-hoc that it's
difficult to meaningfully document it, and so short that it is in fact
self-explanatory.  WDYT?

> > 
> > 
> > +# 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?
Yes, nice catch.  Done.

> 
> [...]

Thanks,
   Stefano