Re: [PATCH 2/5] Tests defs: new subroutine `skip' for test skipping.

[Stefano Lattarini]

On Saturday 15 January 2011, Ralf Wildenhues wrote:
> [ Jim, I have a question below ]
> 
> * Stefano Lattarini wrote on Sat, Jan 15, 2011 at 03:16:51PM CET:
> > On Saturday 15 January 2011, Ralf Wildenhues wrote:
> > > * Stefano Lattarini wrote on Mon, Nov 15, 2010 at 06:23:21PM CET:
> > > > Tests defs: new subroutine `skip' for test skipping.
> > > 
> > > I don't like having this patch alone.  If we switch to functions, then
> > > we should so wholesale, and at once, and fully.
> > >
> > OK, but the main point of this patch was *not* to introduce more functions
> > in the "testing framework", only to ensure that SKIP'd tests offer a more
> > informative message about the reasons of the SKIP [...]
> 
> Granted, but why stop with SKIP and disallow more informative FAILs?
> At least we can define the respective functions and use them in new
> tests.
>
OK, this makes sense.

> > I introduced the skip function only to reduce code duplication, since
> > using:
> >   skip "this test shouldn't be run on a win32-like system"
> > is clearer and less error-prone than having to resort to:
> >   echo "this test shouldn't be run on a win32-like system" >&2
> >   Exit 77
> 
> Sure.  But let's reap other benefits as well while we're at it.
>
Right.

> > > Also, there is
> > > precedent in {coreutils,gnulib}/tests/init.sh which has more features
> > > than this implementation and I think we should follow (and/or improve
> > > upon if that is not sufficient):
> [...]
> 
> > > Generally, when we change something, it is a good idea to avoid NIH.
> > > (That doesn't mean we have to change to remove existing NIH, but avoid
> > > new.  The point being that stuff that is new is untested and broken.)
> > >
> > Sorry, but I don't fully understand what you're requesting here.
> 
> Apologies for being a bit dense.
> 
> > You're stating that we should start using init.sh in Automake?  If yes,
> > I (mostly) agree (while the init.sh is not yet fully adequate for our
> > purposes, it shouldn't be diffucult to make it more flexible; and the
> > advantage given by code reuse would be obvious).  The main problem here
> > is that I lack a copyright assignment for Gnulib, so I can't help in
> > making init.sh flexible enough for our needs :-(
> > 
> > On the other hand, if you're stating that we should try to be as
> > init.sh-compatible as possible (in view of a possible future passage
> > to init.sh), I agree even more, and I could amend the patch to use
> > `skip_' instead of `skip'.
> > 
> > Or again, are you stating that our `skip' subroutine should be
> > implemented through (basically) a copy & paste from init.sh?
> 
> Well, I'm not sure if using gnulib's init.sh outright is a good goal
> right away.  We've had so much testsuite churn now, and many tests fail
> at the moment, that I'm tempted to call for a couple of weeks of "no
> testsuite changes except fixing FAILures".
> 
> Back to my original point though: even if we don't use init.sh right
> away, it is a good idea to make new code of ours similar to gnulib's,
> with the goal of being more mergeable at some point in the future.
> And while we do that, we can already fold back bugfixes to gnulib where
> appropriate.  As a first step, we can copy code from gnulib.
> 
> Jim, gnulib/tests/init.sh is GPLv3+ right now.  We would like to use
> code added/modified in commit v0.0-3988-g0ae8d63.  Do you mind if we
> copy that into Automake's testsuite setup, currently GPLv2+?  AFAICS
> you're the sole author of that and prior changes around these lines.
> 
> Oh, Stefano, can you please start the assignment process for gnulib?
>
OK, I will sonish (but I certainly don't need to remind you that the
process might take quite a long time anyway).

> If that is not possible, then I'll have to do these changes sometime.
> 
> > This
> > would be fine with me too (but might require a couple of preliminary
> > patches, such as reaming `$me' to `$ME_' in the testsuite, etc.).
> 
> Certainly not; a
>   ME_=$me
> 
> avoids that really useless churn; alternatively a s/ME_/me/g for the
> parts taken from init.sh.  Doing one of these is prudent for any merging
> of pending branches anyway.  A global s/me/ME_/g can be done later when
> we actually turn to use init.sh (and all branches using $me have long
> been dead).
>
OK, I spoke without having thought enough before.  Sorry.

> > > That said, I found minor issues in the proposed patch which I'm listing
> > > so you take measures to avoid them when redoing.  This kind of patch
> > > should really be written with a script, not manually.
> > >
> > Ah, but how can a script devise the correct, non-preexisting
> > messages for the various possible SKIPs?
> 
> Well, skip_ should cope with not being passed any message: it shouldn't
> output anything in that case.
>
I agree (my implementation does so, BTW).

> That's a change we could submit to gnulib.  (If gnulib wants to
> require a message, that can be done with maintainer checks.)
> 
> All in all, this calls for a three-step conversion (can be done in one
> commit, but avoids the sort of bug I found in the prior patch
> submission):
> 
> - s/Exit 77/skip_/g
> - check/fix any occurrences in here-documents manually:
>   /<</,/^E[NO][DF]/{ /skip_/p; }
> - edit some or all skip_ occurrences to add a short explanation
>   (less than (79 - strlen (": skipped test: ") - length of longest
>   test name) characters).
>
Agreed.  I'll repropose the patch as a three-step patch series
(maybe not right away).  BTW, at this point it might also be
based off of maint, right?  To make integration of new tests 
easier, and so.

Thanks,
  Stefano