Re: testsuite failures when test scripts are run with zsh

[Stefano Lattarini]

The updated patch is still not complete, but i'd like to hear comments
and criticisms about the work-in-progress.  A "temporary" partial patch
is attached.

At Friday 09 October 2009, Ralf Wildenhues <address@hidden> 
wrote:
> Hello Stefano,
>
> [CUT]
>
> Thanks.  A few nits (and I hope you don't mind that I'm being
> fairly nit-picky below):

> Have you checked whether any of the changed testsuite files need
> updated copyright years?
I didn't.  Now the years should have been updated (I hope there's no 
mistake, as I used `grep'/`sed -i').

> The format of ChangeLog entries for GNU is described in the GNU
> Coding Standards (GCS).
> [CUT]
> The GCS further state to not use parentheses after function names.
I must admit that I just skimmed through the GCS, but never read them
completely and carefully.  I guess I'll have to.
In the meantime, I corrected the ChangeLog entry as you suggested
(except for a small difference, which is reported below).

> I personally don't like mixed-case function names; can we rename
> the thing to run_cmd, or even run_command instead?  I think
> AUTOMAKE_run etc were only done this way because they use
> $AUTOMAKE and the similarity was desired.
Agreed. Let's rename it `run_command'.

> So, in this case it would IMHO be sufficient to write something
> like:
>
>       Fix testsuite: avoid Zsh-related problems with `set -x'.
>       * tests/README: Describe Zsh 4.x `set -x' aka. `xtrace' issue
>       and workaround with run_command.
>         * tests/defs.in (run_command): New function, to be used for
>         commands where standard error needs to be captured.
>
> > +   * tests/acloca14.test:
> > +   * tests/acloca17.test:
>
> [...]
Better and clearer than mine.  Fixed.

> For such file lists, there are basically three possibilities to
> list them, according to GCS:
> ...
> 2)
>       * file1: Blurb.
>       * file2: Likewise.
>       ...
>       * fileN: Likewise.
>
Settled for this.

> >  Unset variable `TEST_LOG_COMPILER', that might cause spurious
> >   failures by leaking in the environment of the make processes
> > executed by the test scripts.  Updated the code which ensures
> >  Bourne-compatibility in Zsh, by adding a call to `setopt
> >  NO_GLOB_SUBST' (as done by autoconf 2.64).
> These changes should be in two separate patches.  Thanks.
You're right, I became sloppy and lazy after too much editing and 
tweaking, and I ended up cramming too many things in the patch.
I already separated these changes and sent them in two distinct mails
to the list.

> > * tests/version8.test:
> >     use new subroutine `run_CMD()' instead of hand-crafted stdout
> >     and/or stderr redirections; this, togheter with the changes in
> >     file `tests/defs.in', fixes misbehaviour w.r.t. zsh for (at
> >     least) the following tests:
> >       - aclocal8.test
> >      [...]
>
> 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?

> > --- a/tests/README
> > +++ b/tests/README
> > @@ -157,6 +157,29 @@ Do not
> >    reason, but at least it makes sure the original error is still
> >    here.)
> >
> > +  If you must run a program and later analize its stderr, do
> > *not* run it
>
> I'd shorten this whole description a lot.  Portability issues
> should be explained in sufficient detail in the portability section
> of the Autoconf manual, and we shouldn't repeat them in detail
> elsewhere, only briefly mention them (and maybe refer to the
> Autoconf manual).  That avoids fragmenting the documentation files
> in the long run.  Also, don't write what shouldn't be done, but
> what should be done, unless inferring the former from the latter is
> not obvious.  If possible, add a new maintainer-check rule in the
> toplevel that ensures that we follow the new guide line (this rule
> then checks what shouldn't be done!).
>
> For example, I'd merely write the following here:
>
>   To run a program and analyze its stderr, use the run_command
> function: run_command PROG [ARGS...]
>     grep $pattern stderr
>
> as 'info Autoconf "File Descriptors"' already describes the issue
> in question.
You're right.  I missed the relevant parts regarding Zsh in the autoconf
manual.  I applied your change, just adding one exemple where the stderr
is expected to be empty (this was the concrete case where the zsh "bug"
revealed itself).

> > --- a/tests/defs.in
> > +++ b/tests/defs.in
>
> [...]
>
> > @@ -396,26 +397,100 @@ is_newest ()
> >    test -z "$is_newest_files"
> >  }
> >
> > # run_CMD [-e STATUS] [-i FILE] [-m] [--] COMMAND [ARGUMENTS..]
> > # -------------------------------------------------------------
> > # Run the given COMMAND (can be an external commnds, a function
> I'd just elide the part in parentheses, BTW; that's expected.
Agreed.

