Re: [PATCH 1/1] t9117: prefer test_path_* helper functions

From: shejialuo <shejialuo@gmail.com>
Date: Mar 1, 2024, 11:29 UTC
Message-ID: <20240301112914.121184-1-shejialuo@gmail.com>
In-Reply-To: <CAPig+cRfO8t1tdCL6MB4b9XopF3HkZ==hU83AFZ38b-2zsXDjQ@mail.gmail.com>

Thanks for your comment.

> Although you make the test pass by using `!` to invert the result of
> `test_path_is_dir`, the complaint will nevertheless get lodged, and
> may very well be confusing for anyone scrutinizing the output of the
> tests when running the script with `-v` or `-x`.

I have run the script with `-v`, I have got the following result:

  Directory trunk doesn't exist

I come to realisize the fault with your dedicated comments. An assertion is an assertion.

And I am impressed by the following idea:

Show 15 quoted lines

> Rather than asserting some initial condition at the start of the test,
> it is more common and more robust simply to _ensure_ that the desired
> initial condition holds. So, for instance, instead of asserting `test
> ! -d target`, modern practice is to ensure that `target` doesn't
> exist. Thus:
>
>    test_expect_success 'clone to target directory' '
>        rm -rf target &&
>        git svn clone "$svnrepo"/project/trunk target &&
>        ...
>
> is a more robust implementation. This also addresses the problem that
> the `rm -rf target` at the very end of each test won't be executed if
> any command earlier in the test fails (due to the short-circuiting
> behavior of the &&-operator).

The command `rm -rf target` ensures an exit status of 0 regardless of whether the `target` exists. Thus the code will elegant make sure the initial condition holds. I think I could add a patch to clean the code.