diff mbox series

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

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

Commit Message

Glen Choo Feb. 24, 2022, 10:08 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>
---
Per Ævar's suggestion [1], I reverted the test suite changes that
replaced "test_cmp" with "grep", and opted to munge out irrelevant
details with "sed". Opinion on "grep" vs "test_cmp" seems a bit split,
and there are good arguments for either, but for these tests, I think
test_cmp works better for a few reasons:

- The motivation for removing test_cmp in the old patch 1 [2], i.e.
  we _want_ to ignore changes in the test output, turned out to be
  absolutely incorrect. We actually care a lot about the changes in test
  output because it tells us where we are reading the submodule info
  from.

  In the v1-2 test scheme, verify_fetch_result() would ignore those
  changes, so I added extra extra grep-s specifically to check
  where the submodule is read from. ([3] comments on one of these). I
  could have generalized verify_fetch_result() to handle those extra
  grep-s, but since the original motivation for using grep is gone,
  test_cmp seemed like a viable alternative for an intuitive test
  scheme.

- verify_fetch_result() is easier to reason about because we now assert
  on the output almost verbatim (besides munging), instead of mixing
  greps and negative greps to get a similar result. This should be
  helpful for someone updating the tests later.

- When a test can't use verify_fetch_result() (e.g. it involves other
  submodules, patch 9 adds some of these), I found it a lot easier to
  write the test using test_cmp instead of grep.

- test_cmp tests are sensitive to unmeaningful changes, but this
  behavior helps us catch unwanted regressions and (in these tests at
  least) it is relatively easy to change test_cmp tests to do the right
  thing.

[1] https://lore.kernel.org/git/220216.86y22bt8gp.gmgdl@evledraar.gmail.com
[2] https://lore.kernel.org/git/20220215172318.73533-2-chooglen@google.com
[3] https://lore.kernel.org/git/20220215220229.1633486-1-jonathantanmy@google.com

 t/t5526-fetch-submodules.sh | 117 +++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 61 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Feb. 24, 2022, 11:52 a.m. UTC | #1 
On Thu, Feb 24 2022, Glen Choo wrote:

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

Hrm, I didn't know that would work, the usual style is:

    cat >file <<...

Instead of:

    cat <<.. >file

Maybe better to use that?

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

These look a lot better, but instead of always passing the result of
"git rev-parse --short HEAD" can't we just always invoke that in these
helpers?

Maybe there are cases where $NEW_HEAD is different, I've just skimmed
this series.

> @@ -62,7 +82,8 @@ verify_fetch_result() {
>  	if [ -f expect.err.deep ]; then
>  		cat expect.err.deep >>expect.err.combined
>  	fi &&
> -	test_cmp expect.err.combined $ACTUAL_ERR
> +	sed -E 's/[0-9a-f]+\.\./OLD_HEAD\.\./' $ACTUAL_ERR >actual.err.cmp &&
> +	test_cmp expect.err.combined actual.err.cmp
>  }

I think this is unportable per check-non-portable-shell.pl:

        /\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)';
Glen Choo Feb. 24, 2022, 4:15 p.m. UTC | #2 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Feb 24 2022, Glen Choo wrote:
>
>> +check_sub() {
>> +	NEW_HEAD=$1 &&
>> +	cat <<-EOF >$pwd/expect.err.sub
>
> Hrm, I didn't know that would work, the usual style is:
>
>     cat >file <<...
>
> Instead of:
>
>     cat <<.. >file
>
> Maybe better to use that?

Thanks, I somehow mixed things up when I wrote that.

>> +	Fetching submodule submodule
>> +	From $pwd/submodule
>> +	   OLD_HEAD..$NEW_HEAD  sub        -> origin/sub
>> +	EOF
>> +}
>> +
>> +check_deep() {
>> +	NEW_HEAD=$1 &&
>> +	cat <<-EOF >$pwd/expect.err.deep
>> +	Fetching submodule submodule/subdir/deepsubmodule
>> +	From $pwd/deepsubmodule
>> +	   OLD_HEAD..$NEW_HEAD  deep       -> origin/deep
>> +	EOF
>> +}
>> +
>> +check_super() {
>> +	NEW_HEAD=$1 &&
>> +	cat <<-EOF >$pwd/expect.err.super
>> +	From $pwd/.
>> +	   OLD_HEAD..$NEW_HEAD  super      -> origin/super
>> +	EOF
>> +}
>
> These look a lot better, but instead of always passing the result of
> "git rev-parse --short HEAD" can't we just always invoke that in these
> helpers?
>
> Maybe there are cases where $NEW_HEAD is different, I've just skimmed
> this series.

I haven't found any other instances where $NEW_HEAD is different, so I
suppose we could move it into the helpers. I don't think it benefits
readability that much to do so, but if you think it's much better, I'll
incorporate it when I reroll this.

>> @@ -62,7 +82,8 @@ verify_fetch_result() {
>>  	if [ -f expect.err.deep ]; then
>>  		cat expect.err.deep >>expect.err.combined
>>  	fi &&
>> -	test_cmp expect.err.combined $ACTUAL_ERR
>> +	sed -E 's/[0-9a-f]+\.\./OLD_HEAD\.\./' $ACTUAL_ERR >actual.err.cmp &&
>> +	test_cmp expect.err.combined actual.err.cmp
>>  }
>
> I think this is unportable per check-non-portable-shell.pl:
>
>         /\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)';

