Re: [Ping PATCHES] {master} Optimize tests `instspc-*.test' for speed.

[Stefano Lattarini]

On Monday 14 February 2011, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> a while ago ...
> 
> * Stefano Lattarini wrote on Tue, Jan 25, 2011 at 06:38:50PM CET:
> >   <http://lists.gnu.org/archive/html/automake-patches/2010-11/msg00152.html>
> >   <http://lists.gnu.org/archive/html/automake-patches/2010-12/msg00006.html>
> 
> > OK for master?
> 
> Both are OK, with nits addressed.
> 
> Thanks,
> Ralf
> 
> > Subject: [PATCH 1/2] tests: optimize `instspc-*.test' for speed
> > 
> > After the split of `instspc.test' into various generated tests,
> > the running time of the testsuite has noticeably increased, since
> > all these new generated tests must run aclocal, autoconf and
> > automake, whereas previously they were run only once (at the
> > beginning of `instspc.test').  But luckily, since the new tests
> > share the same input files for the autotools, this situation can
> > be easily worked around (at the expenses of a slight increase of
> > complexity for the testsuite scaffolding).
> > 
> > * tests/instspc-data.test: New helper test, properly calling
> > the `instspc-tests.sh' script to generate input data for the
> > others `instspc-*.test' tests.
> > * tests/Makefile.am (TESTS): Add `instspc-data.test'.
> > ($(instspc_tests:.test=.log)): Depend on its log file.
> > (instspc-data.log): Depend on `instspc-tests.sh'.
> > * tests/instspc-tests.sh: Recognize new action `generate-data',
> > and use it to create hand-written and autotools-generated static
> > files shared by all the `instspc-*.test' tests.
> > When sourced by the `instspc-*.test' tests, use those previously
> > created files instead of recreating them from scratch.
> > (deindent, create_input_data): New subroutines.
> > Some other related changes and refactorings.
> 
> > [CUT]
> 
> > --- /dev/null
> > +++ b/tests/instspc-data.test
> 
> > +# Helper testcase which generate input data for the other test
> > +# `instspc-*.test'.  It basically delegates the work to the helper
> > +# script `instspc-test.sh'.
> 
> As an alternative to a helper testcase, this could also just be a helper
> script whose run is a prerequisite to the instspc*.log files.  That way
> you don't have a bogus test result.  E.g.:
> 
>   $(instspc_tests:.test=.log): instspc-tests.sh instspc-data.dir/.dirstamp
>   instspc-data.dir/.dirstamp:
>         srcdir=$(srcdir) $(SHELL) $(srcdir)/tests/instspc-setup
> 
> or so (with the script renamed to instspc-setup).  And mostlyclean-local
> removing instspc-data.dir.
> 
> You decide whether this is worthwhile.
>
I had already tried a similar approach in the first version of the patch:
 <http://lists.gnu.org/archive/html/automake-patches/2010-11/msg00152.html>
but IMHO that turned out to be slightly more fragile and more complex than
the current approach.  So I'd rather not go back there.

> > +# Ensure proper definition of $testsrcdir.
> > +. ./defs-static || exit 99
> > +test -n "$testsrcdir" || exit 99 # sanity check
> > +
> > +instspc_action=generate-data
> > +. $testsrcdir/instspc-tests.sh
> 
> > --- a/tests/instspc-tests.sh
> > +++ b/tests/instspc-tests.sh
> 
> > @@ -94,6 +99,91 @@ define_problematic_string ()
> >    esac
> >  }
> >  
> > +# Helper subroutines for creation of input data files.
> > +
> > +deindent ()
> 
> "unindent"?  Hmm.  Both sound weird, but maybe unindent is easier to
> read.
>
They're both fine with me, so I went for `unindent'.

> Might wanna have it in tests/defs?
>
Hmmm... maybe in a follow-up patch.  But then, IMHO, we would want
a smarter implementation, that doesn't undiscriminately strip away
any leading whitespace from every line.  Maybe it should look at
the first non-blank line to determine which amount of whitespace
to remove.  I.e., something like:

deindent > main.c <<EOF
  main()
    {
      return 0;
    }
EOF

should result in main.c containing:

main()
  {
    return 0;
  }

WDYT?

> > [CUT]
> >
> > @@ -189,102 +279,54 @@ if test x"$instspc_action" = x"generate-makefile"; 
> > then
> >    exit 0
> >  fi
> >  
> > -###  If we are still here, we have to run a test ...
> > -
> > -# We'll need the full setup provided by `tests/defs'.  Temporarly disable
> > +# We'll need the full setup provided by `tests/defs'.  Temporarily disable
> >  # the errexit flag, since the setup code might not be prepared to deal
> >  # with it.
> >  set +e
> >  . ./defs || Exit 99
> >  set -e
> >  
> > +# The directory set up by the `generate-data' action should contain all
> > +# the files we need.  So remove the other files created by ./defs.  And
> > +# check we really are in a temporary `*.dir' directory in the build tree,
> > +# since the last thing we want is to remove some random user files!
> > +test -f ../defs-static && test -f ../defs || Exit 99
> > +case `pwd` in *.dir);; *) Exit 99;; esac
> > +rm -f *
> > +
> > +if test x"$instspc_action" = x"generate-data"; then
> > +  # We must *not* remove the test directory, since its contents must be
> > +  # used by following dependent tests.
> > +  trap 'Exit $?' 0
> 
> trap '' 0
> is the portable way to undo the EXIT trap, if I remember correctly.
> You could also just set keep_testdirs, no?
>
Yes, that's cleaner and clearer.  Thanks for the suggestion, I've amended
the patch accordingly.

> > +  create_input_data
> > +  Exit 0
> > +fi
> 
> > Subject: [PATCH 2/2] tests: `instspc-*.test': do not create useless source 
> > file
> > 
> > * tests/instspc-tests.sh (create_input_data): Do not create
> > unused source file `source2.c'.
> 

I have pushed both the patches to master now.

Thanks,
  Stefano