diff mbox series

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

Message ID patch-07.10-7ce22dbe738-20220302T131859Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series tests: add and use a "test_hook" wrapper + hook fixes | expand

Commit Message

Ævar Arnfjörð Bjarmason March 2, 2022, 1:22 p.m. UTC 
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(-)

Comments

Junio C Hamano March 2, 2022, 9:44 p.m. UTC | #1 
Æ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.
diff mbox series

Patch

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\"" &&
+	test_path_is_missing "$repo_name" &&
 	git init "$repo_name" &&
 	git -C "$repo_name" config receive.denyCurrentBranch warn
 }
@@ -86,7 +87,7 @@  mk_test_with_hooks() {
 }
 
 mk_child() {
-	rm -rf "$2" &&
+	test_when_finished "rm -rf \"$2\"" &&
 	git clone "$1" "$2"
 }
 
@@ -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 &&
+test_expect_success 'push with 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 &&
+	test_when_finished "rm event" &&
 	GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&
 	grep_wrote 2 event # 1 commit, 1 tree
 '
 
 test_expect_success 'push with negotiation proceeds anyway even if negotiation fails' '
-	rm event &&
 	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 &&
+	test_when_finished "rm event" &&
 	GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" \
 		git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
 	grep_wrote 5 event && # 2 commits, 2 trees, 1 blob
@@ -1323,7 +1324,7 @@  done
 
 test_expect_success 'fetch follows tags by default' '
 	mk_test testrepo heads/main &&
-	rm -fr src dst &&
+	test_when_finished "rm -rf src" &&
 	git init src &&
 	(
 		cd src &&
@@ -1333,6 +1334,7 @@  test_expect_success 'fetch follows tags by default' '
 		sed -n "p; s|refs/heads/main$|refs/remotes/origin/main|p" tmp1 |
 		sort -k 3 >../expect
 	) &&
+	test_when_finished "rm -rf dst" &&
 	git init dst &&
 	(
 		cd dst &&
@@ -1358,8 +1360,9 @@  test_expect_success 'peeled advertisements are not considered ref tips' '
 
 test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '
 	mk_test testrepo heads/main &&
-	rm -fr src dst &&
+	test_when_finished "rm -rf src" &&
 	git init src &&
+	test_when_finished "rm -rf dst" &&
 	git init --bare dst &&
 	(
 		cd src &&
@@ -1382,8 +1385,9 @@  test_expect_success 'pushing a specific ref applies remote.$name.push as refmap'
 
 test_expect_success 'with no remote.$name.push, it is not used as refmap' '
 	mk_test testrepo heads/main &&
-	rm -fr src dst &&
+	test_when_finished "rm -rf src" &&
 	git init src &&
+	test_when_finished "rm -rf dst" &&
 	git init --bare dst &&
 	(
 		cd src &&
@@ -1404,8 +1408,9 @@  test_expect_success 'with no remote.$name.push, it is not used as refmap' '
 
 test_expect_success 'with no remote.$name.push, upstream mapping is used' '
 	mk_test testrepo heads/main &&
-	rm -fr src dst &&
+	test_when_finished "rm -rf src" &&
 	git init src &&
+	test_when_finished "rm -rf dst" &&
 	git init --bare dst &&
 	(
 		cd src &&
@@ -1433,8 +1438,9 @@  test_expect_success 'with no remote.$name.push, upstream mapping is used' '
 
 test_expect_success 'push does not follow tags by default' '
 	mk_test testrepo heads/main &&
-	rm -fr src dst &&
+	test_when_finished "rm -rf src" &&
 	git init src &&
+	test_when_finished "rm -rf dst" &&
 	git init --bare dst &&
 	(
 		cd src &&
@@ -1456,8 +1462,9 @@  test_expect_success 'push does not follow tags by default' '
 
 test_expect_success 'push --follow-tags only pushes relevant tags' '
 	mk_test testrepo heads/main &&
-	rm -fr src dst &&
+	test_when_finished "rm -rf src" &&
 	git init src &&
+	test_when_finished "rm -rf dst" &&
 	git init --bare dst &&
 	(
 		cd src &&
@@ -1495,9 +1502,9 @@  EOF
 '
 
 test_expect_success 'pushing a tag pushes the tagged object' '
-	rm -rf dst.git &&
 	blob=$(echo unreferenced | git hash-object -w --stdin) &&
 	git tag -m foo tag-of-blob $blob &&
+	test_when_finished "rm -rf dst.git" &&
 	git init --bare dst.git &&
 	git push dst.git tag-of-blob &&
 	# the receiving index-pack should have noticed
@@ -1508,7 +1515,7 @@  test_expect_success 'pushing a tag pushes the tagged object' '
 '
 
 test_expect_success 'push into bare respects core.logallrefupdates' '
-	rm -rf dst.git &&
+	test_when_finished "rm -rf dst.git" &&
 	git init --bare dst.git &&
 	git -C dst.git config core.logallrefupdates true &&
 
@@ -1526,7 +1533,7 @@  test_expect_success 'push into bare respects core.logallrefupdates' '
 '
 
 test_expect_success 'fetch into bare respects core.logallrefupdates' '
-	rm -rf dst.git &&
+	test_when_finished "rm -rf dst.git" &&
 	git init --bare dst.git &&
 	(
 		cd dst.git &&
@@ -1547,6 +1554,7 @@  test_expect_success 'fetch into bare respects core.logallrefupdates' '
 '
 
 test_expect_success 'receive.denyCurrentBranch = updateInstead' '
+	mk_empty testrepo &&
 	git push testrepo main &&
 	(
 		cd testrepo &&
@@ -1649,7 +1657,7 @@  test_expect_success 'receive.denyCurrentBranch = updateInstead' '
 	) &&
 
 	# (5) push into void
-	rm -fr void &&
+	test_when_finished "rm -rf void" &&
 	git init void &&
 	(
 		cd void &&
@@ -1671,7 +1679,7 @@  test_expect_success 'receive.denyCurrentBranch = updateInstead' '
 '
 
 test_expect_success 'updateInstead with push-to-checkout hook' '
-	rm -fr testrepo &&
+	test_when_finished "rm -rf testrepo" &&
 	git init testrepo &&
 	git -C testrepo pull .. main &&
 	git -C testrepo reset --hard HEAD^^ &&
@@ -1730,7 +1738,7 @@  test_expect_success 'updateInstead with push-to-checkout hook' '
 	) &&
 
 	# push into void
-	rm -fr void &&
+	test_when_finished "rm -rf void" &&
 	git init void &&
 	git -C void config receive.denyCurrentBranch updateInstead &&
 	write_script void/.git/hooks/push-to-checkout <<-\EOF &&