threads / patch / 60374

patcht/t7601: Modernize test scripts using functions

Subject: [PATCH] t/t7601: Modernize test scripts using functions

## tl;dr

2 messages between Oct 16, 2023 and Oct 16, 2023. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Dorcas AnonoLitunya· Oct 16, 2023, 15:21 UTC · lore

The test script is currently using the command format 'test -f' to check for existence or absence of files.

Replace it with new helper functions following the format 'test_path_is_file'.

Consequently, the patch also replaces the inverse command '! test -f' or 'test ! -f' with new helper function following the format 'test_path_is_missing'

This adjustment using helper functions makes the code more readable and easier to understand.

Signed-off-by: Dorcas AnonoLitunya <anonolitunya@gmail.com>
---
 t/t7601-merge-pull-config.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
Show changes to t/t7601-merge-pull-config.sh +12 −12
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index bd238d89b0..e08767df66 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' '
 
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
-	test -f c0.c &&
-	test -f c1.c &&
-	test ! -f c2.c &&
-	test ! -f c3.c &&
+	test_path_is_file c0.c &&
+	test_path_is_file c1.c &&
+	test_path_is_missing c2.c &&
+	test_path_is_missing c3.c &&
 	git merge c2 &&
-	test -f c1.c &&
-	test -f c2.c
+	test_path_is_file c1.c &&
+	test_path_is_file c2.c
 '
 
 test_expect_success 'fast-forward pull succeeds with "true" in pull.ff' '
@@ -411,8 +411,8 @@ test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
 	git reset --hard c1 &&
 	git config pull.twohead ours &&
 	git merge c2 &&
-	test -f c1.c &&
-	! test -f c2.c
+	test_path_is_file c1.c &&
+	test_path_is_missing c2.c
 '
 
 test_expect_success 'merge c1 with c2 and c3 (recursive in pull.octopus)' '
@@ -431,10 +431,10 @@ test_expect_success 'merge c1 with c2 and c3 (recursive and octopus in pull.octo
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
 	test "$(git rev-parse c3)" = "$(git rev-parse HEAD^3)" &&
 	git diff --exit-code &&
-	test -f c0.c &&
-	test -f c1.c &&
-	test -f c2.c &&
-	test -f c3.c
+	test_path_is_file c0.c &&
+	test_path_is_file c1.c &&
+	test_path_is_file c2.c &&
+	test_path_is_file c3.c
 '
 
 conflict_count()

-- 
2.42.0.345.gaab89be2eb
Junio C Hamano· Oct 16, 2023, 16:53 UTC · re: Dorcas AnonoLitunya · lore

Re: [PATCH] t/t7601: Modernize test scripts using functions

Dorcas AnonoLitunya <anonolitunya@gmail.com> writes:
> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
Let's try if we can pack a bit more information.  For example
Subject: [PATCH] t7601: use "test_path_is_file" etc. instead of "test -f"
would clarify what kind of modernization is done by this patch.
> The test script is currently using the command format 'test -f' to
> check for existence or absence of files.
"is currently using" -> "uses".
> Replace it with new helper functions following the format
> 'test_path_is_file'.

I am not sure what role "the format" plays in this picture. test_path_is_file is not new---it has been around for quite a while.

> Consequently, the patch also replaces the inverse command '! test -f' or
> 'test ! -f' with new helper function following the format
> 'test_path_is_missing'
A bit more on this later.
> This adjustment using helper functions makes the code more readable and
> easier to understand.

Looking good. If I were writing this, I'll make the whole thing more like this, though:

    t7601: use "test_path_is_file" etc. instead of "test -f"
    Some tests in t7601 use "test -f" and "test ! -f" to see if a
    path exists or is missing.  Use test_path_is_file and
    test_path_is_missing helper functions to clarify these tests a
    bit better.  This especially matters for the "missing" case,
    because "test ! -f F" will be happy if "F" exists as a
    directory, but the intent of the test is that "F" should not
    exist, even as a directory.
Show 16 quoted lines
> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index bd238d89b0..e08767df66 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' '
>  
>  test_expect_success 'merge c1 with c2' '
>  	git reset --hard c1 &&
> -	test -f c0.c &&
> -	test -f c1.c &&
> -	test ! -f c2.c &&
> -	test ! -f c3.c &&
> +	test_path_is_file c0.c &&
> +	test_path_is_file c1.c &&
> +	test_path_is_missing c2.c &&
> +	test_path_is_missing c3.c &&

The original says "We are happy if c2.c is not a file", so it would have been happy if by some mistake "git reset" created a directory there. But the _intent_ of the test is that we do not have anything at c2.c, and the updated code expresses it better.

← back to recent threads