diff mbox series

[v4,02/10] t5526: stop asserting on stderr literally

Message ID 20220304005757.70107-3-chooglen@google.com (mailing list archive)
State Superseded
Headers show
Series fetch --recurse-submodules: fetch unpopulated submodules | expand

Commit Message

Glen Choo March 4, 2022, 12:57 a.m. UTC 
In the previous commit message, we noted that not all of the "git fetch"
stderr is relevant to the tests. Most of the test setup lines are
dedicated to these details of the stderr:

1. which repos (super/sub/deep) are involved in the fetch
2. the head of the remote-tracking branch before the fetch (i.e. $head1)
3. the head of the remote-tracking branch after the fetch (i.e. $head2)

1. and 3. are relevant because they tell us that the expected commit is
fetched by the expected repo, but 2. is completely irrelevant.

Stop asserting on $head1 by replacing it with a dummy value in the
actual and expected output. Do this by introducing test
helpers (check_*()) that make it easier to construct the expected
output, and use sed to munge the actual output.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 t/t5526-fetch-submodules.sh | 119 +++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 62 deletions(-)

Comments

Junio C Hamano March 4, 2022, 2:12 a.m. UTC | #1 
Glen Choo <chooglen@google.com> writes:

> In the previous commit message, we noted that not all of the "git fetch"
> stderr is relevant to the tests. Most of the test setup lines are
> dedicated to these details of the stderr:
>
> 1. which repos (super/sub/deep) are involved in the fetch
> 2. the head of the remote-tracking branch before the fetch (i.e. $head1)
> 3. the head of the remote-tracking branch after the fetch (i.e. $head2)
>
> 1. and 3. are relevant because they tell us that the expected commit is
> fetched by the expected repo, but 2. is completely irrelevant.
>
> Stop asserting on $head1 by replacing it with a dummy value in the
> actual and expected output. Do this by introducing test
> helpers (check_*()) that make it easier to construct the expected
> output, and use sed to munge the actual output.
>
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  t/t5526-fetch-submodules.sh | 119 +++++++++++++++++-------------------
>  1 file changed, 57 insertions(+), 62 deletions(-)
>
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index dff7a4b90b..6b24d37b2b 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -10,6 +10,32 @@ export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
>  
>  pwd=$(pwd)
>  
> +check_sub () {
> +	NEW_HEAD=$1 &&
> +	cat >$pwd/expect.err.sub <<-EOF

Style.

	cat >"$pwd/expect.err.sub" <<-EOF

Here, $pwd most likely has an $IFS letter in it (because we
deliberately use "trash directory.xxxx" as the place to run our
tests in) but a redirection target does not go through $IFS word
splitting, so such a quoting is not technically necessary, but
some versions of bash are known to throw a warning if we don't,
and an extra quoting does not hurt.

> +check_deep () {
> +	NEW_HEAD=$1 &&
> +	cat >$pwd/expect.err.deep <<-EOF

Likewise.

> +	Fetching submodule submodule/subdir/deepsubmodule
> +	From $pwd/deepsubmodule
> +	   OLD_HEAD..$NEW_HEAD  deep       -> origin/deep
> +	EOF
> +}
> +
> +check_super () {
> +	NEW_HEAD=$1 &&
> +	cat >$pwd/expect.err.super <<-EOF

Likewise.

> +	From $pwd/.
> +	   OLD_HEAD..$NEW_HEAD  super      -> origin/super
> +	EOF
> +}
Jonathan Tan March 4, 2022, 10:41 p.m. UTC | #2 
Glen Choo <chooglen@google.com> writes:
> +check_sub () {
> +	NEW_HEAD=$1 &&
> +	cat >$pwd/expect.err.sub <<-EOF
> +	Fetching submodule submodule
> +	From $pwd/submodule
> +	   OLD_HEAD..$NEW_HEAD  sub        -> origin/sub
> +	EOF
> +}
> +
> +check_deep () {
> +	NEW_HEAD=$1 &&
> +	cat >$pwd/expect.err.deep <<-EOF
> +	Fetching submodule submodule/subdir/deepsubmodule
> +	From $pwd/deepsubmodule
> +	   OLD_HEAD..$NEW_HEAD  deep       -> origin/deep
> +	EOF
> +}
> +
> +check_super () {
> +	NEW_HEAD=$1 &&
> +	cat >$pwd/expect.err.super <<-EOF
> +	From $pwd/.
> +	   OLD_HEAD..$NEW_HEAD  super      -> origin/super
> +	EOF
> +}

The check_ names still aren't changed (as I suggested in [1]) but
perhaps it's fine to leave it. It doesn't seem to bother the other
reviewers, and changing it would slightly disrupt the review in that
there will be extra changes in the range-diff.

[1] https://lore.kernel.org/git/20220224230523.2877129-1-jonathantanmy@google.com/
Junio C Hamano March 4, 2022, 11:48 p.m. UTC | #3 
Jonathan Tan <jonathantanmy@google.com> writes:

> The check_ names still aren't changed (as I suggested in [1]) but
> perhaps it's fine to leave it. It doesn't seem to bother the other
> reviewers, and changing it would slightly disrupt the review in that
> there will be extra changes in the range-diff.

At least, please do not count my not mentioning it as such a vote.
I didn't mention it because I saw you did.

Thanks.
Glen Choo March 5, 2022, 12:25 a.m. UTC | #4 
Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> The check_ names still aren't changed (as I suggested in [1]) but
>> perhaps it's fine to leave it. It doesn't seem to bother the other
>> reviewers, and changing it would slightly disrupt the review in that
>> there will be extra changes in the range-diff.
>
> At least, please do not count my not mentioning it as such a vote.
> I didn't mention it because I saw you did.

Oh! Sorry, I intended to do this - I missed this and another suggestion
that you made re: test assertions [1]. Will incorporate this into the
next round.

[1] 20220304234622.647776-1-jonathantanmy@google.com
diff mbox series

Patch

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index dff7a4b90b..6b24d37b2b 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -10,6 +10,32 @@  export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
 
 pwd=$(pwd)
 
+check_sub () {
+	NEW_HEAD=$1 &&
+	cat >$pwd/expect.err.sub <<-EOF
+	Fetching submodule submodule
+	From $pwd/submodule
+	   OLD_HEAD..$NEW_HEAD  sub        -> origin/sub
+	EOF
+}
+
+check_deep () {
+	NEW_HEAD=$1 &&
+	cat >$pwd/expect.err.deep <<-EOF
+	Fetching submodule submodule/subdir/deepsubmodule
+	From $pwd/deepsubmodule
+	   OLD_HEAD..$NEW_HEAD  deep       -> origin/deep
+	EOF
+}
+
+check_super () {
+	NEW_HEAD=$1 &&
+	cat >$pwd/expect.err.super <<-EOF
+	From $pwd/.
+	   OLD_HEAD..$NEW_HEAD  super      -> origin/super
+	EOF
+}
+
 # For each submodule in the test setup, this creates a commit and writes
 # a file that contains the expected err if that new commit were fetched.
 # These output files get concatenated in the right order by
@@ -17,27 +43,21 @@  pwd=$(pwd)
 add_upstream_commit() {
 	(
 		cd submodule &&
-		head1=$(git rev-parse --short HEAD) &&
 		echo new >> subfile &&
 		test_tick &&
 		git add subfile &&
 		git commit -m new subfile &&
-		head2=$(git rev-parse --short HEAD) &&
-		echo "Fetching submodule submodule" > ../expect.err.sub &&
-		echo "From $pwd/submodule" >> ../expect.err.sub &&
-		echo "   $head1..$head2  sub        -> origin/sub" >> ../expect.err.sub
+		new_head=$(git rev-parse --short HEAD) &&
+		check_sub $new_head
 	) &&
 	(
 		cd deepsubmodule &&
-		head1=$(git rev-parse --short HEAD) &&
 		echo new >> deepsubfile &&
 		test_tick &&
 		git add deepsubfile &&
 		git commit -m new deepsubfile &&
-		head2=$(git rev-parse --short HEAD) &&
-		echo "Fetching submodule submodule/subdir/deepsubmodule" > ../expect.err.deep
-		echo "From $pwd/deepsubmodule" >> ../expect.err.deep &&
-		echo "   $head1..$head2  deep       -> origin/deep" >> ../expect.err.deep
+		new_head=$(git rev-parse --short HEAD) &&
+		check_deep $new_head
 	)
 }
 
@@ -47,7 +67,7 @@  add_upstream_commit() {
 #
 # If a repo should not be fetched in the test, its corresponding
 # expect.err file should be rm-ed.
-verify_fetch_result() {
+verify_fetch_result () {
 	ACTUAL_ERR=$1 &&
 	rm -f expect.err.combined &&
 	if test -f expect.err.super
@@ -62,7 +82,8 @@  verify_fetch_result() {
 	then
 		cat expect.err.deep >>expect.err.combined
 	fi &&
-	test_cmp expect.err.combined $ACTUAL_ERR
+	sed -e 's/[0-9a-f][0-9a-f]*\.\./OLD_HEAD\.\./' "$ACTUAL_ERR" >actual.err.cmp &&
+	test_cmp expect.err.combined actual.err.cmp
 }
 
 test_expect_success setup '
@@ -274,12 +295,10 @@  test_expect_success "Recursion doesn't happen when no new commits are fetched in
 '
 
 test_expect_success "Recursion stops when no new submodule commits are fetched" '
-	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >>expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	rm expect.err.deep &&
 	(
 		cd downstream &&
@@ -291,13 +310,11 @@  test_expect_success "Recursion stops when no new submodule commits are fetched"
 
 test_expect_success "Recursion doesn't happen when new superproject commits don't change any submodules" '
 	add_upstream_commit &&
-	head1=$(git rev-parse --short HEAD) &&
 	echo a > file &&
 	git add file &&
 	git commit -m "new file" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >> expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	rm expect.err.sub &&
 	rm expect.err.deep &&
 	(
@@ -318,12 +335,10 @@  test_expect_success "Recursion picks up config in submodule" '
 		)
 	) &&
 	add_upstream_commit &&
-	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >> expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	(
 		cd downstream &&
 		git fetch >../actual.out 2>../actual.err &&
@@ -345,20 +360,15 @@  test_expect_success "Recursion picks up all submodules when necessary" '
 			git fetch &&
 			git checkout -q FETCH_HEAD
 		) &&
-		head1=$(git rev-parse --short HEAD^) &&
 		git add subdir/deepsubmodule &&
 		git commit -m "new deepsubmodule" &&
-		head2=$(git rev-parse --short HEAD) &&
-		echo "Fetching submodule submodule" > ../expect.err.sub &&
-		echo "From $pwd/submodule" >> ../expect.err.sub &&
-		echo "   $head1..$head2  sub        -> origin/sub" >> ../expect.err.sub
+		new_head=$(git rev-parse --short HEAD) &&
+		check_sub $new_head
 	) &&
-	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >> expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	(
 		cd downstream &&
 		git fetch >../actual.out 2>../actual.err
@@ -376,13 +386,10 @@  test_expect_success "'--recurse-submodules=on-demand' doesn't recurse when no ne
 			git fetch &&
 			git checkout -q FETCH_HEAD
 		) &&
-		head1=$(git rev-parse --short HEAD^) &&
 		git add subdir/deepsubmodule &&
 		git commit -m "new deepsubmodule" &&
-		head2=$(git rev-parse --short HEAD) &&
-		echo Fetching submodule submodule > ../expect.err.sub &&
-		echo "From $pwd/submodule" >> ../expect.err.sub &&
-		echo "   $head1..$head2  sub        -> origin/sub" >> ../expect.err.sub
+		new_head=$(git rev-parse --short HEAD) &&
+		check_sub $new_head
 	) &&
 	(
 		cd downstream &&
@@ -395,12 +402,10 @@  test_expect_success "'--recurse-submodules=on-demand' doesn't recurse when no ne
 '
 
 test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necessary (and ignores config)" '
-	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >>expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	(
 		cd downstream &&
 		git config fetch.recurseSubmodules false &&
@@ -421,13 +426,11 @@  test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necess
 
 test_expect_success "'--recurse-submodules=on-demand' stops when no new submodule commits are found in the superproject (and ignores config)" '
 	add_upstream_commit &&
-	head1=$(git rev-parse --short HEAD) &&
 	echo a >> file &&
 	git add file &&
 	git commit -m "new file" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >> expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	rm expect.err.sub &&
 	rm expect.err.deep &&
 	(
@@ -445,12 +448,10 @@  test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config
 	) &&
 	add_upstream_commit &&
 	git config --global fetch.recurseSubmodules false &&
-	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >>expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	rm expect.err.deep &&
 	(
 		cd downstream &&
@@ -473,12 +474,10 @@  test_expect_success "'submodule.<sub>.fetchRecurseSubmodules=on-demand' override
 	) &&
 	add_upstream_commit &&
 	git config fetch.recurseSubmodules false &&
-	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "new submodule" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >>expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	rm expect.err.deep &&
 	(
 		cd downstream &&
@@ -499,12 +498,10 @@  test_expect_success "don't fetch submodule when newly recorded commits are alrea
 		cd submodule &&
 		git checkout -q HEAD^^
 	) &&
-	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git commit -m "submodule rewound" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." > expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >> expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	rm expect.err.sub &&
 	# This file does not exist, but rm -f for readability
 	rm -f expect.err.deep &&
@@ -526,13 +523,11 @@  test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .git
 		git fetch --recurse-submodules
 	) &&
 	add_upstream_commit &&
-	head1=$(git rev-parse --short HEAD) &&
 	git add submodule &&
 	git rm .gitmodules &&
 	git commit -m "new submodule without .gitmodules" &&
-	head2=$(git rev-parse --short HEAD) &&
-	echo "From $pwd/." >expect.err.super &&
-	echo "   $head1..$head2  super      -> origin/super" >>expect.err.super &&
+	new_head=$(git rev-parse --short HEAD) &&
+	check_super $new_head &&
 	rm expect.err.deep &&
 	(
 		cd downstream &&