Re: [PATCH 07/10] fetch+push tests: have tests clean up their own mess

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2022, 21:44 UTC
Message-ID: <xmqqczj4hvx4.fsf@gitster.g>
In-Reply-To: <patch-07.10-7ce22dbe738-20220302T131859Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 20 quoted lines

> Change the "t5516-fetch-push.sh" test code to make use of
> "test_when_finished" to remove data instead of having tests clean up
> leftover data from earlier tests, which may or may not be
> there (e.g. depending on the --run=* option).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t5516-fetch-push.sh | 50 +++++++++++++++++++++++++------------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index e4bb7581568..fbe0a72b0b2 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -23,7 +23,8 @@ D=$(pwd)
>  
>  mk_empty () {
>  	repo_name="$1"
> -	rm -fr "$repo_name" &&
> +	test_when_finished "rm -rf \"$repo_name\"" &&

Any justification to swap between fr and rf?

> +	test_path_is_missing "$repo_name" &&

So, the idea is that the philosophy so far was that each test clears whatever the mess the previous ones created and empties what it cares to be empty with "rm -fr", but now in the new world order, each test relies on test_when_finished to clear any and all effects it leaves to the environment?

OK. Since the clearing the "effects" is to remove the whole thing, there is little room for such a plan to go wrong.

Show 6 quoted lines

> @@ -191,32 +192,32 @@ grep_wrote () {
>  	grep 'write_pack_file/wrote.*"value":"'$1'"' $2
>  }
>  
> -test_expect_success 'push with negotiation' '
> -	# Without negotiation

;-)

Show 14 quoted lines

> +test_expect_success 'push without negotiation' '
>  	mk_empty testrepo &&
>  	git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
>  	test_commit -C testrepo unrelated_commit &&
>  	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
> -	echo now pushing without negotiation &&
> +	test_when_finished "rm event" &&
>  	GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 push testrepo refs/heads/main:refs/remotes/origin/main &&
> -	grep_wrote 5 event && # 2 commits, 2 trees, 1 blob
> +	grep_wrote 5 event # 2 commits, 2 trees, 1 blob
> +'
>  
> -	# Same commands, but with negotiation
> -	rm event &&

Good to split them into two. Presumably the later part wasn't relying on the leftover side effects created by the early part?

Thanks.  Looking good.