Re: [GSoC PATCH] t9400: prefer test_path_* helper functions

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 10, 2025, 06:50 UTC
Message-ID: <Z86LrOEhH3CJOIey@pks.im>
In-Reply-To: <20250308090358.25429-1-contact@aynp.dev>

On Sat, Mar 08, 2025 at 06:03:58PM +0900, Aryan Pathania wrote:

> use `test_path_*` instead of `test -[efd]` to avoid false complaints and

Nit: We want to have full sentences in commit messages. Those sentences
should start with an upper-case letter and end with punctuation.

> output when running the script with `-v` or `-x`

Hm. I'm not exactly sure what "false complaints" you are talking about here. The benefit of `test_path_*`()` over `test -[efd]` is that they actually print information _why_ they have failed, which may help devs to figure out what's happening. So it's kind of the opposite of what you say in the commit message: we're now printing _more_ output, not less.

Show 15 quoted lines

> Signed-off-by: Aryan Pathania <contact@aynp.dev>
> ---
>  t/t9400-git-cvsserver-server.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
> index e499c7f955..6c7cb1964c 100755
> --- a/t/t9400-git-cvsserver-server.sh
> +++ b/t/t9400-git-cvsserver-server.sh
> @@ -254,7 +254,7 @@ test_expect_success 'gitcvs.enabled = false' \
>       true
>     fi &&
>     grep "GITCVS emulation disabled" cvs.log &&
> -   test ! -d cvswork2'
> +   test_path_is_missing cvswork2'

This isn't quite equivalent: we've been checking that the path is not a directory before, but now we verify that the path doesn't exist. It's a sensible change in this context though as our new assertion is stronger than the old one, but it might make sense to point out in the commit message.

Show 9 quoted lines

> @@ -285,7 +285,7 @@ test_expect_success 'gitcvs.dbname' '
>  	GIT_DIR="$SERVERDIR" git config gitcvs.dbname %Ggitcvs.%a.%m.sqlite &&
>  	GIT_CONFIG="$git_config" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&
>  	test_cmp cvswork cvswork2 &&
> -	test -f "$SERVERDIR/gitcvs.ext.main.sqlite" &&
> +	test_path_is_file_not_symlink "$SERVERDIR/gitcvs.ext.main.sqlite" &&
>  	cmp "$SERVERDIR/gitcvs.main.sqlite" "$SERVERDIR/gitcvs.ext.main.sqlite"
>  '
>

We tend to use `test_path_is_file` rather than `test_path_is_file_not_symlink`, but I don't mind it too much.

Patrick