Re: [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty`

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> Add a helper function to ensure that a given path is a non-empty file,
> and give an error message when it is not.
>
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -593,6 +593,15 @@ test_dir_is_empty () {
> +# Check if the file exists and has a size greater than zero
> +test_file_not_empty () {
> +       if ! test -s "$1"
> +       then
> +               echo "'$1' is not a non-empty file."

Although not incorrect, the double-negative is hard to digest. I had
to read it a few times to convince myself that it matched the intent
of the new function. I wonder if a message such as

    echo "'$1' is unexpectedly empty"

would be better. (Subjective, and not at all worth a re-roll.)

> +               false
> +       fi
> +}
>  test_path_is_missing () {

Much later in this same file, a function named test_must_be_empty() is
defined, which is the complement of your new test_file_not_empty()
function. The dissimilar names may cause confusion, so choosing a name
more like the existing function might be warranted.

Also, it might be a good idea to add this new function as a neighbor
of test_must_be_empty() rather than defining it a couple hundred lines
earlier in the file. Alternately, perhaps a preparatory patch could
move test_must_be_empty() closer to the other similar functions
(test_path_is_missing() and cousins).