Re: [PATCH v4 1/3] parallel-tests: add auxiliary script 'pt-driver', refactor

[Ralf Wildenhues]

* Stefano Lattarini wrote on Thu, Jun 16, 2011 at 10:00:31AM CEST:
> This refactoring should cause no API of functionality change,
> and is meant only to simplify the future implementation of TAP
> and SubUnit testsuite drivers.  More precisely, our roadmap is
> to move most of the "testsuite driving" features out of the
> Automake-generated Makefiles, and into external scripts with
> well-defined interfaces.  This will allow the user to define
> its own personalized testsuite drivers, and will also offer us
> a framework upon which to implement our new TAP and SubUnit
> drivers, all in a very unobtrusive way and retaining an high
> degree of code reuse and backward-compatibility.

I generally like the direction this is taking.  The point of best
separation between which code goes into Makefile.in and which into
the driver scripts can be fine-tuned when we have more than one such
script.

Actually, yes, before deciding on this for real I really do want to see
a nontrivial other driver script.  There is no point in hardcoding
too much in several driver scripts if it all needs to be the same
anyway.

Please measure the time overhead your changes introduce into the current
code, for a trivial testsuite (say, 50 tests running 'true'), and a
nontrivial one like Automake's and one with faster tests.  Thanks.

I've already mentioned that I don't like the name of the script.  ;-)
(You could basically replace 'pt' everywhere else with 'test', too,
as the non-parallel test setup stuff does not allow for a driver either,
but that is not something we announce in all our names either.)

> * lib/pt-driver: New auxiliary script.
> * lib/Makefile.am (dist_SCRIPT_DATA): Add it.
> * automake.in (handle_tests): Require the new auxiliary script
> `pt-driver', and define new makefile variable `$(am__pt_driver)',

So this could just be $(am__test_driver).

> used to call it.  Perform new substitution on `DRIVER' when
> processing the `check2.am' file.
> * lib/check.am (am__tty_colors): Define new shell variable
> `$am__color_tests'.
> (am__rst_section): Removed, its role taken over by the new
> `pt-driver' script.
> (am__test_driver_flags): New variable, contains the command
> line options passed to `pt-driver'.
> (am__check_pre): Do not deal with temporary files and exit
> traps anymore, as the `pt-driver' script takes care of that now.
> Define shell variable `$am__enable_hard_errors', used by
> `$(am__test_driver_flags)'.  Reorder so that we don't need to
> save and restore the value of the `TERM' environment variable
> anymore.
> Other related adjustments.
> (am__check_post): Remove, as its role has been completely taken
> over by the `pt-driver' script.
> * am/check2.am (?GENERIC?%EXT%$(EXEEXT).log, ?GENERIC?%EXT%.log,
> ?!GENERIC?%OBJ%): Call the test script through the Automake
> substituted `%DRIVER%', and honor the command-line options
> in `$(am__test_driver_flags)'.  Do not call the obsoleted
> `$(am__check_post)' anymore.
> * tests/check.test: Adjust.
> * tests/check2.test : Likewise.
> * tests/check3.test : Likewise.
> * tests/check4.test : Likewise.
> * tests/check10.test: Likewise.
> * tests/color.test: Likewise.
> * tests/color2.test: Likewise.
> * tests/comment9.test: Likewise.
> * tests/dejagnu.test: Likewise.
> * tests/exeext4.test: Likewise.
> * tests/maken3.test: Likewise.
> * tests/maken4.test: Likewise.
> * tests/parallel-tests-interrupt.test: Likewise.
> * tests/posixsubst-tests.test: Likewise.
> * tests/repeated-options.test: Likewise.
> * tests/check-no-pt-driver.test: New test.
> * tests/parallel-tests-pt-driver.test: Likewise.
> * tests/Makefile.am (TESTS): Update.
> * NEWS: Update.


> index 054c62d..f3116c8 100644
> --- a/lib/am/check2.am
> +++ b/lib/am/check2.am

> @@ -17,7 +17,9 @@
>  ## From a test file to a log file.
>  ?GENERIC?%EXT%.log:
>  ?!GENERIC?%OBJ%: %SOURCE%
> -     @p='%SOURCE%'; $(am__check_pre) %COMPILE% "$$tst" $(am__check_post)
> +     @p='%SOURCE%'; $(am__check_pre) \
> +     %DRIVER% $(am__test_driver_flags) -- \
> +     %COMPILE% "$$tst"

Erm, was there not a need to keep post flags for redirection?  I must
confess I haven't gone through all gnulib threads from the last weeks
yet, but I vaguely remember related issues coming up there.

> --- /dev/null
> +++ b/lib/pt-driver
> @@ -0,0 +1,129 @@
> +#! /bin/sh
> +# pt-driver - basic driver script for the `parallel-tests' mode.

