| Message ID | 20190304120801.28763-2-rohit.ashiwal265@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use helper functions in test script | expand |
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).
Eric Sunshine <sunshine@sunshineco.com> writes: >> +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.) Yeah, that is subjective. The expectation of the test is "not-empty", so I do not see this double-negation as being too bad, though. > 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). Very good suggestions. Looking at neighbouring helpers around must-be-empty, it seems to me that the latter, i.e. moving it to sit next to other "path" helpers, would make the most sense. Thanks.
Hello Eric On 2019-03-04 19:17:50 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote: > On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > > 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.) I think the current message is more accurate as it implies both: 1. There is no file, and 2. If there is, it is not empty "unexpectedly empty" may imply that there is a directory which is not empty and that is not the intention of the function. > 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). I think we should relocate the function `test_must_be_empty` in a separate patch as this patch deals with a different issue. Thanks Rohit
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 80402a428f..681c41ba32 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -593,6 +593,15 @@ test_dir_is_empty () { fi } +# 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." + false + fi +} + test_path_is_missing () { if test -e "$1" then
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> --- t/test-lib-functions.sh | 9 +++++++++ 1 file changed, 9 insertions(+)