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

[Ralf Wildenhues]

[ 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.

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_?

>    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.

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)

before $(TESTS_ENVIRONMENT).  Then the developer could do
  TESTS_SETUP = stderr_fileno_=9; export stderr_fileno_; exec 9>&2;

What do you think?  I further wonder whether we should finally introduce
$(AM_TESTS_ENVIRONMENT) and reserve the non-AM_ variable for environment
settings for the user.

What do you think?

> +# 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 ()

Cheers,
Ralf