[GSoC PATCH] t1420-lost-found.sh: use test_path_is_file for error logging

[GSoC PATCH] t1420-lost-found.sh: use test_path_is_file for error logging

[Andrew Chitester]

This test will fail silently without giving any error message. Use
test_path_is_file in place of test -f to ensure this test errors with a
message.

Signed-off-by: Andrew Chitester <andchi@fastmail.com>
---
 t/t1420-lost-found.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1420-lost-found.sh b/t/t1420-lost-found.sh
index 2fb2f44f02..5fbb1d10ed 100755
--- a/t/t1420-lost-found.sh
+++ b/t/t1420-lost-found.sh
@@ -29,8 +29,8 @@ test_expect_success 'lost and found something' '
 	git reset --hard HEAD^ &&
 	git fsck --lost-found &&
 	test 2 = $(ls .git/lost-found/*/* | wc -l) &&
-	test -f .git/lost-found/commit/$(cat lost-commit) &&
-	test -f .git/lost-found/other/$(cat lost-other)
+	test_path_is_file .git/lost-found/commit/$(cat lost-commit) &&
+	test_path_is_file .git/lost-found/other/$(cat lost-other)
 '
 
 test_done
-- 
2.52.0

Re: [GSoC PATCH] t1420-lost-found.sh: use test_path_is_file for error logging

[Junio C Hamano]

Andrew Chitester <andchi@fastmail.com> writes:

> This test will fail silently without giving any error message. Use
> test_path_is_file in place of test -f to ensure this test errors with a
> message.
>
> Signed-off-by: Andrew Chitester <andchi@fastmail.com>
> ---
>  t/t1420-lost-found.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1420-lost-found.sh b/t/t1420-lost-found.sh
> index 2fb2f44f02..5fbb1d10ed 100755
> --- a/t/t1420-lost-found.sh
> +++ b/t/t1420-lost-found.sh
> @@ -29,8 +29,8 @@ test_expect_success 'lost and found something' '
>  	git reset --hard HEAD^ &&
>  	git fsck --lost-found &&
>  	test 2 = $(ls .git/lost-found/*/* | wc -l) &&
> -	test -f .git/lost-found/commit/$(cat lost-commit) &&
> -	test -f .git/lost-found/other/$(cat lost-other)
> +	test_path_is_file .git/lost-found/commit/$(cat lost-commit) &&
> +	test_path_is_file .git/lost-found/other/$(cat lost-other)
>  '

Looks correct, but given that what these tests want to ensure is
that underneath .git/lost-found there are only these two expected
files, I have to wonder if the output of "ls" here is expected to be
very stable.  I.e. if we rewrote the whole thing to something like
...

	ls .git/lost-found/*/* >actual &&
	cat >expect <<-EOF &&
	.git/lost-found/commit/$(cat lost-commit)
	.git/lost-found/other/$(cat lost-other)
	EOF
	test_cmp expect actual

... would it be a more direct way to say that and is easier to
understand to our readers.

[GSoC PATCH v2 1/1] t1420: modernize the lost-found test

[Andrew Chitester]

This test indirectly checks that the lost-found folder has 2 files in it
and then checks that the expected two files exist. Make this more
deliberate by removing the old test -f and compare the actual ls of the
lost-found directory with the expected files.

Signed-off-by: Andrew Chitester <andchi@fastmail.com>
---
 t/t1420-lost-found.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t1420-lost-found.sh b/t/t1420-lost-found.sh
index 2fb2f44f02..926c6d63e3 100755
--- a/t/t1420-lost-found.sh
+++ b/t/t1420-lost-found.sh
@@ -28,9 +28,12 @@ test_expect_success 'lost and found something' '
 	test_tick &&
 	git reset --hard HEAD^ &&
 	git fsck --lost-found &&
-	test 2 = $(ls .git/lost-found/*/* | wc -l) &&
-	test -f .git/lost-found/commit/$(cat lost-commit) &&
-	test -f .git/lost-found/other/$(cat lost-other)
+	ls .git/lost-found/*/* >actual &&
+	cat >expect <<-EOF &&
+	.git/lost-found/commit/$(cat lost-commit)
+	.git/lost-found/other/$(cat lost-other)
+	EOF
+	test_cmp expect actual
 '
 
 test_done
-- 
2.52.0

Re: [GSoC PATCH] t1420-lost-found.sh: use test_path_is_file for error logging

[Andrew Chitester]

Junio C Hamano <gitster@pobox.com> writes:

> Looks correct, but given that what these tests want to ensure is
> that underneath .git/lost-found there are only these two expected
> files, I have to wonder if the output of "ls" here is expected to be
> very stable.  I.e. if we rewrote the whole thing to something like
> ...
>
> 	ls .git/lost-found/*/* >actual &&
> 	cat >expect <<-EOF &&
> 	.git/lost-found/commit/$(cat lost-commit)
> 	.git/lost-found/other/$(cat lost-other)
> 	EOF
> 	test_cmp expect actual
>
> ... would it be a more direct way to say that and is easier to
> understand to our readers.

Thanks for the feedback. This is an elegant solution that I did not
consider. Looking through the other tests, I am seeing this similar
pattern of comparing an expected result with the actual result. It is
much more deliberate and readable this way. I sent a v2, as a reply to
my original message, but I think I forgot to Cc you in that message. I'm
still figuring out the email workflow.