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

[Stefano Lattarini]

On Tuesday 18 January 2011, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Mon, Jan 17, 2011 at 10:55:55PM CET:
> > On Monday 17 January 2011, Ralf Wildenhues wrote:
> > > * Stefano Lattarini wrote on Sun, Jan 16, 2011 at 03:56:07PM CET:
> > > > +  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.
> 
> I'm not sure I understand:
> $ grep ' 99' tests/*
> 
> shows quite a few potential uses already in defs* and instspc-tests.sh.
>
But note that most of the uses of `[Ee]xit 99' in `instspc-tests.sh'
indicate usage errors or internal errors, not setup failures.

> > OTOH, having a generic `hard_error_()' or `hard_fail_()' function might be
> > much more useful ...
> 
> Can you please be more specific?  What would be the semantic difference
> of your hard_error_ from framework_failure_?  I don't see any.
>
No semantic difference, only a more generic error message, as in e.g.:

 hard_error_() { warn_ "$ME_: hard/internal error: $@"; Exit 99; }

OK, no big deal, but I'd rather be precise if possible.

> > > > +# 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.
> 
> Yes, and that's why we might want to have AM_TESTS_ENVIRONMENT as well.
>
In case I haven't made it clear before, I agree with this idea too.

> > > 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?  :-)
> 
> For the developer-reserved variable, yes, sorry for that typo above.
>
OK, then we agree.

> > > > +# 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?
> 
> I'm with Jim on this one, vertical compactness can be useful too.
>
Honestly, not being a huge fan of the GNU formatting standards, I'm
mostly with Jim too here :-)

I just had the impression that you preferred consistency in these
issues ...

> Cheers,
> Ralf
> 

Regards,
  Stefano