Re: [PATCH v3 1/2] t1092: add tests for `git diff-files`

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 9, 2023, 03:00 UTC
Message-ID: <xmqqlek6pr2b.fsf@gitster.g>
In-Reply-To: <20230309013314.119128-2-cheskaqiqi@gmail.com>

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> +	! test_file_not_empty sparse-checkout-out &&

If you looked at existing uses of this test helper, you probably wouldn't have written this.

    $ git grep -e test_file_not_empty t/t[0-9]\*.sh | wc -l
    34
    $ git grep -e '! test_file_not_empty' t/t[0-9]\*.sh | wc -l
    0

This is because test_file_not_empty is designed to fail loudly with a complaint when the given file is empty. Its implementation reads like so:

        # Check if the file exists and has a size greater than zero
        test_file_not_empty () {
                test "$#" = 2 && BUG "2 param"
                if ! test -s "$1"
                then
                        echo "'$1' is not a non-empty file."
                        false
                fi
        }

In the successful case in your test, you expect the file to be empty (I didn't check if it should be empty or not---I am just taking your word for it). It means that the "! test_file_not_empty" is expected to keep complaining that it is NOT a non-empty file.

Not very nice, no?

Perhaps test_must_be_empty is what you wanted to use.

Show 10 quoted lines

> +	! test_file_not_empty sparse-index-out &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	! test_file_not_empty sparse-checkout-out &&
> +	! test_file_not_empty sparse-index-out &&
> +
> +	# file present on-disk with modifications
> +	run_on_sparse ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git diff-files &&
> +	test_sparse_match git diff-files newdirectory/testfile &&
> +	test_file_not_empty sparse-checkout-out

Now, are we happy if the file is not empty and has any garbage in it? Don't we know what the list of different paths are and what the common output between the two should look like?

In general "as long as it is not empty, any garbage is fine" is a poor primitive to use in tests, unless (1) we are testing output that is deliberately designed to be unstable, or (2) we know the program that produces output will always show an empty result when it fails in any way.

> +'
> +
>  test_done

Thanks.