> > # and with standard input taken from FILE if option `-i' is given
> What is -i needed for?  Why can't we just use
>   run_command command < input
> in that case?  AFAICS none of the other commands inside that
> function do anything with their standard input.
Well, frankly I don't remember why I added that `-i' option, and now I 
don't see why it should be needed.  Moreover, it's not used anywhere
in the test scripts.  So let's remove it.

> >          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 "IGNORE".
>
> Since 0 is the default expected STATUS, can we omit '-e 0' from
> tests, unless that serves to be stressed somewhere?
Yes, it's probably less obtrusive this way.  Changed as you suggested.

> 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?
[STILL NOT CHANGED IN THE ATTACHED PATCH]

> > +# 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' +# option is given.
> >
> >                      All the redirections are done without
> > triggering the +# zsh4 bug related to `-x' shell switch
> > (described in details in the +# tests/README file).
>
> I'd drop this sentence, too.
Yes, it's better to just place it (or an equivalent explanation) in 
tests/README. Sentence dropped.

>
> > +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?

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

>
> > +  _run_stdin=-
> > +  _run_expected_exitcode=0
> > +  _run_mix_stdout_and_stderr=no
>
> I don't see a need to use underscore-prefixed variables.  We
> control both the defs.in file as well as all files that use it. 
> IMHO a run_ prefix is sufficient.
Agreed.

> But please don't rename _run_cmd
> below to run_cmd if that's also the function name (some shells
> don't have separate name spaces for functions and variables).
Since I renamed the function as `run_command', I'd go with the name
`run_cmd' for the variable.

> Missing function-scope variable names are a problem not really
> solved well by underscores: as soon as you have more than one level
> of functions, you'd need yet another way to avoid clashes.  IOW:
> let's just not use the ugly notation in the first place.  Thanks.
Agreed.

>
> > +  while test $# -gt 0; do
> > +    case "$1" in
>
> No need to quote the argument after 'case'.
OK, fixed.

>
> > +      -e) _run_expected_exitcode=$2; shift;;
> > +      -i) _run_stdin=$2; shift;;
> > +      -m) _run_mix_stdout_and_stderr=yes;;
> > +      --) shift; break;;
> > +      -?) echo "run_CMD(): invalid switch '$1'" >&2; Exit 99;;
>
> Please use \` for left quotes in messages.  I know this is ugly,
> but inconsistent usage is even uglier, so a change should be done
> across the board, and when unicode or similar can be assumed.
OK, fixed.

>
> > +       *) break;;
> > +    esac
> > +    shift
> > +  done
> > +  case $# in
> > +    0) echo "run_CMD(): missing COMMAND argument" >&2; Exit 99;;
> > +    *) _run_cmd=$1; shift;;
> > +  esac
> > +  _run_exitcode=0
> > +  if test x"${_run_mix_stdout_and_stderr}" = x"yes"; then
>
> How come you're starting to mix $foo and ${foo} style here?
> Several instances below.
Basically, I prefer `$foo' over ${foo}, but I find the form `$_foo' 
really ugly (note the leading underscore): in that case I really 
prefer ${_foo}.  But this won't be a problem anymore, as I took your 
suggestion and removed leading underscores from variable names.

> > +    _run_evald_cmd='"${_run_cmd}" ${1+"$@"} >stdall 2>&1'
> > +  else
> > +    _run_evald_cmd='"${_run_cmd}" ${1+"$@"} >stdout 2>stderr'
> > +  fi
> > +  if test x"${_run_stdin}" != x"-"; then
> > +    _run_evald_cmd="${_run_evald_cmd}"' <"${_run_stdin}"'
> > +  fi
> > +  _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.
[STILL NOT CHANGED IN THE ATTACHED PATCH]

