[4.5/12] t5520: replace test -f with test-lib functions
diff mbox series

Message ID 542cd04c2ebda88b8fa63dc4dcb1c42d10afc844.1571355109.git.liu.denton@gmail.com
State New
Headers show
Series
  • Untitled series #189563
Related show

Commit Message

Denton Liu Oct. 17, 2019, 11:35 p.m. UTC
Although `test -f` has the same functionality as test_path_is_file(), in
the case where test_path_is_file() fails, we get much better debugging
information.

Replace `test -f` with test_path_is_file() so that future developers
will have a better experience debugging these test cases. Also, in the
case of `! test -f`, replace it with test_path_is_missing().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
I just realised that test_path_is_missing() is a much better replacement
than `test_must_fail test_path_is_file`. Please use this patch instead
of 04/12.

 t/t5520-pull.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Eric Sunshine Oct. 18, 2019, 12:11 a.m. UTC | #1
On Thu, Oct 17, 2019 at 7:35 PM Denton Liu <liu.denton@gmail.com> wrote:
> Although `test -f` has the same functionality as test_path_is_file(), in
> the case where test_path_is_file() fails, we get much better debugging
> information.
>
> Replace `test -f` with test_path_is_file() so that future developers
> will have a better experience debugging these test cases. Also, in the
> case of `! test -f`, replace it with test_path_is_missing().
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> I just realised that test_path_is_missing() is a much better replacement
> than `test_must_fail test_path_is_file`.

That depends upon context...

> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -99,7 +99,7 @@ test_expect_success 'pulling into void must not create an octopus' '
> -               ! test -f file
> +               test_path_is_missing file

There is a semantic difference between checking that "file" is not a
_file_ versus checking that the a path itself (which may be a
directory or a "special file") does not exist. In this particular
test, the difference doesn't matter, so the conversion is sensible
enough, but it would save reviewers time if you, as author of the
patch, state that you carefully considered the distinction before
making each change.

Patch
diff mbox series

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 55560ce3cd..004d5884cd 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -39,8 +39,8 @@  test_expect_success 'pulling into void' '
 		cd cloned &&
 		git pull ..
 	) &&
-	test -f file &&
-	test -f cloned/file &&
+	test_path_is_file file &&
+	test_path_is_file cloned/file &&
 	test_cmp file cloned/file
 '
 
@@ -50,8 +50,8 @@  test_expect_success 'pulling into void using master:master' '
 		cd cloned-uho &&
 		git pull .. master:master
 	) &&
-	test -f file &&
-	test -f cloned-uho/file &&
+	test_path_is_file file &&
+	test_path_is_file cloned-uho/file &&
 	test_cmp file cloned-uho/file
 '
 
@@ -99,7 +99,7 @@  test_expect_success 'pulling into void must not create an octopus' '
 	(
 		cd cloned-octopus &&
 		test_must_fail git pull .. master master &&
-		! test -f file
+		test_path_is_missing file
 	)
 '