> +# Make unconditional expansion of undefined variables an error.  This
> +# helps a lot in preventing typo-related bugs.
> +set -u

Not sure how older shells handle this, but for development it should be
ok.  We haven't used it elsewhere.

> +print_usage ()
> +{
> +  cat <<END
> +Usage:
> +  pt-driver [--help|--version] --test-name=NAME --log-file=PATH
> +          [--expect-failure={yes|no}] [--color-tests={yes|no}]
> +          [--enable-hard-errors={yes|no}] [--] TEST-SCRIPT
> +The \`--test-name' and \`--log-file' options are mandatory.
> +END

echo "\
Usage ...

"?

(Old shells will create (and sometimes forget to remove) a temporary
file for this here-document, even if print_usage is never called.)

> +# TODO: better error handling in option parsing (in particular, ensure
> +# TODO: $logfile and $test_name are defined).
> +test_name= # Used for reporting.
> +logfile=   # Where to save the result and output of the test script.
> +expect_failure=no
> +color_tests=no
> +enable_hard_errors=yes
> +while test $# -gt 0; do
> +  case $1 in
> +  --help) print_usage; exit $?;;
> +  --version) echo "pt-driver $scriptversion"; exit $?;;
> +  --test-name) test_name=$2; shift;;
> +  --log-file) logfile=$2; shift;;
> +  --color-tests) color_tests=$2; shift;;
> +  --expect-failure) expect_failure=$2; shift;;
> +  --enable-hard-errors) enable_hard_errors=$2; shift;;
> +  --) shift; break;;
> +  -*) usage_error "invalid option: '$1'";;
> +  esac
> +  shift
> +done
> +
> +if test $color_tests = yes; then

--color-tests="ka boom"

> +tmpfile=$logfile-t
> +do_exit='rm -f $tmpfile; (exit $st); exit $st'
> +trap "st=129; $do_exit" 1
> +trap "st=130; $do_exit" 2
> +trap "st=141; $do_exit" 13
> +trap "st=143; $do_exit" 15
> +rm -f $tmpfile
> +
> +# Test script is run here.
> +"$@" >$tmpfile 2>&1

This will not print progress output while testing.  Hmm.  No change from
previous semantics, but I really would like to fix that eventually.
(I've suggested a similar improvement for Autotest basically.)

> +estatus=$?
> +if test $enable_hard_errors = no && test $estatus -eq 99; then
> +  estatus=1
> +fi
> +
> +case $estatus:$expect_failure in
> +  0:yes) col=$red; res=XPASS;;
> +  0:*)   col=$grn; res=PASS ;;
> +  77:*)  col=$blu; res=SKIP ;;
> +  99:*)  col=$red; res=FAIL ;;
> +  *:yes) col=$lgn; res=XFAIL;;
> +  *:*)   col=$red; res=FAIL ;;
> +esac
> +echo "${col}${res}${std}: $test_name"
> +echo "$res: $test_name (exit: $estatus)" | rst_section > $logfile
> +cat $tmpfile >> $logfile
> +rm -f $tmpfile

> --- a/tests/check4.test
> +++ b/tests/check4.test
> @@ -46,8 +46,15 @@ chmod +x ok.sh dir/fail.sh
>  
>  $ACLOCAL
>  $AUTOCONF
> -$AUTOMAKE
> +
> +if test x"$parallel_tests" = x"yes"; then
> +  $AUTOMAKE --add-missing
> +else
> +  $AUTOMAKE
> +fi

You can just call $AUTOMAKE --add-missing unconditionally.
After all, that's what users should do as well.
Several instances.

>  ./configure --prefix "`pwd`/inst"
> +
>  $MAKE check >stdout && { cat stdout; Exit 1; }
>  cat stdout
>  grep 'FAIL: fail.sh' stdout

> --- a/tests/parallel-tests-interrupt.test
> +++ b/tests/parallel-tests-interrupt.test
> @@ -28,12 +28,20 @@ END
>  
>  cat > Makefile.am << 'END'
>  TESTS = foo.test
> -## Ugly, but required by foo.test.  See below.
> -TEST_LOG_COMPILER = echo $$$$ > pid && exec 9>&2 && $(SHELL) -x
> +## Provide more debugging info.
> +TEST_LOG_COMPILER = $(SHELL) -x
> +## Rut required by foo.test; see below.
> +AM_TESTS_ENVIRONMENT = 9>&2

Separate bugfix, separate patch?

>  END
>  
>  # This is hacky and ugly, but has the great advantage of avoiding us a lot
>  # of pain with background processes and related synchronization issues.

Does the comment still match the code it precedes?

> +cat - "$top_testsrcdir"/lib/pt-driver > pt-driver <<'END'
> +#!/bin/sh
> +echo $$ > pid
> +END
> +
>  cat > foo.test << 'END'
>  #!/bin/sh
>  exec 2>&9

Cheers,
Ralf