Re: [PATCH] {master} Extend, fix and improve tests on Lex and Yacc support.

[Ralf Wildenhues]

* Stefano Lattarini wrote on Thu, Dec 16, 2010 at 10:10:29PM CET:
> On Thursday 16 December 2010, Ralf Wildenhues wrote:
> 
> > > BTW, notice that I'm planning to further extend the Lex/Yacc tests
> > > and make them more "semantic", but that should be better done in a
> > > follow-up patch IMVHO.
> > 
> > If there is any way I can get you to not do any more of such
> > conversions: Please don't work even more on *these* tests.  They are
> > Good Enough[tm].
> >
> IMHO they're not.  See just below.
> 
> > The incremental gain from more change is not worth the additional
> > work from you nor review from me.
> > 
> > Actually, lex and yacc support has *several* *real* issues, with maybe
> > more than a dozen reported bugs in the last few years, many of them
> > unfixed.  See the list archives.
> >
> Yes, but I woldn't dare trying to modify the Lex/Yacc related code with
> the limited understanding I have of it, without having a *much* stronger
> testsuite than it's currently available.

The *current* tests are good enough for the current code.  How many bugs
have we found due to the testsuite frenzy in the last few months with
its 200+ patches?  I'd guess less than 5.  That's an awful signal to
noise ratio.  How much have you gotten to know the Automake code base
(outside of tests/) as a result of that?  I don't know, I cannot guess,
but normally one needs to work on code to get to know it really.

Why not enhance the testsuite as you go along fixing bugs or otherwise
improving code outside the testsuite?  That ensures that we progress.
Also, while working on the code, it is often clearer which semantics you
are possibly changing; then, we can expose them in the testsuite as we
stumble over them.

> > It would be really nice if somebody who knows their ways around
> > bison/yacc and flex/lex well
> >
> I'm not that somebody, unfortunately.

Don't give yourself up before starting.

> > (or is willing to learn)
> >
> Well, maybe I might *try* to be this somebody...  once I feel
> backed up by a strong-enough testsuite.

I don't believe this argumentation, for reasons stated above.

Of course good testsuite coverage doesn't ever fully make up for knowing
portability issues, semantics of tools, and searching history and
archives regularly.  That just comes with it.

> > > --- a/tests/defs.in
> > > +++ b/tests/defs.in
> > > @@ -113,6 +113,13 @@ do
> > >        echo "$me: running etags --version -o /dev/null"
> > >        ( etags --version -o /dev/null ) || exit 77
> > >        ;;
> > > +    flex)
> > > +      # Since flex is required, we pick LEX for ./configure.
> > > +      LEX=flex
> > > +      export LEX
> > > +      echo "$me: running flex --version"
> > > +      ( flex --version ) || exit 77
> > > +      ;;
> > 
> > I don't understand why the new 'required=flex' entry should be needed in
> > defs.in.  Any tests that fail without the defs.in change?
> > 
> Not for me, but since we do something similar for bison, I was just trying
> to be consistent.  Should I remove this hunk?

Yes, please.  We don't need unneeded code.

> > >  cat > foo.l << 'END'
> > >  %%
> > > -"END"   return EOF;
> > > +"GOOD"   return EOF;
> > >  .
> > >  %%
> > >  int
> > >  main ()
> > >  {
> > > -  while (yylex () != EOF)
> > > -    ;
> > > -
> > > -  return 0;
> > > +  if (yylex () == EOF)
> > > +    return 0;
> > > +  else
> > > +    return 1;
> > 
> > That's not "semantic" either.  One wouldn't write a lexer like that.
> >
> Yes, but this will have IMVHO a lower risk of hanging due to possible
> errors/blunders in other parts of the testcase.  Hanging which, BTW,
> would happen ...
> 
> > >  }
> > >  END
> > 
> > > +# Program should build and run.
> > >  $MAKE
> > > -echo 'This is the END' | ./foo
> > > -$MAKE distcheck
> > > +echo GOOD | ./foo
> > > +echo BAD | ./foo && Exit 1
> > 
> ... here.
> 
> I think we should keep the lexer my stupid but safer way.

Maybe; but *now*, that is a good time for adding a comment in main
above, because that is one bit of unobvious code.  (Unobvious is the
reason why the code does not look nor work like a normal lexer.)
OK with that fixed.

> > >  # Don't redefine several times the same variable.
> > > -cp Makefile.am Makefile.src
> > > +cp -f Makefile.am Makefile.src
> > 
> > Why should you need this change?  Generally, I don't see why you ever
> > need 'cp -f'.
> >
> I dimly remember that I used to think there were cp implementations which
> might prompt if stdout/stderr is a tty and the dest file exists, unless
> the `-f' option is used..

That is true, but when you run a test, there is no tty involved.
What am I missing?

> But I might be very mistaken here, and since
> you tell me the `-f' is useless ...

No.  It is useless *here*.

Thanks,
Ralf