Re: testsuite failures when test scripts are run with zsh

[Ralf Wildenhues]

Hi Stefano,

* Stefano Lattarini wrote on Tue, Oct 13, 2009 at 04:24:38PM CEST:
> At Tuesday 13 October 2009, Ralf Wildenhues wrote:
> >
> > 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.
> I see your point.  It's OK with me to have `run_command' calling Exit 
> on failures, since (as you stressed) that's by far the most common 
> scenario.  However, we should then really add a similar subroutine 
> (say `run_redirect' -- tell me if you have a better name) which only 
> takes care of portably redirecting stdout/stderr of a command (and, 
> obviously, rewrite `run_command' implementation to advantage of the 
> new function).
> Is this OK with you?

This is making things too complicated for my taste.  Your run_command
already has an -e option.  If we're going to return the exit status
anyway from the function, then we wouldn't need that -e *at all*, we
could just return whatever status the command caused.

So if it pleases you better, then we can change it to either of the
following:

- '-e ignore' ignores the status of the command; run_command returns it;
- no '-e' just lets run_command return the status of the command, rather
  than checking the command against zero.

I prefer the first, but only on the grounds that more typing is done
only in the rare case.

> > If we really need to run some command where we need to ignore
> > the exit status, then we can still use
> >
> >   run_command '$command || :'
> This is wrong, as currently `run_command' do not `eval' its COMMAND.
> And the exit status of $command would be lost, which might not be what 
> we really want.

OK, sorry about that glitch.

> > 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.
> I don't understand. The `eval' used in the run_command implementation 
> is there just to provide a shortand: no argument passed by the caller 
> is ever eval'd (and this is the right thing to do, I think).

OK.

> > 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.
> Mmhh, I see another possible misunderstanding creeping in here.
> Better to clear it out, just to be absolutely sure.  What I was trying
> to say is that we should get rid of AUTOMAKE_run and AUTOMAKE_fails,
> not add ACLOCAL_run and ACLOCAL_fails (especially now that I'm going
> to follow your advice and make run_command use `Exit 1' on failures).
> Is this OK?

OK.

> > > > 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.
> This will become mostly a moot issue once run_command will call
> `Exit 1' on unexpected exit status.  Should I anyway use "FAIL" 
> instead of "fail"?

Either way is fine.

> > > > > 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.
> Well, even if we are going to make `set -e' the default, I think that 
> this change should be introduced with patch.

Do you mean "should be introduced with a separate patch"?  If yes, then
I agree.

> > 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.
> Or we could add proper code in `tests/defs.in', to make it re-execute 
> the test scripts with CONFIG_SHELL.  But this should be obviously done 
> in a distinct patch.

Yes.

> --- a/tests/defs.in
> +++ b/tests/defs.in
> @@ -395,26 +395,91 @@ is_newest ()
>    test -z "$is_newest_files"
>  }
>  
> +# run_command [-e STATUS] [-i FILE] [-m] [--] COMMAND [ARGUMENTS..]
> +# -----------------------------------------------------------------
> +# Run the given COMMAND with ARGUMENTS and fail if COMMAND does not exit
> +# with STATUS.  If status is "VOID" or "IGNORE", any exit value of the
> +# command is acceptable.  If STATUS is "FAIL", then any exit value of the
> +# command *but 0* is acceptable.  Default STATUS is `0'.
> +# Also, save standard output and standard error from COMMAND, by default
> +# respectively in files `stdout' and `stderr' (in the current directory),
> +# or togheter in the file `stdall' (in the current directory) if the `-m'

typo togheter

> +# option is given.
> +run_command ()
> +{
> +  set +x # xtrace verbosity temporarly disabled in `run_command'
> +  run_exitcode_expected=0
> +  run_mix_stdout_and_stderr=no
> +  while test $# -gt 0; do
> +    case $1 in
> +      -e) run_exitcode_expected=$2; shift;;
> +      -m) run_mix_stdout_and_stderr=yes;;
> +      --) shift; break;;
> +      -?) echo "run_commmand(): invalid switch \`$1'" >&2; Exit 99;;

s/()//

> +       *) break;;
> +    esac
> +    shift
> +  done
> +  case $# in
> +    0) echo "run_command(): missing COMMAND argument" >&2; Exit 99;;

Likewise.

> +    *) run_cmd=$1; shift;;
> +  esac
> +  if test x"$run_mix_stdout_and_stderr" = x"yes"; then
> +    run_evald_cmd='"$run_cmd" ${1+"$@"} >stdall 2>&1'
> +  else
> +    run_evald_cmd='"$run_cmd" ${1+"$@"} >stdout 2>stderr'
> +  fi
> +  if eval "$run_evald_cmd"; then
> +    run_exitcode_got=0
> +  else
> +    run_exitcode_got=$?
> +  fi
> +  if test x"$run_mix_stdout_and_stderr" = x"yes"; then
> +    set -x
> +    cat stdall
> +    { set +x; } 2>/dev/null
> + else
> +    set -x
> +    cat stderr >&2
> +    cat stdout
> +    { set +x; } 2>/dev/null
> +  fi
> +  case $run_exitcode_expected in
> +    VOID|void|IGNORE|ignore|IGNORED|ignored|$run_exitcode_got)
> +      run_rc=0
> +      ;;
> +    FAIL|fail|FAILURE|failure)
> +      if test $run_exitcode_got -gt 0; then
> +        run_rc=0
> +      else
> +        run_rc=1
> +      fi
> +      ;;
> +    *)
> +      run_rc=1
> +      ;;
> +  esac
> +  echo "run_command: exit status $run_exitcode_got (expecting" \
> +       "$run_exitcode_expected)"
> +  set -x # reactivating temporarly turned-off xtrace verbosity
> +  return $run_rc
> +}

Thanks,
Ralf