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

[Jonathan Nieder <jrnieder@gmail.com>]

Elijah Newren wrote:

> --- 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' '
[...]
> +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).

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

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

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