[PATCH] t7004: replace wc -l with modern test helpers

[PATCH] t7004: replace wc -l with modern test helpers

[Siddharth Shrimali]

Pipelines of the form "test $(git tag | wc -l) -eq 0" suppress git's
exit code. This means a crash or unexpected failure from git tag would
go undetected. Additionally, the use of $(...) creates a subshell for
each check, which adds unnecessary overhead.

Replace these patterns with test_must_be_empty and test_line_count.
These helpers check the output of git directly from a file, ensuring
git's exit code is captured properly via the preceding "&&" chain.
They also provide better diagnostics on failure by printing the
contents of the file when a check does not pass.

Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 t/t7004-tag.sh | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index ce2ff2a28a..faf7d97fc4 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -33,8 +33,10 @@ test_expect_success 'listing all tags in an empty tree should succeed' '
 '
 
 test_expect_success 'listing all tags in an empty tree should output nothing' '
-	test $(git tag -l | wc -l) -eq 0 &&
-	test $(git tag | wc -l) -eq 0
+	git tag -l >actual &&
+	test_must_be_empty actual &&
+	git tag >actual &&
+	test_must_be_empty actual
 '
 
 test_expect_success 'sort tags, ignore case' '
@@ -178,7 +180,8 @@ test_expect_success 'listing tags using a non-matching pattern should succeed' '
 '
 
 test_expect_success 'listing tags using a non-matching pattern should output nothing' '
-	test $(git tag -l xxx | wc -l) -eq 0
+	git tag -l xxx >actual &&
+	test_must_be_empty actual
 '
 
 # special cases for creating tags:
@@ -188,13 +191,15 @@ test_expect_success 'trying to create a tag with the name of one existing should
 '
 
 test_expect_success 'trying to create a tag with a non-valid name should fail' '
-	test $(git tag -l | wc -l) -eq 1 &&
+	git tag -l >actual &&
+	test_line_count = 1 actual &&
 	test_must_fail git tag "" &&
 	test_must_fail git tag .othertag &&
 	test_must_fail git tag "other tag" &&
 	test_must_fail git tag "othertag^" &&
 	test_must_fail git tag "other~tag" &&
-	test $(git tag -l | wc -l) -eq 1
+	git tag -l >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'creating a tag using HEAD directly should succeed' '
-- 
2.51.2

Re: [PATCH] t7004: replace wc -l with modern test helpers

[Junio C Hamano]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> Pipelines of the form "test $(git tag | wc -l) -eq 0" suppress git's
> exit code. This means a crash or unexpected failure from git tag would
> go undetected. Additionally, the use of $(...) creates a subshell for
> each check, which adds unnecessary overhead.
>
> Replace these patterns with test_must_be_empty and test_line_count.
> These helpers check the output of git directly from a file, ensuring
> git's exit code is captured properly via the preceding "&&" chain.
> They also provide better diagnostics on failure by printing the
> contents of the file when a check does not pass.
>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
>  t/t7004-tag.sh | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

They all look the result of good and mechanical transformation.
Will queue.

Some tests, however, may want to be cleaned up on top in a follow-up
patch.

For example, taking a look at this one:

>  test_expect_success 'trying to create a tag with a non-valid name should fail' '
> -	test $(git tag -l | wc -l) -eq 1 &&
> +	git tag -l >actual &&
> +	test_line_count = 1 actual &&
>  	test_must_fail git tag "" &&
>  	test_must_fail git tag .othertag &&
>  	test_must_fail git tag "other tag" &&
>  	test_must_fail git tag "othertag^" &&
>  	test_must_fail git tag "other~tag" &&
> -	test $(git tag -l | wc -l) -eq 1
> +	git tag -l >actual &&
> +	test_line_count = 1 actual
>  '

it is dubious to insist that we have a single tag at the beginning
of the test, as more tests can be added before this step, or some
tests may be retired before this step, changing the expected state
before we start this step.  As the test title says, all the tag
creation tests that we mark with test_must_fail are very relevant
to this one, but the last one that counts the existing tag again is
pointless, as it adds an unnecessary dependency on the state left by
previous tests (i.e., if we start with a single tag in the repository,
and we expect all the tag creation in this test fail, then we should
end up with a single tag in the repository---but this means that every
time a future change makes previous tests leave no tag or two or
more tags before this test runs, this test needs to be updated to
expect different number of tags).

I didn't bother looking at all the other tests outside what was
visible in the patch, so please do not just react on this comment
and update only this test.  If we were to make such a clean up that
is more than mechanical rewrite, it should be done with a better
understanding of the whole picture of the test script, not just what
I happened to have noticed within the context of this patch that
changed something unrelated (i.e., use of "| wc -l").

Thanks.