Re: testsuite failures when test scripts are run with zsh

[Ralf Wildenhues]

Hi Stefano,

* Stefano Lattarini wrote on Sat, Oct 10, 2009 at 03:04:59PM CEST:
> At Saturday 10 October 2009, Ralf Wildenhues wrote:
> > I didn't make myself clear enough, sorry.  What I meant was that we
> > shouldn't change uses of $AUTOMAKE with redirections to
> > 'run_command $AUTOMAKE' when we also have AUTOMAKE_run.  We should
> > only use one of the two, and the latter is already used.  There is
> > no need to convert uses of $AUTOMAKE which do not have
> > redirections.

> I understand.  But what about instead susbstituting everywhere
> `run_command $AUTOMAKE' instead of `AUTOMAKE_run' and
> `run_command -e 1 $AUTOMAKE' instead of `AUTOMAKE_fails'
> (of course remembering to add a proper `|| Exit 1' in tests not
> using `set -e')?

First off, I think that run_command really should Exit when the command
does not produce the intended status.  It should not be necessary to do
  run_command -e 1 $command || Exit 1

That is much safer, and less maintenance.  If we really need to run some
command where we need to ignore the exit status, then we can still use

  run_command '$command || :'

or make this command another function or so.

Which brings me to the second inconsistent issue with this API: the
'eval'uation level of the command is part of the API.  This is important,
because when the absolute source and build directories contain white
space in the name (and Automake mostly works in this case now), we
should be doing the right thing.

Then to your question above: yes it is ok to replace all instances of
AUTOMAKE_run and AUTOMAKE_fails (there is no need to replace plain
$AUTOMAKE without redirection).

> If you think about it, the testsuite don't have `ACLOCAL_run' or
> `ACLOCAL_fails', but simply uses `run_command $ACLOCAL' and
> `run_COMMAND -e 1 $ACLOCAL'.
> By the way, this change should require a small change in
> `tests/README' too.
> If you agree with it, I think it should be done with a distinct patch.

Sounds good.


> > Yeah, so how about
> >   if eval "${_run_evald_cmd}"; then
> >     run_exitcode=0
> >   else
> >     run_exitcode=$?
> >   fi
> This seems better; however, are we sure that the value of `$?' is 
> reliable there?

Yes.

> > BTW, your run_command doesn't do what it advertizes to do: it
> > doesn't necessarily cause a test failure when it should, esp. when
> > a test doesn't use 'set -e'.
> I think that here there's a misunderstanding about the meaning of 
> `fail': you mean "the testcase fails", while I mean "the function 
> fails", i.e. it return a value != 0.  Do you have a rewording to 
> suggest to make things clearer?

"fail" is FAIL, and exit status != 0 is returning a nonzero exit status.

> > I didn't think of the need to Exit within run_command when I wrote
> > my previous review; it makes the issue moot of reordering output
> > and exit handling: output handling should come first.
> Sorry, but I failed to parse this sentence. Can you reformulate it in 
> a simpler way?

What I was trying to say here is what I wrote above in my first
paragraph of this message.

> > I don't understand why you'd want to make run_command not exit in
> > case of failure, in case that was intentional.
> Yes, it was intentional.  I think it provides more flexibility (which 
> is useful for example in the test `init.test',). Moreover, in the 
> usual case, the test scripts are run with the `-e'  flag on, so that an
> "uncatched" failing `run_command' call makes the testcase fail.
> In case `set -e' is not used, the testcase's writer should anyway be 
> prepared to add manually add `|| Exit 1' wherever deemed appropriate.
> So I'd prefer not to ever call `Exit' in run_command.  Objections?

Yes; see above.

> > > > >  # AUTOMAKE_run status [options...]
> > > > >  # --------------------------------
> > > > > -# Run Automake with OPTIONS, and fail if automake
> > > > > +# Run Automake with OPTIONS, and make the testcase FAIL if
> > > > > automake
> > > >
> > > > Why this change?  I'd drop it, likewise for the comment to
> > > > AUTOMAKE_fails.
> > >
> > > It seemed clearer: it's not the function that it's failing, but
> > > the whole testcase.
> >
> > OK, but please s/testcase/test/g, and the ChangeLog entry should
> > mention this ((AUTOMAKE_run): Update comment.).
> Done. Also added `(AUTOMAKE_fail): Updated comment.' to the ChangeLog.

Thinking about this again "make the test fail" is both bad English
and not any clearer than "fail".  But this API is going away anyway
when you replace it all with run_command, so this is all moot.

> > [CUT]
> > > By the way, your observation has made me think: wouldn't it be
> > > better to enable `set -e' in defs.in, so that all the test cases
> > > could have a more uniform environment?
> >
> > This would require an audit of all tests that currently don't use
> > set -e.  This needs testing on Solaris, OpenBSD, NetBSD, Tru64,
> > because of bugs and warts in their shell's implementation of
> > errexit.
> Unfortunately, I don't have access to any of those system, so I'll
> have to drop the ball on this.

I can test the final iteration of this patch.

BTW, now that we have TEST_LOG_COMPILER, and correctly unset it in
defs.in, too, we can set it in tests/Makefile.am and worry less about
shells like Solaris /bin/sh.  Of course, that would require us to warn
that running tests directly (i.e., without 'make' in-between), might
require to use a decent shell.

Cheers,
Ralf