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

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Emily Shaffer <emilyshaffer@google.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH 07/10] fetch+push tests: have tests clean up their own mess
Date: Wed, 02 Mar 2022 13:44:07 -0800	[thread overview]
Message-ID: <xmqqczj4hvx4.fsf@gitster.g> (raw)
In-Reply-To: <patch-07.10-7ce22dbe738-20220302T131859Z-avarab@gmail.com> (=?utf-8?B?IsOGdmFyIEFybmZqw7Zyw7A=?= Bjarmason"'s message of "Wed, 2 Mar 2022 14:22:26 +0100")

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

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

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

;-)

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