Re: [PATCH] Extend tests on AC_LIBOBJ and friends.

[Stefano Lattarini]

On Monday 13 December 2010, Ralf Wildenhues wrote:
> Hello Stefano,
> 
> * Stefano Lattarini wrote on Thu, Dec 09, 2010 at 09:41:46PM CET:
> > From 88c5437de865d8d5574d12c1ba6256743987efc0 Mon Sep 17 00:00:00 2001
> > From: Stefano Lattarini <address@hidden>
> > Date: Tue, 31 Aug 2010 15:14:29 +0200
> > Subject: [PATCH] Extend tests on AC_LIBOBJ and friends.
> > 
> > * tests/aclibobj.test: Removed, superseded by ...
> > * tests/libobj1.test: ... this new test, which runs autoconf,
> 
> Can we agree on refraining testsuite removals to the absolute bare
> minimum necessary, please?
>
This isn't a "real" removal, as the new test `libobj-basic.test' is
(or at least should be) a strict superset of the old `aclibobj.test'.
But the changes were pervasive enough that I preferred to label them
as a "remove old test, add new test".  Anyway ...

> I've mentioned before why they are harmful:
> comparing old and new testsuite results is much more painful with them.
>
... in light of this, it would maybe have been better just to rewrite the
`aclibobj.test' test without also changing its name; the ChangeLog entry
could have read:
  * tests/aclibobj.test: Rewritten to be more "semantic".
without losing in clarity and terseness.

I'll try to avoid such blunders in future.

> Also, one always risks lowering testsuite exposure.
>
This shouldn't be a risk here; see above.

> So removals should be limited to tests that are harmful in themselves
> only.  Renames have some of the same problems.
> 
> > --- a/tests/aclibobj.test
> > +++ b/tests/libobj15b.test
> 
> > @@ -14,36 +14,30 @@
> >  # You should have received a copy of the GNU General Public License
> >  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >  
> > -# Make sure AC_LIBOBJ and friends work.
> > +# Nonexistent source for AC_LIBSOURCE should cause Automake to fail.
> >  
> >  . ./defs || Exit 1
> >  
> > +set -e
> > +
> >  cat >> configure.in << 'END'
> >  AC_PROG_CC
> >  AC_PROG_RANLIB
> > +AC_LIBSOURCE([foobar.c])
> > +# FIXME: without a call to AC_OUTPUT, Automake does not fail when file(s)
> > +#        specified in AC_LIBSOURCE call(s) do not exist.  Should we fix
> > +#        this limitation?  Or is the current behaviour good enough?
> 
> Instead of adding FIXMEs to the tests, please prefer to ask the question
> on the list.
>
I might file a bug report on bug-automake about this issue, and then
amend these comments to refer to it.  WDYT?

> I know I could've just read the patch, but we've discussed
> before (and I think agreed upon) that FIXMEs do not work, unless you
> plan to fix it yourself soon afterwards.
>
In my defense, I wrote this patch before we agreed upon that.  I should
have been more careful when amending it, though.  Sorry about this.

> With regard to this particular issue: users are supposed to call
> AC_OUTPUT.  If they don't, tough luck for them.  Or vice versa: without
> AC_OUTPUT, you don't need to assume correct semantics in the testsuite.
>
I tend to agree with you here, but in case of erronous usages, I think
it would be better to warn regardlessly of other errors (such as a
forgotten AC_OUTPUT call, as in this case), if that's feasible at all.

> >  AC_OUTPUT
> >  END
> 
> > --- a/tests/aclibobj.test
> > +++ b/tests/libobj20c.test
> 
> > @@ -14,36 +14,40 @@
> >  # You should have received a copy of the GNU General Public License
> >  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >  
> > -# Make sure AC_LIBOBJ and friends work.
> > +# Test error reporting for AC_CONFIG_LIBOBJ_DIR.
> > +# See also sister tests `libobj20a.test' and `libobj20b.test' .
> >  
> >  . ./defs || Exit 1
> >  
> > +set -e
> > +
> >  cat >> configure.in << 'END'
> > +AC_CONFIG_LIBOBJ_DIR([libobj-dir])
> >  AC_PROG_CC
> >  AC_PROG_RANLIB
> > -AC_OUTPUT
> > +AC_LIBOBJ([foobar])
> >  END
> >  
> >  cat > Makefile.am << 'END'
> > +AUTOMAKE_OPTIONS = subdir-objects ## required for our setup to work
> 
> Bug: ## comment preceded by something other than whitespace
> (more than one instance of this).
> 
Thanks for spotting these; will fix in a follow-up patch.

Regards,
   Stefano