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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 5, 2019, 00:17 UTC
Message-ID: <CAPig+cTTJgXERud0=svc5b+ctwQxoQ6cmpiA7WHMa7TUZ37BgQ@mail.gmail.com>
In-Reply-To: <20190304120801.28763-2-rohit.ashiwal265@gmail.com>

On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:

Show 12 quoted lines

> 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).