Re: [PATCHv5 01/16] test-lib: make test_expect_code a test command

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 3, 2010, 14:13 UTC
Message-ID: <20101003141339.GB17084@burratino>
In-Reply-To: <1286082644-31595-2-git-send-email-newren@gmail.com>

Elijah Newren wrote:

Show 14 quoted lines

> --- a/t/README
> +++ b/t/README
> @@ -482,6 +475,15 @@ library for your script to use.
>  	    'Perl API' \
>  	    "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl
>
> + - test_expect_code <exit-code> <git-command>
> +
> +   Run a git command and ensure that it exits with the given exit
> +   code. For example:
> +
> +	test_expect_success 'Merge with d/f conflicts' '
> +		test_expect_code 1 git merge "merge msg" B master
> +	'

Side note: this helper should be safe to use even for non-git commands. "Huh?" you might ask. "But test_must_fail and test_might_fail..." Well, the distinction is this: test_must_fail and test_might_fail rely on details of git's funny exit code conventions --- e.g., that 130 is not a controlled failure and 129 is one --- while test_expect_code has simpler, more generally valid semantics.

But maybe in practice this helper would only be used for git commands anyway.

> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -130,22 +130,57 @@ test_expect_success 'tests clean up after themselves' '

[...]

Show 5 quoted lines

> +test_expect_success 'tests clean up even on failures' "
> +    mkdir failing-cleanup &&
> +    (cd failing-cleanup &&
> +    cat >failing-cleanup.sh <<EOF &&
> +#!$SHELL_PATH

Is $SHELL_PATH allowed to contain a shell metacharacter? (just curious).

Show 6 quoted lines

> +
> +test_description='Failing tests with cleanup commands'
> +
> +# Point to the t/test-lib.sh, which isn't in ../ as usual
> +TEST_DIRECTORY=\"$TEST_DIRECTORY\"
> +. \"\$TEST_DIRECTORY\"/test-lib.sh

Quoting issues? I suspect the first $TEST_DIRECTORY here would be twice expanded and the second would be once expanded before failing-cleanup.sh is written.

On the other hand, a $TEST_DIRECTORY with backslashes in it is asking for trouble for other reasons already.

Show 9 quoted lines

> +
> +test_expect_success 'tests clean up even after a failure' '
> +    touch clean-after-failure &&
> +    test_when_finished rm clean-after-failure &&
> +    (exit 1)
>  '
>  
> +test_expect_success 'failure to clean up causes the test to fail' '
> +    test_when_finished \"(exit 2)\"

This changes the semantics of the test: before, it checked that the exit code was propagated out in these failure cases, but now it just checks that the test fails.

The new semantics are probably more appropriate --- who relies on the exact exit status from a test script, anyway?

> --- a/t/t1504-ceiling-dirs.sh
> +++ b/t/t1504-ceiling-dirs.sh

[...]

> --- a/t/t6020-merge-df.sh
> +++ b/t/t6020-merge-df.sh

[...]

Nice. :)

Show 22 quoted lines

> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -658,6 +640,28 @@ test_might_fail () {
>  	return 0
>  }
>  
> +# Similar to test_must_fail and test_might_fail, but check that a
> +# given command exited with a given exit code. Meant to be used as:
> +#
> +#	test_expect_success 'Merge with d/f conflicts' '
> +#		test_expect_code 1 git merge "merge msg" B master
> +#	'
> +
> +test_expect_code () {
> +	want_code=$1
> +	shift
> +	"$@"
> +	exit_code=$?
> +	if test $exit_code = $want_code
> +	then
> +		echo >&2 "test_expect_code: command exited with $exit_code: $*"
> +		return 0

This makes the tests noisier on success. I have no strong feelings either way about that, but it's probably worth mentioning in the commit message.

Anyway, for what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.