Re: [PATCH] Modernize, improve and/or extend tests `colon*.test.

[Ralf Wildenhues]

Hi Stefano,

* Stefano Lattarini wrote on Fri, Jun 18, 2010 at 01:25:20PM CEST:
> At Thursday 17 June 2010, Ralf Wildenhues wrote:
> > Thanks, most of this is uncontroversial, but a couple of things I
> >  don't understand:
> > > --- a/tests/colon3.test
> > > +++ b/tests/colon3.test
> > > [CUT]
> > > -
> > > -# Makefile should depend on two.in.
> > > -echo Grep2
> > > -grep '^Makefile:.* \$(srcdir)/two.in' zardoz.in
> > > -# Likewise three.in.
> > > -echo Grep3
> > > -grep '^Makefile:.* \$(srcdir)/three.in' zardoz.in
> > > +$PERL -ne '
> > > +  s/\bzardoz\.(in|am)\b/zrdz.$1/g;
> > > +  print if /zardoz/;
> > > +' <zardoz.in >out
> > > +test ! -s out || { cat out; Exit 1; }
> > 
> > Your changes seem to have valued efficiency so far.  But calling
> > perl is bound to be more expensive than a couple of greps.
> Yes, but it's blazingly fast if compared with an aclocal/automake 
> call.

Sure, but you're not replacing an aclocal/automake call.  If those are
slow, then the solution is to make them faster, not to make the rest
slower.  ;-)

> > The change looks good to me, how come you went this way though?
> > Just curious.
> Because writing that perl script was waaay simpler than cooking up a 
> sed script or a sed/grep pipeline doing the same thing portably.  
> And even if that perl script isn't much efficient, it is very simple to
> understand IMO.

You may not agree but I find the perl script way harder to understand.
And it's not more correct in that it won't survive line wraps in the
Makefile either.

> Maybe you can think of a *simple* way to do that which doesn't involve 
> perl?  Just curious (no pun intended :-)

A shell function in defs.in 'extract_makefile_deps TARGET FILE' that
contains a sed script similar to extract_makefile_var that does what
you want, and respects backslash-newline?

Something like (mind the embedded TABs):

  sed -n '
    /^[ ]*TARGET[        ]*:/b doit
    /^[^:]\{1,\}[        ]TARGET[        ]*:/b doit
    /^[ ]*TARGET[        ][^:]*:/b doit
    /^[^:]\{1,\}[        ]TARGET[        ][^:]*:/b doit
    d
    :doit
    /\\$/{
      N
      s/\\\n/ /
      b doit
    }
    s/[^:]*://p'

I find this as easy (or as hard) to read as your proposed perl script,
and it is more correct, too.

> > > --- a/tests/colon5.test
> > > +++ b/tests/colon5.test
> > > [CUT]
> > >
> > > +cat > Makefile.am <<'END'
> > > +.PHONY: test
> > > +test:
> > > + case ' $(DIST_COMMON) ' in \
> > > +   *' $(srcdir)/Makefile.dep '*) exit 0;; \
> > 
> > Hmm.  The variable might just as validly list Makefile.dep, without
> > preceding `$(srcdir)/'.  That is really necessary only if the file
> >  is listed both as a prerequisite and as a target somewhere in the
> >  makefile.

> But why should we relax this test if it's working correctly?  After 
> all, Automake is expected to have full control in the generation of 
> the autotools-related rebuild rules, right?  And we can relax the test 
> in the future if the necessity arises.

Well, this is a common dilemma in testing: should the tester be allowed
to use insider information or not?  Sometimes the answer is yes: we may
really want to make sure something is not only the way the documentation
states, but the way it was implemented under the hood.

But other times that is not so clear: say, three years down the road,
somebody wants to change something in automake.in, do a factorization or
so.  This causes a test failure, due to a test like colon3 peeking at
internal details.  This typically causes much head scratching: was the
test bad before, or did we just introduce a bug with the patch?

Getting the right balance here is the most non-automatic part of
testsuite work.  Ideally, we want the testsuite to be as strict as
possible without ever peeking in details which do not matter for the
documented API semantics.  So that any change we make to automake.in
produces no testsuite failure if the change still provides the right
user API.


For a related example of this theoretizing: your various patches
introduce several greps for error message strings.  I have acked them,
mostly because I think it is a good idea, generally.  Now, a while ago
I started a patch series to add i18n to Automake.  Each new such grep
causes another test failure when the locale is not English.  Of course,
there is a simple remedy: set LC_ALL=C in defs.in, but also of course,
that cuts us out of a part of the code path our users are seeing.

In this particular example, the balance is easy to judge: if the
testsuite covers all error messages Automake can generate, we have an
easy way to automatically ensure all messages really are annotated for
translation if all annotated ones are translated (which gettext can
verify for me).  But I am still not sure whether to localize the grep
or override the locale, when it comes to merge the code.  Both have
merits.  At least in this case a test failure is easy to analyze.

> For the moment, I haven't changed this fragment.  Obviously, you are
> free to amend it yourself if you still think it's too strict.

Yes, the test is too strict that way.

> > > +sed '/@SET_MAKE@/d' <Makefile.in >Makefile.sed
> > > +$MAKE -f Makefile.sed SHELL=$SHELL test
> > 
> > I see this idiom is used a couple of times in the testsuite.  How
> >  come?
> NOTE: the following is just my opinion...  Absolutely no warranty :-)

> >  Because running ./configure is a bit more expensive than sed?
> Yes, in part (and sometimes it is indeed *much* more expensive).  And 
> also because, if one wants to run just a small subset of the Makefile's
> rules, or to check just a small subset of its variables, he doesn't 
> need all the prerequisites ./configure would look for (erroring out if 
> it fails to find any of them).  For example, the older versions of
> 'ansi.test' seemed to use the "sed hack" to do this.

OK.

Thanks,
Ralf

> --- a/tests/colon3.test
> +++ b/tests/colon3.test
[...]
> @@ -35,21 +36,24 @@ END
>  $ACLOCAL
>  $AUTOMAKE
>  
> -# We actually check several things here.
>  # Automake should have created zardoz.in.
>  test -f zardoz.in
>  
>  # The generated file should refer to zardoz.in and zardoz.am, but
> -# never just "zardoz".
> -echo Grep1
> -grep zardoz zardoz.in | $FGREP -v 'zardoz.in' | $FGREP -v 'zardoz.am' > O || 
> :
> -# We cat the output file so we see in when verbose.
> -cat O
> -test -z "`cat O`"
> -
> -# Makefile should depend on two.in.
> -echo Grep2
> -grep '^Makefile:.* \$(srcdir)/two.in' zardoz.in
> -# Likewise three.in.
> -echo Grep3
> -grep '^Makefile:.* \$(srcdir)/three.in' zardoz.in
> +# never just "zardoz".  Also, Makefile should depend on two.in and
> +# three.in.
> +$PERL -npe '
> +  # Take line wrapping into account.
> +  while(s/\\\n//)
> +  {
> +    my $x = <>;
> +    $x =~ s/^\s*//;
> +    $_ .= $x;
> +  }
> +  s/\bzardoz\.(in|am)\b/zrdz.$1/g;
> +' <zardoz.in >out