Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2018, 20:16 UTC
Message-ID: <xmqq7eks1z6h.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180814110525.17801-4-ao2@ao2.it>

Antonio Ospite <ao2@ao2.it> writes:

Show 7 quoted lines

>  test_expect_success 'error message contains blob reference' '
> +	# Remove the error introduced in the previous test.
> +	# It is not needed in the following tests.
> +	test_when_finished "git -C super reset --hard HEAD^" &&
>  	(cd super &&
>  		sha1=$(git rev-parse HEAD) &&
>  		test-tool submodule-config \

Antonio Ospite <ao2@ao2.it> writes:

Show 27 quoted lines

> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> invalid lines in .gitmodules but then only the second commit is removed.
>
> This may affect future subsequent tests if they assume that the
> .gitmodules file has no errors.
>
> Remove both the commits as soon as they are not needed anymore.
>
> The error introduced in test 5 is also required by test 6, so the two
> commits from above are removed respectively in tests 6 and 8.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  t/t7411-submodule-config.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850ac..c6b6cf6fae 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets continue' '
>  '
>  
>  test_expect_success 'error message contains blob reference' '
> +	# Remove the error introduced in the previous test.
> +	# It is not needed in the following tests.
> +	test_when_finished "git -C super reset --hard HEAD^" &&

Hmm, that is ugly. Depending on where in the subshell the previous test failed, you'd still be taking us to an unexpected place. Imagine if "git commit -m 'add error'" failed, for example, in the test before this one.

I am wondering if the proper fix is to merge the previous one and this one into a single test. The combined test would

    - remember where the HEAD in super is and arrange to come back
      to it when test is done
    - break .gitmodules and commit it
    - run test-tool and check its output
    - also check its error output

in a single test_expect_success.

Show 17 quoted lines

> @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' '
>  '
>  
>  test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> +	test_when_finished "git -C super reset --hard HEAD^" &&
>  	(cd super &&
>  		git config -f .gitmodules \
>  			submodule.submodule.fetchrecursesubmodules blabla &&
> @@ -134,8 +138,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>  			HEAD b \
>  			HEAD submodule \
>  				>actual &&
> -		test_cmp expect_error actual  &&
> -		git reset --hard HEAD^
> +		test_cmp expect_error actual
>  	)
>  '

If we want to be more robust, you'd probably need to find a better anchoring point than HEAD, which can be pointing different commit depending on where in the subshell the process is hit with ^C, i.e.

	ORIG=$(git -C super rev-parse HEAD) &&
	test_when_finished "git -C super reset --hard $ORIG" &&
	(
		cd super &&
		...

The patch is still an improvement compared to the current code, where a broken test-tool that does not produce expected output in the file 'actual' is guaranteed to leave us at a commit that we do not expect to be at, but not entirely satisfactory.