Re: testsuite failures when test scripts are run with zsh

[Ralf Wildenhues]

* Stefano Lattarini wrote on Sat, Oct 10, 2009 at 03:42:52AM CEST:
> At Friday 09 October 2009, Ralf Wildenhues wrote:
> > I'd shorten this to:
> >
> >     Use run_command throughout.
> I'd prefer to keep it a bit more verbose. I settled for this:
>   "Use new subroutine run_command instead of hand-crafted redirections
>    of stdout and/or stderr."
> Are you OK with this?

Sure.

> > Hmm, I see a few inconsistencies cropping up here.  First, we
> > already have AUTOMAKE_run.  It has slightly different syntax.  With
> > your patch, some automake invocations that capture output use
> > AUTOMAKE_run, while others use run_command.
> >
> > These inconsistencies should be resolved.  I'm fine with having all
> > automake invocations use AUTOMAKE_run.
> Don't you think this should be done in a separate patch?

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.

> > > +run_CMD ()
> > > +{
> > > +  # NOTE: all internal variables used here starts with the
> > > `_run' +  # prefix, to minimize possibility of name clashes with
> > > global +  # variables defined in user code.
> > > +  : 'entering run_CMD(): become quiet'
> > > +  set +x # xtrace verbosity stops here
> >
> > No need to comment the obvious, here as well as at the end of this
> > function; these 5 lines can be replaced with
> >      set +x
> Well, I thought that things were clearer the way I did, but if more 
> experienced developers find that comments to be too "patronizing" or 
> annoying, I think it's better to remove them.  I'd just like to keep 
> two comments:
>   set +x # xtrace verbosity temporarly disabled in `run_command'
> at the beginning, and:
>   set -x # reactivating temporarly turned-off xtrace verbosity
> at the end.  Objections?

Fine with me.

> > If you care about reusability of this function in a context where
> > you can't be sure whether xtrace is enabled at this point, you can
> > use something like this instead:
> >      case $i in *x*) run_xtrace=;; *) run_xtrace=:;; esac
> >      $run_xtrace set +x
> >      ...
> >      $run_xtrace set -x
> Well, ATM I'd prefer to keep the function simpler.  It can be made 
> more reusable and general later, if the need will arise.
> What do you think?

Sure.

> > > +  _run_evald_cmd="${_run_evald_cmd} || _run_exitcode=\$?"
> > > +  eval "${_run_evald_cmd}"
> >
> > Why not simplify these two lines to
> >      eval "${_run_evald_cmd}"
> >      run_exitcode=$?
> > and drop the _run_exitcode initialization above?
> This way, a failing `eval' would make the shell abort if `set -e' is 
> active.  Or am I mistaken?  Feedback needed.

Yeah, so how about
  if eval "${_run_evald_cmd}"; then
    run_exitcode=0
  else
    run_exitcode=$?
  fi

> > > +  if test x"${_run_mix_stdout_and_stderr}" = x"yes"; then
> > > +    echo "=== stdout and stderr"
> >
> > Instead of all the file boundary markers, can we just re-enable
> > xtrace here?  That makes the trace output look more similar to that
> > we get from commands not run through this function.  You can
> > reorder this chunk of code to be after the exit code handling to
> > not deal with xtrace twice.
> Well, I liked the `===' marker: it sticked out clearly without being 
> obtrusive.

The question is: why do you want it to stick out further than the rest
of the trace output?  I can't see any reason.

> But if you prefer a stricter consistency, I might just
> use 'echo "+ cat stdout" >&2' etc. instead of 'echo "=== stdout"' etc.

You can have that easier by just using wrapping the output in "set -x"/
"set +x".

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

I don't understand why you'd want to make run_command not exit in case
of failure, if case that was intentional.

BTW2, do you have access to a Solaris or OpenBSD box to test this
function with its shells?

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

> > >  # does not exit with STATUS.
> > >  AUTOMAKE_run ()
> > >  {
> > > -  expected_exitcode=$1
> > > +  _am_run_expected_exitcode=$1

> or is OK to use `am_run_expected_exitcode' instead
> of `_am_run_expected_exitcode'?

Sure.

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

> And another aside: I was about to modify also the file
> `tests/dejagnu-p.test', before remembering it is automatically
> generated (and, well, also before noticing it was a false positive).
> 
> I think it would be useful to make the autogoenerated files (tests/defs,
> tests/*-p.test, etc) readonly.  It could be done in a separate patch.
> What do you think?

Hmm, I'm a bit leery of making things read-only, but making sure the
files contain a "generated by ... " line near the top seems a good idea.

Thanks,
Ralf