Re: [PATCH 5/5] Improved test silent5.test.

[Ralf Wildenhues]

* Stefano Lattarini wrote on Sun, Apr 25, 2010 at 04:28:43PM CEST:
> At Sunday 25 April 2010, Ralf Wildenhues wrote:
> > > --- a/tests/silent5.test
> > > +++ b/tests/silent5.test
> > >
> > > @@ -116,7 +119,7 @@ do
> > >    $MAKE >stdout || { cat stdout; Exit 1; }
> > >    cat stdout
> > >    grep ' -c' stdout && Exit 1
> > > -  grep ' -o ' stdout && Exit 1
> > > +  grep ' -o' stdout && Exit 1
> > 
> > This test has become less instead of more strict; oversight or
> > purpose?
> Why it should be less strict? Now even a command like:
>   gcc -ofoo foo.o
> would make the test fail, while previously a command like:
>   gcc -o foo foo.o
> was required.

D'oh.  Sorry 'bout that, I misread the patch.

> > >  grep mv stdout && Exit 1
> On the contrary, this seems too much strict, since it would fail (with 
> GNU make at least) if the automake source tree is placed in a 
> directory whose name contains the `mv' substring.

Indeed.  Grepping for '^mv ' and ' mv ' should work though.

> The other 
> silent*.test tests might have a similar problem too.
> This should IMHO be addressed in a different patch series.  WDYT?

Well, fix this first then have the new patches be correct right away.
There is no point adding new code with known bugs.

> > > +  # Ensure a clean rebuild.
> > >    $MAKE clean
> > > +  rm -f foo5.c foo6.[ch] sub/baz5.c sub/baz6.[ch]
> > 
> > Hmm, I wonder if doing clean and rebuild with the same V flag (and
> > without removing generated sources in between) would be a sensible
> > addition on top of this: it might trigger a different set of rules
> > (as I think you may be able to see with heirloom make).
> Yes, more sanity checks wouldn't hurt I guess.
> Should I amend the patch, or are you going to do that yourself?

Feel free to amend.

> > > +  # Ensure a clean reconfiguration/rebuild.
> > >    $MAKE clean
> > >    $MAKE maintainer-clean
> > > +  rm -f foo5.c foo6.[ch] sub/baz5.c sub/baz6.[ch]
> > 
> > Wait, maintainer-clean should have removed all these files at this
> > point (and some of the other lex/yacc tests should have this
> >  tested, too).  Does that not work for you?
> No, it works just fine; but in the (unlikely) case of a failure, it 
> could cause false positives (or even false negatives!) in 
> silent5.test.

Well, but failure in case of a bug is a good thing, even if it is
drive-by failure.  At least for semantics that are known to work.

> Also, silent5.test is not supposed to test `maintainer-clean' w.r.t.
> Lex/Yacc;

No; but one general idea is that if you have a test for some specific
semantic X, then in tests where you don't check for X, you may assume
that generally, X just works.  OK?

> > Did you run 'make maintainer-check' on the series?
> *Blush*  Obviously no; and in fact `make maintainer-check' spuriously 
> fails with:

> I really think that the maintainer-checks should be made more 
> configurable esp. w.r.t. whitelists (but this is obviously material for 
> an unrelated patch series); for the moment, I just amended the patch 
> to avoid that spurious warning (updated patch is attached).

Looks better, thanks.  I'll wait for the mv issue above.

Cheers,
Ralf