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
