diff mbox series

[3/3] t: detect and signal failure within loop

Message ID 31a962fd5070d68964e545fb5506d795e8845ec3.1661192802.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 0e66bc1b215b1dd2b0f05c5777eb8ee2ae4e9523
Headers show
Series tests: fix broken &&-chains & abort loops on error | expand

Commit Message

Eric Sunshine Aug. 22, 2022, 6:26 p.m. UTC 
From: Eric Sunshine <sunshine@sunshineco.com>

Failures within `for` and `while` loops can go unnoticed if not detected
and signaled manually since the loop itself does not abort when a
contained command fails, nor will a failure necessarily be detected when
the loop finishes since the loop returns the exit code of the last
command it ran on the final iteration, which may not be the command
which failed. Therefore, detect and signal failures manually within
loops using the idiom `|| return 1` (or `|| exit 1` within subshells).

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/perf/p7527-builtin-fsmonitor.sh        | 2 +-
 t/t1092-sparse-checkout-compatibility.sh | 6 +++---
 t/t5329-pack-objects-cruft.sh            | 8 ++++----
 t/t6429-merge-sequence-rename-caching.sh | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

Comments

Junio C Hamano Aug. 22, 2022, 8:22 p.m. UTC | #1 
"Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
> index 8968f7a08d8..6049e2c1d78 100755
> --- a/t/t5329-pack-objects-cruft.sh
> +++ b/t/t5329-pack-objects-cruft.sh
> @@ -29,7 +29,7 @@ basic_cruft_pack_tests () {
>  				while read oid
>  				do
>  					path="$objdir/$(test_oid_to_path "$oid")" &&
> -					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
> +					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" || exit 1
>  				done |
>  				sort -k1
>  			) >expect &&

With the loop being on the upstream of a pipe, does the added "exit
1" have any effect?

Everything else in these three patches looked very sensible, but
this one I found questionable.

Thanks.
Junio C Hamano Aug. 22, 2022, 8:59 p.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> writes:

> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
>> index 8968f7a08d8..6049e2c1d78 100755
>> --- a/t/t5329-pack-objects-cruft.sh
>> +++ b/t/t5329-pack-objects-cruft.sh
>> @@ -29,7 +29,7 @@ basic_cruft_pack_tests () {
>>  				while read oid
>>  				do
>>  					path="$objdir/$(test_oid_to_path "$oid")" &&
>> -					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
>> +					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" || exit 1
>>  				done |
>>  				sort -k1
>>  			) >expect &&
>
> With the loop being on the upstream of a pipe, does the added "exit
> 1" have any effect?

And the answer is "no".  Without use of rhetorical question:

    The loop is on the upstream side of a pipe, so "exit 1" will be
    lost.  "sort -k1" will get a shortened output, unless the
    failure happens at the last iteration, so it is likely that the
    test may fail, but relying on the "expect" (what is supposed to
    have the _right_ answer) file not being right to get our
    breakage noticed does not sound right.

> Everything else in these three patches looked very sensible, but
> this one I found questionable.

As to the questionable one, we could probably do something like the
attached patch if we really wanted to.  We can guarantee that this
"expect" will never match any "actual", which is output from
pack-mtimes test tool command.  Whatever "tricky/ugly" approach we
choose to take, I think this one deserves to be done in a single
patch on its own with an explanation.

----- >8 --------- >8 --------- >8 --------- >8 ----
t5329: notice a failure within a loop

We try to write "|| return 1" at the end of a sequence of &&-chained
command in a loop of our tests, so that a failure of any step during
the earlier iteration of the loop can properly be caught.

There is one loop in this test script that is used to compute the
expected result, that will be later compared with an actual output
produced by the "test-tool pack-mtimes" command.  This particular
loop, however, is placed on the upstream side of a pipe, whose
non-zero exit code does not get noticed.

Emit a line that will never be produced by the "test-tool pack-mtimes"
to cause the later comparison to fail.  As we use test_cmp to compare
this "expected output" file with the "actual output", the "error
message" we are emitting into the expected output stream will stand
out and shown to the tester.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5329-pack-objects-cruft.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git c/t/t5329-pack-objects-cruft.sh w/t/t5329-pack-objects-cruft.sh
index 6049e2c1d7..43d752acc7 100755
--- c/t/t5329-pack-objects-cruft.sh
+++ w/t/t5329-pack-objects-cruft.sh
@@ -29,7 +29,8 @@ basic_cruft_pack_tests () {
 				while read oid
 				do
 					path="$objdir/$(test_oid_to_path "$oid")" &&
-					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
+					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" ||
+					echo "object list generation failed for $obj"
 				done |
 				sort -k1
 			) >expect &&
