Re: [PATCH v5] tests: use test_path_is_missing instead of '! test -f'

[Junio C Hamano <gitster@pobox.com>]

Jayesh Daga <jayeshdaga99@gmail.com> writes:

> Replace a raw '! test -f' check with test_path_is_missing.

Did you already say that on the commit title?

> The test_path_is_missing helper integrates with Git’s test
> framework and produces clearer failure output.
>
> In contrast,
> a plain shell '! test -f' check only reports a generic failure
> status, which makes it harder to understand whether the file
> unexpectedly exists or if another issue caused the test to fail.

"clearer" probably is not clear enough, but don't add more words on
it.

The problem with using "test", whether negated or not, is that they
*silently* succeed or fail.  Take a typical test that does a bunch
of things like this ...

	do something &&
	do something else &&
	test -f this_must_be_a_file &&
	test ! -e this_must_not_exist &&
	do yet another thing &&
	! test -d this_should_not_be_a_directory

... and expects all of them to succeed.  If it fails in one of the
steps, it is impossible to see from the test output, even when you
are running with the "-v" option , e.g., "sh t/0601-*.sh -v", where
in the sequence it failed.  Maybe "do something" and "do something
else" shows different messages so you can tell these two steps
succeeded, but did the test fail because this_must_be_a_file did not
exist, or was it because a filesystem entity this_must_not_exist
existed?

Our test helpers improve by being loud when the expectation is not
met.  When "test ! -e this_must_not_exist" is rewritten with
"test_path_is_missing this_must_not_exist", and when that thing is
missing from the filesystem, test_path_is_missing will succeed
silently.  But whe it exists, it loudly reports "We did not want to
see it, but it exists!", when it fails.

    Using plain "test" commands in a series of tests concatenated
    with && makes it hard to tell from the failure output which one
    of the steps failed, since "test" silently succeeds and fails.

    In this partciular instance, we expect that ".git/refs/heads/f"
    should no longer exist in the filesystem.  test_path_is_missing
    helper function silently succeeds, as does "! test -f", when it
    finds that the file is not there, but it will loudly report when
    the file exists, contrary to our expectation, which makes it
    easier to debug a test failure.

or something like that.

> It also avoids relying on negated shell conditions, making the
> test easier to read and understand.

It is not a single test being "hard to understand".  As a developer,
you are expected to know what "! test -f .git/refs/heads/f" expects
(i.e., it does not want to see a file there).


> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
> ---
> v5:
> - Clarify rationale for using test helper
> - Explain diagnostic improvement and negation issues
> - Address review comments on vague wording
>
> v4:
> - Correct commit message to match actual change
> - Improve rationale (diagnostics, consistency)
> - Move version notes below '---'
> - Fix author name to match sign-off
>
> v3:
> - Fix commit message wording
> ---
>  t/pack-refs-tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
> index 2fdaccb6c7..4a85d96c6b 100644
> --- a/t/pack-refs-tests.sh
> +++ b/t/pack-refs-tests.sh
> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune
>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '
>  	git branch f &&
>  	git ${pack_refs} --all --prune &&
> -	! test -f .git/refs/heads/f
> +	test_path_is_missing .git/refs/heads/f
>  '
>  
>  test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '