Re: [PATCH 01/25] t: use test_might_fail() instead of manipulating exit code manually

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2018, 19:20 UTC
Message-ID: <xmqqin5wqg7e.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180702002405.3042-2-sunshine@sunshineco.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 8 quoted lines

>  test_expect_success 'merge my-side@{u} records the correct name' '
>  (
> -	cd clone || exit
> -	git checkout master || exit
> -	git branch -D new ;# can fail but is ok
> +	cd clone &&
> +	git checkout master &&
> +	test_might_fail git branch -D new &&

I wonder why we had these "|| exit" in the first place, but these are obviously good changes.

Show 12 quoted lines

> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 1e81b33b2e..39133bcbc8 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -435,7 +435,7 @@ test_expect_success 'writing split index with null sha1 does not write cache tre
>  	commit=$(git commit-tree $tree -p HEAD <msg) &&
>  	git update-ref HEAD "$commit" &&
>  	GIT_ALLOW_NULL_SHA1=1 git reset --hard &&
> -	(test-tool dump-cache-tree >cache-tree.out || true) &&
> +	test_might_fail test-tool dump-cache-tree >cache-tree.out &&
>  	test_line_count = 0 cache-tree.out
>  '

I wonder where the unstable expectation for the exit status comes from. 4bddd983 ("split-index: don't write cache tree with null oid entries", 2018-01-07) is not exactly illuminating, either X-<.

Show 14 quoted lines

> diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
> index 0a8af76aab..6579c81216 100755
> --- a/t/t4012-diff-binary.sh
> +++ b/t/t4012-diff-binary.sh
> @@ -102,10 +102,8 @@ test_expect_success 'apply binary patch' '
>  
>  test_expect_success 'diff --no-index with binary creation' '
>  	echo Q | q_to_nul >binary &&
> -	(: hide error code from diff, which just indicates differences
> -	 git diff --binary --no-index /dev/null binary >current ||
> -	 true
> -	) &&
> +	# hide error code from diff, which just indicates differences
> +	test_might_fail git diff --binary --no-index /dev/null binary >current &&

As you said in downthread, we only care about the correct output in the "current" file, which is verified by using it as input to the "apply --binary", so might-fail is fine here. But we probably want to make sure "diff --binary --no-index" to *notice* that there is a difference and report that with its exit status. I wouldn't say so if this was about testing "git apply", but the test title tells us that it is about "diff --no-index", so...