Elijah Newren Aug. 23, 2022, 3:05 a.m. UTC | #3 
On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> Failures within `for` and `while` loops can go unnoticed if not detected
> and signaled manually since the loop itself does not abort when a
> contained command fails, nor will a failure necessarily be detected when
> the loop finishes since the loop returns the exit code of the last
> command it ran on the final iteration, which may not be the command
> which failed. Therefore, detect and signal failures manually within
> loops using the idiom `|| return 1` (or `|| exit 1` within subshells).
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
[...]
> diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
> index e1ce9199164..650b3cd14ff 100755
> --- a/t/t6429-merge-sequence-rename-caching.sh
> +++ b/t/t6429-merge-sequence-rename-caching.sh
> @@ -725,7 +725,7 @@ test_expect_success 'avoid assuming we detected renames' '
>                 mkdir unrelated &&
>                 for i in $(test_seq 1 10)
>                 do
> -                       >unrelated/$i
> +                       >unrelated/$i || exit 1
>                 done &&
>                 test_seq  2 10 >numbers &&
>                 test_seq 12 20 >values &&
> --
> gitgitgadget

That's not something I'm likely ever going to remember to think of as
capable of failing and needing this special care.  Is this a
preliminary series before you send chainlint improvements that finds
this kind of thing for us?  Or did you notice this some other way?

Change is fine, of course, I'm just curious how it was found (and how
I can avoid adding more of these that you'll need to later fix up).
Johannes Sixt Aug. 23, 2022, 6:30 a.m. UTC | #4 
Am 22.08.22 um 22:59 schrieb Junio C Hamano:
> t5329: notice a failure within a loop
> 
> We try to write "|| return 1" at the end of a sequence of &&-chained
> command in a loop of our tests, so that a failure of any step during
> the earlier iteration of the loop can properly be caught.
> 
> There is one loop in this test script that is used to compute the
> expected result, that will be later compared with an actual output
> produced by the "test-tool pack-mtimes" command.  This particular
> loop, however, is placed on the upstream side of a pipe, whose
> non-zero exit code does not get noticed.
> 
> Emit a line that will never be produced by the "test-tool pack-mtimes"
> to cause the later comparison to fail.  As we use test_cmp to compare
> this "expected output" file with the "actual output", the "error
> message" we are emitting into the expected output stream will stand
> out and shown to the tester.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t5329-pack-objects-cruft.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git c/t/t5329-pack-objects-cruft.sh w/t/t5329-pack-objects-cruft.sh
> index 6049e2c1d7..43d752acc7 100755
> --- c/t/t5329-pack-objects-cruft.sh
> +++ w/t/t5329-pack-objects-cruft.sh
> @@ -29,7 +29,8 @@ basic_cruft_pack_tests () {
>  				while read oid
>  				do
>  					path="$objdir/$(test_oid_to_path "$oid")" &&
> -					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
> +					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" ||
> +					echo "object list generation failed for $obj"

This looks like the right thing to do. But write $oid, not $obj.

>  				done |
>  				sort -k1
>  			) >expect &&
> 
> 

-- Hannes
Eric Sunshine Aug. 28, 2022, 4:50 a.m. UTC | #5 
On Mon, Aug 22, 2022 at 11:05 PM Elijah Newren <newren@gmail.com> wrote:
> On Mon, Aug 22, 2022 at 11:26 AM Eric Sunshine via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Failures within `for` and `while` loops can go unnoticed if not detected
> > and signaled manually since the loop itself does not abort when a
> > contained command fails, nor will a failure necessarily be detected when
> > the loop finishes since the loop returns the exit code of the last
> > command it ran on the final iteration, which may not be the command
> > which failed. Therefore, detect and signal failures manually within
> > loops using the idiom `|| return 1` (or `|| exit 1` within subshells).
> >
> > diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
> >                 for i in $(test_seq 1 10)
> >                 do
> > -                       >unrelated/$i
> > +                       >unrelated/$i || exit 1
> >                 done &&
>
> That's not something I'm likely ever going to remember to think of as
> capable of failing and needing this special care.  Is this a
> preliminary series before you send chainlint improvements that finds
> this kind of thing for us?  Or did you notice this some other way?

