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

[Stefano Lattarini]

At Wednesday 21 July 2010, Ralf Wildenhues wrote:
> > > 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.  ;-)
... but I think you got what I was truly meaning, didn't you? ;-)

> > > 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.
Really?  I'd never thought that possible, at least not for someone 
which knows perl quite well (like you do, I'd say :-)

> And it's not more correct in that it won't survive
> line wraps in the Makefile either.
Oh, but that was deliberate.  After all, if it's quite unpredictable 
wheter a line is going to be wrapped, it's also unpredictable how a 
line is going to wrapped, isn't it?  So, why bother where backslashes
and newlines are placed?

> > 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 don't want to seem overly argumentative, but... do you really see 
this as simple?  WOW!  (BTW, are `\n' and \{...\} truly portable in 
sed?  I'd have said "no" until a moment ago, but now I think I was 
most probably mistaken, since you are using them here).

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

And to summarize: are you going to keep the perl script anyway, are 
you going to amend the patch yourself, or should I amend it (and, in 
this last case, how)?
 
> > > > --- 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?
Yes, this is a valid concern.

> 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.
Agreed.  This would be more feasible and natural if we had unit tests 
for automake internals...  but, alas, ATM I don't see any way to 
introduce such tests without an *extensive* reorganization and 
refactoring of Automake code, which would probably (surely?) be way 
too much dangerous and time-consuming.  Oh well.

> 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.
Ouch.  I really hoped that Automake could have remained i18n-free
(yes, I have a strong dislike for i18n, esp. w.r.t. command-line 
tools: new added complexity to gain so little -- and to lose 
greppability of error and warning messages in the meantime... sigh).

> 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.
Another remedy would be not to introduce i18n in Automake... ;-)

> 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).
Sorry, I can't understand what you're saying here...

> 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.
Agreed.

So, should I amend the test myself?  If yes, is this ok?

  case ' $(DIST_COMMON) ' in
    *' $(srcdir)/Makefile.dep '*|*' Makefile.dep '*) exit 0;;
    ...

Or should I be even more lax, and do this?

  case ' $(DIST_COMMON) ' in
    *' Makefile.dep '*|*'/Makefile.dep '*) exit 0;;
    ...

Regards,
   Stefano