Re: testsuite failures when test scripts are run with zsh

[Ralf Wildenhues]

Hello Stefano,

* Stefano Lattarini wrote on Wed, Oct 07, 2009 at 08:01:51PM CEST:
> The master branch of the automake Git repository has advanced again 
> since I submitted the latest version of my patch.  In particular, a new
> test cases has been added, which required some editing to work smoothly
> with Zsh.

Alright.  Please send patches to the automake-patches list, that way
it's easier to find them again later.  I'm adding the automake-patches
list now; you can remove bug-automake from followups.

> Thus I rebased my patch against the latest master branch.  The updated 
> patch is attached.

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?

> +2009-10-07  Stefano Lattarini  <address@hidden>
> +
> +     Fix testsuite: avoid Zsh-related problems and glitches, especially
> +     those due to a Zsh "bug" w.r.t. `set -x'.
> +     * tests/README: Describe a bug (or at least a weird behaviour) in
> +     the way the 4.x versions of Zsh deal with the `-x' a.k.a. `xtrace'
> +     switch.  This bug  can unexpectedly break apparently flawless test
> +     scripts.  Describe also a workaround for it, involving the use of
> +     the new shell function `run_CMD()', defined in tests/defs.in (see
> +     below).
> +     * tests/defs.in: New subroutine `run_CMD()'.  Implementation of
> +     subroutine `AUTOMAKE_run()' rewritten to use `run_CMD()'.  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).

The format of ChangeLog entries for GNU is described in the GNU Coding
Standards (GCS).  While I try to be a bit more verbose in Automake's log
entries, esp. since we've moved to git, there is no need to repeat in
detail things that are documented also in the files that are changed.

The GCS further state to not use parentheses after function names.

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.

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:
[...]

For such file lists, there are basically three possibilities to list
them, according to GCS:

1)
        * file1, file2,
        file3, ...
        ...
        fileN-1, fileN: Blurb.

with lines wrapped at around column 72,

2)
        * file1: Blurb.
        * file2: Likewise.
        ...
        * fileN: Likewise.

3)
        All instances changed.

If most or all tests were affected, then I'd definitely go with (3), but
in any case you should have text following a semi-colon.

        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.

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

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


> --- 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 shell function or

typo commnds

I'd just elide the part in parentheses, BTW; that's expected.

> +# shell builtin) with ARGUMENTS (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.

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

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.

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

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

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

> +  _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.  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).

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.

> +  while test $# -gt 0; do
> +    case "$1" in

No need to quote the argument after 'case'.

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

> +       *) 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.

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

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

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

> +      ;;
> +    *)
> +      _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.

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

Why?

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

>  }
>  
>  # AUTOMAKE_fails [options...]
>  # ---------------------------
> -# Run Automake with OPTIONS, and fail if automake
> -# does not exit with STATUS.
> +# Run Automake with OPTIONS, and make the testcase FAIL if automake
> +# does not exit with status 1.
>  AUTOMAKE_fails ()
>  {
>    AUTOMAKE_run 1 ${1+"$@"}

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.

Cheers, and thanks for your work on this,
Ralf