> > +  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.  But if you prefer a stricter consistency, I might just
use 'echo "+ cat stdout" >&2' etc. instead of 'echo "=== stdout"' etc.
Note that for now I left the `===' markers in place.  Feedback needed.
[STILL NOT CHANGED IN THE ATTACHED PATCH]

> > +    cat stdall
> > +    echo "==="
> > + else
> > +    echo "=== stderr" >&2
> > +    cat stderr >&2
> > +    echo "===" >&2
> > +    echo "=== stdout"
> > +    cat stdout
> > +    echo "==="
> > +  fi
> > +  case ${_run_expected_exitcode} in
> > +    VOID|void|IGNORE|ignore|IGNORED|ignored|${_run_exitcode})
> > +      _run_rc=0
> > +      ;;
> > +    FAIL|fail|FAILURE|failure)
> > +      test ${_run_exitcode} -gt 0 && _run_rc=0 || _run_rc=1
>
> Please make this
>    if ... ; then run_rc=0; else ... ; fi
> as this may run with `set -e' set, and I'm not sure whether some
> broken shell may err-exit here otherwise.  (See the Autoconf manual
> for details.)
OK, fixed.

> > +      ;;
> > +    *)
> > +      _run_rc=1
> > +      ;;
> > +  esac
> > +  set -x # xtrace verbosity restart here
> > +  : "exit status ${_run_exitcode} (expecting
> > ${_run_expected_exitcode})" +  : "leaving run_CMD()"
> > +  return ${_run_rc}
> > +}
> >
> >
> >  # 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.
[STILL NOT CHANGED IN THE ATTACHED PATCH]

> >  # does not exit with STATUS.
> >  AUTOMAKE_run ()
> >  {
> > -  expected_exitcode=$1
> > +  _am_run_expected_exitcode=$1
> Why?
For pseudo-consistency with `run_command'.  Do you prefer this change 
removed completely, or is OK to use `am_run_expected_exitcode' instead
of `_am_run_expected_exitcode'? Feedback needed.
[STILL NOT CHANGED IN THE ATTACHED PATCH]

> >    shift
> > -  exitcode=0
> > -  $AUTOMAKE ${1+"$@"} >stdout 2>stderr || exitcode=$?
> > -  cat stderr >&2
> > -  cat stdout
> > -  test $exitcode = $expected_exitcode || Exit 1
> > +  run_CMD -e ${_am_run_expected_exitcode} -- $AUTOMAKE ${1+"$@"}
> > || Exit 1
>
> This change needs to be listed in the ChangeLog entry.
Done.

> In some of the tests, you use `test -s FILE || Exit 1', while in
> others, you use plain `test ! -s FILE'.  If you use the latter, or
> any other command that you'd like to fail the test with nonzero
> status, you have to ensure that the test has enabled `set -e'
> earlier on, or use the former variant.
Good catch.  Luckily, it seems that the only test scripts using
`test -s' or `test ! -s' and which also don't have `set -e' enabled
are:
  
  $ grep 'test.*-s' $(grep -L '\<set  *-e\>' tests/*.test)
  tests/acsilent.test:test -s stdall && Exit 1
  tests/dejagnu-p.test:# Test to make sure dejagnu ...
  tests/dejagnu.test:# Test to make sure dejagnu ...
  tests/dejagnu7.test:# Check that "make check" fails, when ...
  tests/dejagnu7.test:runtest --help | $FGREP -e --status || Exit 77
  tests/mdate4.test:test -f sub/mdate-sh
  tests/unused.test:test ! -s stderr

which prunig the false positives boil down to:
   
   tests/acsilent.test   tests/unused.test

and neither of these two required a correction (the `test ! -s stderr'
is the last command in `unused.test', so that if it fails, the whole test case 
fails).

-*-*-*-

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 reduce the possibility of repeating mistakes like the one
you pointed out above (which was very small, but only for sheer luck).
Of course this propesed change would require to change also some
exising tests (those which don't use `set -e'), but this can be done
in a later patch.

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?

-*-*-*-

> Cheers, and thanks for your work on this,
> Ralf
Thanks for your review.  I hope this time I'm making fewer mistakes.

Regards,
    Stefano

Fix-testsuite-avoid-Zsh-related-problems-with-set-x.patch
Description: Text Data