Re: [PATCH] yacc: extend and improve tests

[Stefano Lattarini]

On Saturday 08 January 2011, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> * Stefano Lattarini wrote on Sat, Jan 08, 2011 at 02:33:34PM CET:
> > Here are some testsuite enhancements that might help the on-going
> > work on Yacc support.  OK for a proper temporary branch off of
> > 'yacc-clean' (after having merged into it the older temporary
> > branch 'tests-lexyacc-extend' a.k.a. commit v1.11-249-gd4dcf50),
> > to be merged in master afterwards?
> 
> Sure.  You decide where and when to merge this best.
> 
> OK with nits below addressed.
>
I agree with most nits, and I've amended the patch accordingly,
just a couple of remarks below...

> Thanks!
> Ralf
> 
> > Subject: [PATCH] yacc: extend and improve tests
> > 
> > * tests/yacc-basic.test: Also check that the intermediate C file
> > is mentioned in the generated Makefile.in, and that it is created
> > by the first make invocation.
> > * tests/yacc.test: Test removed, it has become obsolete now.
> 
> Is that true?  AFAICS the other tests all require 'bison' to be present,
> while this one does not.
>
True, but this might become less of a concern once we'll start to
require simply a generic "yacc" program rather than "bison" in the
testsuite; that said, we better wait for that to happen before
removing yacc.test.  Test re-inserted.

> [CUT]

> > --- /dev/null
> > +++ b/tests/yacc-d-basic.test
> > @@ -0,0 +1,157 @@
> > +#! /bin/sh
> > +# Copyright (C) 2011 Free Software Foundation, Inc.
> 
> If this supersedes yacc3.test, then does it not also derive from it?
>
Actually no; I've basically written it from scratch (with some help
from `yacc-basic.test'). 
> I think in that case it is prudent to add the copyright years from that
> file here as well.
> 
Given the rationale below, I've left the copyright year to be simply
`2011'.  Hope this ok.

> > +# Tests on basic Yacc support for when we have -d in YFLAGS, AM_YFLAGS
> > +# and maude_YFLAGS.
> 
> s/and/or/  ?
> 
> > +. ./defs || Exit 1
> 
> Doesn't this require bison?
>
Obviously yes; fixed. (Well, technically it requires yacc, but that's
for a follow-up thread ;-)

> > +
> > +# Even the generated header file is renamed when target-specific YFLAGS
> > +# are used.  This might not be the best semantic, but it has been in place
> 
> semantics
> 
I went for "behaviour" instead.

> > +# for quite a long time, so just go along with it for now.
> 
> Just out of curiosity: can you explain why this may not be good
> semantics?
>
Because the details of outfile renamings when per-object flags are
involved should be (as much as possible) an internal detail of
automake.  But in this case this detail must be exposed, since the
output header file might be #included in other files (such as in
the examples in this same testcase).  Not a big deal, but noting
this wart in a comment couldn't hurt either IMHO.

> > +sed 's/"parse\.h"/"zardoz-parse.h"/' foo/parse.y > baz/parse.y
> > +sed 's/"parse\.h"/"zardoz-parse.h"/' foo/main.c > baz/main.c
> > +
> > [CUT]
> > +done
> > +cd baz
> > +$MAKE echo-distcom
> > +$MAKE -s echo-distcom | grep '[ /]zardoz-parse.c '
> > +$MAKE -s echo-distcom | grep '[ /]zardoz-parse.h '
> 
> Did you try with some non-GNU make (e.g., heirloom or BSD make)?
>
Yes; NetBSD make (debian port) and Heirloom/Solaris make works
correctly, while FreeBSD make fails the distcheck with:
  ...
  ERROR: files left in build directory after distclean:
  ./foo/parse.h
  ./foo/parse.c
  ./bar/parse.h
  ./bar/parse.c
  ./baz/zardoz-parse.h
  ./baz/zardoz-parse.c
  *** Error code 1

There are failures in other tests with FreeBSD (and Solaris) make,
but I'd rather leave them in place as they expose real issues (no
point in sweeping the dirt under the rug to have a 100% testsuite
pass in any case, right?).

> > +cd ..
> > +
> > +$MAKE distdir
> > +ls -l $distdir
> > +test -f $distdir/foo/parse.c
> > +test -f $distdir/foo/parse.h
> > +test -f $distdir/bar/parse.c
> > +test -f $distdir/bar/parse.h
> > +test -f $distdir/baz/zardoz-parse.c
> > +test -f $distdir/baz/zardoz-parse.h
> > +
> > +# Sanity check on distribution.
> 
> s/on //
>
Really? Sounds weird to me (but I made the edit anyway).

> > --- /dev/null
> > +++ b/tests/yacc-d-vpath.test
> > @@ -0,0 +1,112 @@
> > +#! /bin/sh
> > +# Copyright (C) 2011 Free Software Foundation, Inc.
> [CUT]
> 
> yaccvpath?
>
Yes.

> Is the RHS wrong on purpose here?
> 
No; fixed (BTW, it wasn't really wrong, just uselessly inconsistent).

> > --- /dev/null
> > +++ b/tests/yacc-dist-nobuild.test
> > @@ -0,0 +1,95 @@
> > +#! /bin/sh
> 
> > +# This test checks that dependent files are updated before including
> > +# in the distribution. `parse.c' depends on `parse.y'. The later is
> 
> .  The latter
> 
> > +# updated so that `parse.c' should be rebuild. Then we are running
> 
> rebuilt.  T
> 
> > +# `make' and `make distdir' and check whether the version of `parse.c'
> > +# to be distributed is up to date.
> > +
No, this was a copy & paste leftover.  Removed.

> > +# Check that distributed Yacc-generated parsers are not uselessly
> > +# remade from an unpacked distributed tarball.
> > +
This is the real comment for the test.

> > --- /dev/null
> > +++ b/tests/yacc-nodist.test
> > @@ -0,0 +1,103 @@
> 
> > +# Checks for .c and .h files derived from non-distributed .y sources.
> > +
> > +required=bison
> > +. ./defs || Exit 1
> > +
> > +set -e
> > [CUT]
> > +cat > sub1/Makefile.am << 'END'
> > +parse.y:
> > +   rm -f $@ address@hidden
> > +   { : \
> 
> Prepend ':;' to the command, please, for some bash version.
>
Darn, I used to know about this issue!  Sorry for the noise.

> > --- a/tests/yacc2.test
> > +++ b/tests/yacc2.test
> 
> copyright?
> 
Stupid oversight, sorry.

-*-*-

I will push soonish (tonight or tomorrow).

Thanks,
  Stefano