From: Junio C Hamano <gitster@pobox.com>
To: Dorcas AnonoLitunya <anonolitunya@gmail.com>
Cc: christian.couder@gmail.com, git@vger.kernel.org
Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
Date: Mon, 16 Oct 2023 09:53:55 -0700 [thread overview]
Message-ID: <xmqq1qdumrto.fsf@gitster.g> (raw)
In-Reply-To: <20231016152113.135970-1-anonolitunya@gmail.com> (Dorcas AnonoLitunya's message of "Mon, 16 Oct 2023 18:21:00 +0300")
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.
> 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.
next prev parent reply other threads:[~2023-10-16 16:53 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-16 15:21 [PATCH] t/t7601: Modernize test scripts using functions Dorcas AnonoLitunya
2023-10-16 16:53 ` Junio C Hamano [this message]
2023-10-16 18:43 Dorcas Litunya
2023-10-17 20:21 ` none Junio C Hamano
2023-10-18 12:52 ` [PATCH] t/t7601: Modernize test scripts using functions Dorcas Litunya
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq1qdumrto.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=anonolitunya@gmail.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).