Ah thanks, my sed-fu is pretty poor, so I appreciate the tip :)

I used that because I wanted +, but I found what I needed from the sed
manpage i.e. that + is equivalent to \{1,\}).
Eric Sunshine Feb. 24, 2022, 6:13 p.m. UTC | #3 
On Thu, Feb 24, 2022 at 11:46 AM Glen Choo <chooglen@google.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> > On Thu, Feb 24 2022, Glen Choo wrote:
> >> +    sed -E 's/[0-9a-f]+\.\./OLD_HEAD\.\./' $ACTUAL_ERR >actual.err.cmp &&
> >
> > I think this is unportable per check-non-portable-shell.pl:
> >
> >         /\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)';
>
> I used that because I wanted +, but I found what I needed from the sed
> manpage i.e. that + is equivalent to \{1,\}).

This isn't necessarily going to be portable either for older sed
implementations. Most portable would be:

    [0-9a-f][0-9a-f]*

(Whether or not we need to worry about those older sed impmenetations
is a different question...)
Jonathan Tan Feb. 24, 2022, 11:05 p.m. UTC | #4 
Glen Choo <chooglen@google.com> writes:
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 0e93df1665..a3890e2f6c 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -13,6 +13,32 @@ export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
>  
>  pwd=$(pwd)
>  
> +check_sub() {
> +	NEW_HEAD=$1 &&
> +	cat <<-EOF >$pwd/expect.err.sub
> +	Fetching submodule submodule
> +	From $pwd/submodule
> +	   OLD_HEAD..$NEW_HEAD  sub        -> origin/sub
> +	EOF
> +}
> +
> +check_deep() {
> +	NEW_HEAD=$1 &&
> +	cat <<-EOF >$pwd/expect.err.deep
> +	Fetching submodule submodule/subdir/deepsubmodule
> +	From $pwd/deepsubmodule
> +	   OLD_HEAD..$NEW_HEAD  deep       -> origin/deep
> +	EOF
> +}
> +
> +check_super() {
> +	NEW_HEAD=$1 &&
> +	cat <<-EOF >$pwd/expect.err.super
> +	From $pwd/.
> +	   OLD_HEAD..$NEW_HEAD  super      -> origin/super
> +	EOF
> +}

These don't do any checking, but just write what's expected to a file.
Could these be called something like write_sub_expected etc.?

Other than that, the patches up to this look fine (besides the comments
left by others).
Glen Choo Feb. 25, 2022, 2:26 a.m. UTC | #5 
Jonathan Tan <jonathantanmy@google.com> writes:

> Glen Choo <chooglen@google.com> writes:
>> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
>> index 0e93df1665..a3890e2f6c 100755
>> --- a/t/t5526-fetch-submodules.sh
>> +++ b/t/t5526-fetch-submodules.sh
>> @@ -13,6 +13,32 @@ export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
>>  
>>  pwd=$(pwd)
>>  
>> +check_sub() {
>> +	NEW_HEAD=$1 &&
>> +	cat <<-EOF >$pwd/expect.err.sub
>> +	Fetching submodule submodule
>> +	From $pwd/submodule
>> +	   OLD_HEAD..$NEW_HEAD  sub        -> origin/sub
>> +	EOF
>> +}
>> +
>> +check_deep() {
>> +	NEW_HEAD=$1 &&
>> +	cat <<-EOF >$pwd/expect.err.deep
>> +	Fetching submodule submodule/subdir/deepsubmodule
>> +	From $pwd/deepsubmodule
>> +	   OLD_HEAD..$NEW_HEAD  deep       -> origin/deep
>> +	EOF
>> +}
>> +
>> +check_super() {
>> +	NEW_HEAD=$1 &&
>> +	cat <<-EOF >$pwd/expect.err.super
>> +	From $pwd/.
>> +	   OLD_HEAD..$NEW_HEAD  super      -> origin/super
>> +	EOF
>> +}
>
> These don't do any checking, but just write what's expected to a file.
> Could these be called something like write_sub_expected etc.?
>

Thanks for the suggestion! I was struggling with names.
diff mbox series

Patch

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 0e93df1665..a3890e2f6c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -13,6 +13,32 @@  export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
 
 pwd=$(pwd)
 
+check_sub() {
+	NEW_HEAD=$1 &&
+	cat <<-EOF >$pwd/expect.err.sub
+	Fetching submodule submodule
+	From $pwd/submodule
+	   OLD_HEAD..$NEW_HEAD  sub        -> origin/sub
+	EOF
+}
+
+check_deep() {
+	NEW_HEAD=$1 &&
+	cat <<-EOF >$pwd/expect.err.deep
+	Fetching submodule submodule/subdir/deepsubmodule
+	From $pwd/deepsubmodule
+	   OLD_HEAD..$NEW_HEAD  deep       -> origin/deep
+	EOF
+}
+
+check_super() {
+	NEW_HEAD=$1 &&
+	cat <<-EOF >$pwd/expect.err.super
+	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
@@ -20,27 +46,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
 	)
 }
 
@@ -62,7 +82,8 @@  verify_fetch_result() {
 	if [ -f expect.err.deep ]; then
 		cat expect.err.deep >>expect.err.combined
 	fi &&
-	test_cmp expect.err.combined $ACTUAL_ERR
+	sed -E 's/[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 &&