Re: [PATCH 1/6] Maint-check: check consistency of list of test scripts.

[Stefano Lattarini]

At Monday 04 January 2010, Ralf Wildenhues <address@hidden> 
wrote:
> Hello Stefano, and a happy new year everyone,
> 
> * Stefano Lattarini wrote on Sat, Dec 26, 2009 at 01:57:41AM CET:
> > This patch add a new maintainer-specific check:
> >   `maintainer-check-list-of-tests'.
> >
> > This check verifies that the tests listed in $(TESTS) correspond
> > to the test scripts on the filesystem, counting both the
> > "committed" (e.g. `aclibobj.test')  and "generated" (e.g.
> > `check-p.test') test scripts. Note that maintainer-check fails
> > after applying this patch, as some test scripts are missing from
> > $(TESTS).  This will be fixed by the next patch.
> 
> I often have uncommitted test files in automake/tests/ lying
>  around, for issues that are not fixed yet.  I don't really want to
>  move them away in order for `make maintainer-check' to pass. 
>  OTOH, I think it is fine to require that tests entered into
>  version control are listed in the makefile as well (they can be
>  marked XFAIL if they don't work yet).
I didn't think about this situation, which in fact complicates things
a bit.  The problem you pointed out could probably be solved by
substituting the `$(srcdir)/*.test' wildcard with something like
$(srcdir)/`cd $(srdir) && git ls-tree --name-only | grep '\.test$'`.

But this would further complicate the target's rules, and IMHO they're
already complicated enough.  So I ask: wouldn't it be better if you 
rename your uncommitted test scripts with a suffix different from
the "official" `.test' suffix (say using `foo.newtest' or `foo.t' instead
of `foo.test')?

Whether your decision will be, I suggest that, if a change must be 
done, we'll do it in a separate patch.

> If in a patch series, you first introduce code duplication and then
> clean it up, IMVHO it is better to introduce the cleanup right
>  away, or even first, and then add the new code (this concerns 5/6
>  and 6/6 of this series).
Of course you're right that refactoring should usually be done before
adding new features.  But since the code duplication we are talking
about was introduced by my own patches, obviously it couldn't be
cleaned up in a patch preceding them :-)

Speaking about the "introduction of the cleanup right away": I'd
rather keep the patches "new functionality" and "related refactoring"
separated, since this way we give a more faithful view of the way the
changes was concretely made, and IMHO also leave the possibility to
the reviewers of suggesting better refactorings, or other 
improvements.  WDYT?

>  In this particular case, the duplication
>  is still there after your cleanup; see lib/am/subdirs.am.
You're right.  But that duplication (between Makefile.am and 
lib/am/subdirs.am) was already there before my patches, so I didn't
feel compelled to remove it, especially because (as you say below)
doing it the right way would probably require an API extension,
which ATM goes beyond my abilities and my time's availability.
>  It is  probably a better idea to provide for a user API to extend
>  the list of recursive targets; especially, since the home-grown
>  copy  for the toplevel recheck rule inherits a bug since fixed in
>  the subdirs.am version.
FWIW (speaking only as an Autmake's *user* here), I'd find such a new 
API very useful.

Regards,
    Stefano