This could fail due to lack of space on the filesystem or "inode"
exhaustion (indeed, I've seen out-of-space failures when I've
underallocated the ramdisk on which I run tests). But there are some
cases of added `|| return` or `|| exit` in the original series[1]
which are just churn because the code inside the loop wouldn't /
couldn't / shouldn't fail. I wasn't happy about adding those simply to
pacify a not-smart-enough linter, but I eventually convinced myself
that the small amount of inconvenience of those pointless cases was
greatly outweighed by the vast number of cases in which adding `||
return` or `|| exit` was the correct thing to do since those cases
could genuinely have allowed errors in Git or in the tests themselves
to go unnoticed.

Yes, this is another preliminary series before sending the new
`chainlint` series, and it was the new linter which found these cases.

[1]: https://lore.kernel.org/git/20211209051115.52629-1-sunshine@sunshineco.com/

> Change is fine, of course, I'm just curious how it was found (and how
> I can avoid adding more of these that you'll need to later fix up).

Although the implementation of the new linter has been complete for
over a year, I finally found time to work on polishing the patch
series itself. I had hoped to get it submitted this past week, but
time constraints prevented it. So, the answer is that the new linter
(once I submit it and once Junio accepts it -- if he does) should help
you avoid introducing more such cases.
diff mbox series

Patch

diff --git a/t/perf/p7527-builtin-fsmonitor.sh b/t/perf/p7527-builtin-fsmonitor.sh
index 9338b9ea008..c3f9a4caa4c 100755
--- a/t/perf/p7527-builtin-fsmonitor.sh
+++ b/t/perf/p7527-builtin-fsmonitor.sh
@@ -249,7 +249,7 @@  test_expect_success "Cleanup temp and matrix branches" "
 	do
 		for fsm_val in $fsm_values
 		do
-			cleanup $uc_val $fsm_val
+			cleanup $uc_val $fsm_val || return 1
 		done
 	done
 "
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index e13368861ce..0302e36fd66 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -556,7 +556,7 @@  test_expect_success 'blame with pathspec inside sparse definition' '
 			deep/deeper1/a \
 			deep/deeper1/deepest/a
 	do
-		test_all_match git blame $file
+		test_all_match git blame $file || return 1
 	done
 '
 
@@ -1571,7 +1571,7 @@  test_expect_success 'sparse index is not expanded: blame' '
 			deep/deeper1/a \
 			deep/deeper1/deepest/a
 	do
-		ensure_not_expanded blame $file
+		ensure_not_expanded blame $file || return 1
 	done
 '
 
@@ -1907,7 +1907,7 @@  test_expect_success 'rm pathspec outside sparse definition' '
 		test_sparse_match test_must_fail git rm $file &&
 		test_sparse_match test_must_fail git rm --cached $file &&
 		test_sparse_match git rm --sparse $file &&
-		test_sparse_match git status --porcelain=v2
+		test_sparse_match git status --porcelain=v2 || return 1
 	done &&
 
 	cat >folder1-full <<-EOF &&
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 8968f7a08d8..6049e2c1d78 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -29,7 +29,7 @@  basic_cruft_pack_tests () {
 				while read oid
 				do
 					path="$objdir/$(test_oid_to_path "$oid")" &&
-					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")"
+					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" || exit 1
 				done |
 				sort -k1
 			) >expect &&
@@ -232,7 +232,7 @@  test_expect_success 'cruft tags rescue tagged objects' '
 		while read oid
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $oid)"
+				"$objdir/$(test_oid_to_path $oid)" || exit 1
 		done <objects &&
 
 		test-tool chmtime -500 \
@@ -272,7 +272,7 @@  test_expect_success 'cruft commits rescue parents, trees' '
 		while read object
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $object)"
+				"$objdir/$(test_oid_to_path $object)" || exit 1
 		done <objects &&
 		test-tool chmtime +500 "$objdir/$(test_oid_to_path \
 			$(git rev-parse HEAD))" &&
@@ -345,7 +345,7 @@  test_expect_success 'expired objects are pruned' '
 		while read object
 		do
 			test-tool chmtime -1000 \
-				"$objdir/$(test_oid_to_path $object)"
+				"$objdir/$(test_oid_to_path $object)" || exit 1
 		done <objects &&
 
 		keep="$(basename "$(ls $packdir/pack-*.pack)")" &&
diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
index e1ce9199164..650b3cd14ff 100755
--- a/t/t6429-merge-sequence-rename-caching.sh
+++ b/t/t6429-merge-sequence-rename-caching.sh
@@ -725,7 +725,7 @@  test_expect_success 'avoid assuming we detected renames' '
 		mkdir unrelated &&
 		for i in $(test_seq 1 10)
 		do
-			>unrelated/$i
+			>unrelated/$i || exit 1
 		done &&
 		test_seq  2 10 >numbers &&
 		test_seq 12 20 >values &&