Re: [PATCH] {master} Improve and extend tests on `:=' variable assignments.

[Stefano Lattarini]

On Monday 29 November 2010, Ralf Wildenhues wrote:
> Hello Stefano,
>
Hi Ralf.  Most of your objections are correct, and I'll apply the
patch with just the original nits addressed.  Still, I disagree with
you on a minor point; see below if you are interested.

> * Stefano Lattarini wrote on Mon, Nov 29, 2010 at 01:55:27PM CET:
> > On Monday 29 November 2010, Ralf Wildenhues wrote:
> > > * Stefano Lattarini wrote on Thu, Nov 25, 2010 at 02:37:28PM CET:
> > > > The attached patch is based off of maint, and intended for master.
> > > > OK to apply?
> > > 
> > > With nits addressed.
> 
> > > > --- a/tests/colneq2.test
> > > > +++ b/tests/colneq2.test
> > > > @@ -20,14 +20,23 @@
> > > >  
> > > >  set -e
> > > >  
> > > > +cat >> configure.in << 'END'
> > > > +AC_OUTPUT
> > > > +END
> > > > +
> > > >  cat > Makefile.am << 'END'
> > > >  t = a b c
> > > > -EXTRA_DIST = $(t:=.test)
> > > 
> > > Please leave the EXTRA_DIST line in.  It is something different if
> > > automake skips a normal variable containing this, and a variable that is
> > > special to automake.
> > > 
> > In truth, I was planning to add a new test about substitutions and
> > indirections in definition of "special" automake variables, as TESTS,
> > bin_PROGRAMS and the like; that's why I felt justified in removing the
> > line above.  But I ended up not completing and submitting that patch
> > because the patch queue was (and is) already too long.
> 
> Let's recap what happened: you sent a patch that takes away a (minor)
> bit of test coverage;
>
(as an aside: it's not so minor in my opinion)

> I approve the patch but ask you to keep that coverage in, now you update
> the patch with an unrelated new change whose applicability depends on
> completely different factors (namely deciding whether some behavior is
> desirable or not),
>
Why should its applicatibility depend on such a decision?  The testcase
just serves to expose the current behaviour explicitly, without telling
if it's desirable or not.  That could be decided at a later time, and
the testcase can be adjusted accordinlgy, if needed.  That's not a good
reason for not having the testcase from today.

On the other hand, as you notice below, such a testcase should have been
submitted in a follow-up patch.

> plus also a related new test coverage patch.

> This procedure is not helpful.  Instead of turning N pending patches
> into N+2 pending patches, why not turn it into N-1 pending patches.
> And THEN, send a new patch doing whatever else you wanted to do.  Make
> that independent.
>
> I approved the original submission with nits addressed, so why not
> just apply it with those nits addressed, then we can at least forget
> about that part of the story and concentrate on the rest only?
>
OK, you are absolutely right here.  Sorry for not having done that
right away.

> And keep making progress, instead of wasting both patch
> production time on your side and review time on my side?
> 
> > Now I have completed at least the testcase for EXTRA_DIST (see
> > attachement); so, OK to keep this change if that testcase is added
> > in a prior commit?
> 
> No.  Sorry.
> 

> > > Uh, oh.  Thin ice.  This is OK, but we gotta remember that ...
>
The new testcase would be a good reminder, BTW ;-)

> > > ... it won't work reliably if some of these variables are actually
> > > automake-set before they are overridden.
>

> > > automake generally orders all of its
> > > variable settings before all of the user ones (so the user ones are
> > > preferred).  When I override, e.g., libdir here, however, it doesn't
> > > get reordered to the user part.  I wonder whether that is a bug in
> > > automake.
> > >
> > I've added a new (xfailing) testcase to expose this bug/limitation.
> > OK to apply it?
> 
> No.  The above paragraph was meant as an incentive to think, dig, and
> find out whether this is a bug or not, and if it is, whether it is
> possible in general to fix it without harming other legitimate use cases
> or not.  The test case does not address that question.
>
Nor it intends to (but your observations here make even clearer that
the testcase should have been introduced in a separate patch).

> Not everything can be answered with a test case, and no, I don't want
> test cases that basically ask me whether the test case is right or
> wrong.
> 
I think some tests of this kind could be helpful anyway (but this debate
will have to wait for the proper thread -- I'm not doing the same mistake
twice in the same day ;-)

Regards,
   Stefano