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

From: Brandon Williams <bmwill@google.com>
Date: Aug 14, 2018, 17:06 UTC
Message-ID: <20180814170619.GE240194@google.com>
In-Reply-To: <20180814110525.17801-4-ao2@ao2.it>

On 08/14, Antonio Ospite wrote:

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

Thanks for cleaning this up. We seem to have a habit for leaving testing state around for longer than is necessary which makes it a bit more difficult to read and understand when looking at it later. What would really be nice is if each test was self-contained...course that would take a herculean effort to realize in our testsuite so I'm not suggesting you do that :)

Show 41 quoted lines

> 
> 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^" &&
>  	(cd super &&
>  		sha1=$(git rev-parse HEAD) &&
>  		test-tool submodule-config \
> @@ -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
>  	)
>  '
>  
> -- 
> 2.18.0
>

-- 
Brandon Williams