Re: [PATCH] test-driver: optionally run in test directory

[Stefano Lattarini]

Hi Philipp.

On 10/25/2013 10:34 AM, Philipp A. Hartmann wrote:
> In case the actual TESTS are organised in sub-directories, some
> (potentially) required files are often located in the same
> directory.  Correctly loading such auxiliary data would require
> explicit environment handling in the tests.
> 
> Secondly, test scripts may produce intermediate files with
> non-unique names, leading to races and conflicts when running
> tests in parallel in the same (top-level) tests directory.
> 
> This patch adds an option '--change-dir={yes|no}' to the
> test-driver to change to the sub-directory of the given
> test script (directory component taken from the script
> name, if any) before running the script.
> 
> This option can then be passed via AM_LOG_DRIVER_FLAGS to
> the test driver invocation.
> 
> Notes:
>  * The dirname emulation is borrowed from `install-sh'.
>  * It should not have any effect in case of local filenames.
>  * The '$log_file' already contains the correct path and the
>    output redirection is not part of the sub-shell.
>
Honestly, I'm very reluctant to add more code to the test-driver
script for a feature that, as you noticed, is very easy to implement
(in an even more controlled and granular fashion) with environment
variables and/or simple LOG_COMPILER wrapper script.  In addition,
the custom test drivers support in automake (since 1.12) means that
you *can* easily use your own version of the script in your package,
without any need to patch automake directly.

Let me still do a review of your patch though; hopefully you can
benefit from it in case you decide to keep using the patched
script (instead of environment variables) in your project.

> Signed-off-by: Philipp A. Hartmann <address@hidden>
> ---
>  lib/test-driver | 46 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/test-driver b/lib/test-driver
> index 32bf39e..2724b18 100755
> --- a/lib/test-driver
> +++ b/lib/test-driver
> @@ -44,7 +44,8 @@ print_usage ()
>  Usage:
>    test-driver --test-name=NAME --log-file=PATH --trs-file=PATH
>                [--expect-failure={yes|no}] [--color-tests={yes|no}]
> -              [--enable-hard-errors={yes|no}] [--] TEST-SCRIPT
> +              [--enable-hard-errors={yes|no}] [--change-dir={yes|no}]
> +              [--] TEST-SCRIPT
>  The '--test-name', '--log-file' and '--trs-file' options are mandatory.
>  END
>  }
> @@ -56,6 +57,7 @@ log_file=  # Where to save the output of the test script.
>  trs_file=  # Where to save the metadata of the test run.
>  expect_failure=no
>  color_tests=no
> +change_dir=no
>  enable_hard_errors=yes
>  while test $# -gt 0; do
>    case $1 in
> @@ -64,6 +66,7 @@ while test $# -gt 0; do
>    --test-name) test_name=$2; shift;;
>    --log-file) log_file=$2; shift;;
>    --trs-file) trs_file=$2; shift;;
> +  --change-dir) change_dir=$2; shift;;
>    --color-tests) color_tests=$2; shift;;
>    --expect-failure) expect_failure=$2; shift;;
>    --enable-hard-errors) enable_hard_errors=$2; shift;;
> @@ -91,9 +94,44 @@ trap "st=130; $do_exit" 2
>  trap "st=141; $do_exit" 13
>  trap "st=143; $do_exit" 15
>  
> -# Test script is run here.
> -"$@" >$log_file 2>&1
> -estatus=$?
> +# Optionally switch to directory of test script before running it.
> +if test $change_dir = yes; then
> +  tstbin=`basename "$@"`
>
'basename' only take a single argument, so you should just use "$1"
here.  (Note that this is more a theoretical issue, since at the
moment there is no way the test-driver script will be passed more
than one non-option argument by the automake generated Makefile
rules).

> +  # Prefer dirname, but fall back on a substitute if dirname fails.
>
If you assume 'basename' exists, you could as well just assume
'dirname' exists as well.  And I don't think any system lacking
basename or dirname is worth supporting or worrying about today.

> +  tstdir=`
> +     (dirname "$@") 2>/dev/null ||
> +     expr X"$@" : 'X\(.*[^/]\)//*[^/][^/]*/*$' \| \
> +          X"$@" : 'X\(//\)[^/]' \| \
> +          X"$@" : 'X\(//\)$' \| \
> +          X"$@" : 'X\(/\)' \| . 2>/dev/null ||
> +     echo X"$@" |
> +         sed '/^X\(.*[^/]\)\/\/*[^/][^/]*\/*$/{
> +                s//\1/
> +                q
> +              }
> +              /^X\(\/\/\)[^/].*/{
> +                s//\1/
> +                q
> +              }
> +              /^X\(\/\/\)$/{
> +                s//\1/
> +                q
> +              }
> +              /^X\(\/\).*/{
> +                s//\1/
> +                q
> +              }
> +              s/.*/./; q'
> +  `
> +  # Test script is run here (in the directory of the script).
> +  (cd "$tstdir" ; "./$tstbin") >$log_file 2>&1
>
This will break in VPATH rules, since it will run in the source
directory rather than in the build directory -- and in VPATH
builds, the source directory is allowed to be read-only (this
actually happens in "make distcheck").  I don't know how much
adding proper VPATH support would complicate the implementation;
nor have I given it much thought honestly, since personally
I'd just go with environment variables (this is what Automake's
own testsuite does).

> +  estatus=$?
> +else
> +  # Test script is run here.
> +  "$@" >$log_file 2>&1
> +  estatus=$?
> +fi
> +
>  if test $enable_hard_errors = no && test $estatus -eq 99; then
>    estatus=1
>  fi
> 

HTH,
  Stefano