Re: [PATCH] {maint} tests: new subroutines for test skipping/failing

[Stefano Lattarini]

On Monday 17 January 2011, Ralf Wildenhues wrote:
> [ Cc:ing Jim because of ideas at the end ]
> 
> * Stefano Lattarini wrote on Sun, Jan 16, 2011 at 03:56:07PM CET:
> > This patch stemmed from this discussion:
> >  <http://lists.gnu.org/archive/html/automake-patches/2011-01/msg00144.html>
> > 
> > I'd like to have the patch applied to maint, to make eventual integration
> > of new tests easier.  But the follow-up patches converting the testsuite
> > to the use of skip_() and providing more informative messages for skipped
> > tests should go to master only, to avoid unecessary "merging churn".
> > 
> > OK?
> 
> Unfortunately not, for a couple of reasons.  I'm sorry I didn't think
> about it in more depth before, but there are several things that I think
> could be better with this patch.
> 
> First off, a legal one, quoting maintain.info:
> 
>   When you copy legally significant code from another free software
>   package with a GPL-compatible license, you should look in the package's
>   records to find out the authors of the part you are copying, and list
>   them as the contributors of the code that you copied.  If all you did
>   was copy it, not write it, then for copyright purposes you are _not_
>   one of the contributors of _this_ code.
> 
> So, if we copy the code, then Jim is the author of it.
> 
OK.

> Then, there are issues that the code exposes that I think we should
> address:
>
> > tests: new subroutines for test skipping/failing.
> > 
> > * tests/defs.in (Exit): Move definition of this function earlier.
> > (warn_, skip_, fail_, framework_failure_): New functions, inspired
> > to the homonyms in gnulib's tests/init.sh.
> > ($stderr_fileno_): New global variable, used by the new functions
> > above.
> > * tests/README: Updated.
> 
> > --- a/tests/README
> > +++ b/tests/README
> > @@ -100,6 +100,10 @@ Do
> >    Include ./defs in every test script (see existing tests for examples
> >    of how to do this).
> >  
> > +  Use the `skip_' function to skip tests, with a meaningful message if
> > +  possible.  Where convenient, use the `warn_' function to print generic
> > +  warnings, and the `fail_' function for test failures.
> 
> Why not document framework_failure_?
> 
Because it doesn't really fit into the setup pattern of automake tests: we
never really use complex setups (almost always, `cat', `echo' and maybe
`cp' are all that is used!), so there's little use for the function.  While
copying it in is OK, IMVHO there's no reason to document it ATM.

OTOH, having a generic `hard_error_()' or `hard_fail_()' function might be
much more useful ...  Maybe I might submit a patch to gnulib to have it
defined "upstream" first?  Even if I currently lack a copyright assignment
for gnulib, I guess that the patch should be small and obvious enough to
be classified as a "tiny change"...

> >    For tests that use the `parallel-tests' Automake option, set the shell
> >    variable `parallel_tests' to "yes" before including ./defs.  Also,
> >    use for them a name that ends in `-p.test' and does not clash with any
> 
> > --- a/tests/defs.in
> > +++ b/tests/defs.in
> 
> > @@ -88,6 +88,31 @@ echo "$PATH"
> >  # (See note about `export' in the Autoconf manual.)
> >  export PATH
> >  
> > +# Print warnings (e.g., about skipped and failed tests) to this file 
> > number.
> > +# Override by putting, say:
> > +#   stderr_fileno_=9; export stderr_fileno_; exec 9>&2; $(SHELL)
> > +# in the definition of TESTS_ENVIRONMENT.
> 
> That may be good advice in the context of gnulib.  However, it describes
> what is essentially a bug, or at least usability issue, in Automake:
> that the test author cannot write to the original stderr with the
> parallel-tests driver any more, and has to use a workaround which breaks
> user overrides of TESTS_ENVIRONMENT.
>
Note that I intended the above as a suggestion for the *user*, not for the
developer!  I know quite well that TESTS_ENVIRONMENT is a user-reserved
variable.

That said, I agree this might be seen as a usability issue.

> I think this should be addressed in the driver(s), ideally in a way that
> is both backward-compatible and allows the testsuite author to write
> identical code for the simple driver as for the parallel-tests driver.
> For example, am__check_pre could contain
>   $(TESTS_SETUP)
> 
> or even
>   $(AM_TESTS_SETUP) $(TESTS_SETUP)
>
I like this last proposal.  In fact, it has never struck me as particularly
clear or user-friendly that one might have to resort to TESTS_ENVIRONMENT
not only to define/extend the testsuite environment, but also a possibly
fully-fledged setup for the testsuite.
 
> before $(TESTS_ENVIRONMENT).  Then the developer could do
>   TESTS_SETUP = stderr_fileno_=9; export stderr_fileno_; exec 9>&2;
> 
> What do you think?
>
That's good, as long as the above is substituted with:
 AM_TESTS_SETUP = stderr_fileno_=9; export stderr_fileno_; exec 9>&2;

We don't really want to start invading the user namespace again,
right?  :-)

> I further wonder whether we should finally introduce
> $(AM_TESTS_ENVIRONMENT) and reserve the non-AM_ variable
> for environment settings for the user.
>
Definitely yes in my opinion.

> What do you think?
>
See above :-)

> > +# This is useful when using automake's parallel tests mode, to print
> > +# the reason for skip/failure to console, rather than to the .log files.
> > +: ${stderr_fileno_=2}
> > +
> > +warn_() { echo "$@" 1>&$stderr_fileno_; }
> > +fail_() { warn_ "$me: failed test: $@"; Exit 1; }
> > +skip_() { warn_ "$me: skipped test: $@"; Exit 77; }
> > +framework_failure_() { warn_ "$me: set-up failure: $@"; Exit 99; }
> 
> space before ()
>
Sorry, I thought we wanted to be closer to the original as possible.
At this point, we might as well go directly  with:

  warn_ ()
  {
    echo "$@" 1>&$stderr_fileno_
  }

etc.  WDYT?

Regards,
   Stefano