From: SZEDER Gábor <szeder@ira.uka.de>
Date: Wed, 25 Mar 2015 02:53:52 GMT
Subject: Re: [PATCH 01/25] t/test-lib: introduce --chain-lint option
Message-ID: <20150325035352.Horde.wlh5PImm_EGVzNTHeSEb7g1@webmail.informatik.kit.edu>
In-Reply-To: <20150320100548.GA12543@peff.net>


Quoting Jeff King <peff@peff.net>:

> However, there are a number of places it cannot reach:
>
>  - it cannot find a failure to break out of loops on error,
>    like:
>
>      cmd1 &&
>      for i in a b c; do
> 	     cmd2 $i
>      done &&
>      cmd3
>
>    which will not notice failures of "cmd2 a" or "cmd b"

s/cmd b/cmd2 b/ ?

>  - it cannot find a missing &&-chain inside a block or
>    subfunction, like:
>
>      foo () {
> 	     cmd1
> 	     cmd2
>      }
>
>      foo &&
>      bar
>
>    which will not notice a failure of cmd1.

And inside subshells.  I think it's worth mentioning, too, because
subshells are used frequently when setting environment variables

   ( GIT_FOO=bar && export GIT_FOO && cmd1 && ... ) && test_cmp

or changing directory

   ( cd subdir && cmd1 && ... ) && test_cmp

I was wondering whether we could do better here with helper functions,
something along the lines of:

   # Set and export environment variable, automatically unsetting it after
   # the test is over
   test_setenv () {
       eval "$1=\$2" &&
       export "$1" &&
       # sane_unset, to allow unsetting during the test
       test_when_finished "sane_unset '$1'"
   }

   # Change to given directory, automatically return to current working
   # directory after the test is over
   test_cd () {
       test_when_finished "cd '$PWD'" &&
       cd "$1"
   }

With these the above examples would become

   test_setenv GIT_FOO bar && cmd1 && ... && test_cmp

and

   test_cd subdir && cmd1 && ... && test_cmp

which means increased coverage for --chain-lint.


Thanks for working on this.  I looked into this after seeing Jonathan's
patch back then, got quite far but never reached a chain-lint-clean
state, and only sent patches for the two most amusing breakages
(ddeaf7ef0d, 056f34bbcd).
I'm glad it's off my TODO list and I don't have to rebase a 1.5 year old
branch to current master :)

Gábor

>  - it only checks tests that you run; every platform will
>    have some tests skipped due to missing prequisites,
>    so it's impossible to say from one run that the test
>    suite is free of broken &&-chains. However, all tests get
>    run by _somebody_, so eventually we will notice problems.
>
>  - it does not operate on test_when_finished or prerequisite
>    blocks. It could, but these tends to be much shorter and
>    less of a problem, so I punted on them in this patch.
>
> This patch was inspired by an earlier patch by Jonathan
> Nieder:
>
>   http://article.gmane.org/gmane.comp.version-control.git/235913
>
> This implementation and all bugs are mine.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/README      | 10 ++++++++++
>  t/test-lib.sh | 16 ++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/t/README b/t/README
> index d5bb0c9..35438bc 100644
> --- a/t/README
> +++ b/t/README
> @@ -168,6 +168,16 @@ appropriately before running "make".
>  	Using this option with a RAM-based filesystem (such as tmpfs)
>  	can massively speed up the test suite.
>
> +--chain-lint::
> +--no-chain-lint::
> +	If --chain-lint is enabled, the test harness will check each
> +	test to make sure that it properly "&&-chains" all commands (so
> +	that a failure in the middle does not go unnoticed by the final
> +	exit code of the test). This check is performed in addition to
> +	running the tests themselves. You may also enable or disable
> +	this feature by setting the GIT_TEST_CHAIN_LINT environment
> +	variable to "1" or "0", respectively.
> +
>  You can also set the GIT_TEST_INSTALLED environment variable to
>  the bindir of an existing git installation to test that installation.
>  You still need to have built this git sandbox, from which various
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index c096778..50b3d3f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -232,6 +232,12 @@ do
>  	--root=*)
>  		root=$(expr "z$1" : 'z[^=]*=\(.*\)')
>  		shift ;;
> +	--chain-lint)
> +		GIT_TEST_CHAIN_LINT=1
> +		shift ;;
> +	--no-chain-lint)
> +		GIT_TEST_CHAIN_LINT=0
> +		shift ;;
>  	-x)
>  		trace=t
>  		verbose=t
> @@ -524,6 +530,16 @@ test_eval_ () {
>  test_run_ () {
>  	test_cleanup=:
>  	expecting_failure=$2
> +
> +	if test "${GIT_TEST_CHAIN_LINT:-0}" != 0; then
> +		# 117 is magic because it is unlikely to match the exit
> +		# code of other programs
> +		test_eval_ "(exit 117) && $1"
> +		if test "$?" != 117; then
> +			error "bug in the test script: broken &&-chain: $1"
> +		fi
> +	fi
> +
>  	setup_malloc_check
>  	test_eval_ "$1"
>  	eval_ret=$?
> --
> 2.3.3.520.g3cfbb5d