| Message ID | patch-2.3-67ddd821df-20210420T122706Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib-functions.sh: trickery to make -x less verbose | expand |
On Tue, Apr 20, 2021 at 8:29 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > [...] > The goal here is to get rid of the verbosity of having e.g. a "test 2 > -ne 2" line for every "test_cmp". We use "$@" as an argument to "test" > to intentionally feed the "test" operator too many arguments if the > functions are called with too many arguments, thus piggy-backing on it > to check the number of arguments we get. > [...] > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > @@ -756,41 +756,43 @@ test_external_without_stderr () { > # debugging-friendly alternatives to "test [-f|-d|-e]" > +# The commands test the existence or non-existence of > +# a given argument. > +# > +# The pattern of using "$@" to "test" instead of "$1" is not a bug. We > +# are counting on "test" to error on too many arguments if more than > +# one is given. Checking "$#" explicitly would lead to overly verbose > +# -x output. > test_path_is_file () { > + if ! test -f "$@" Thanks. The new comment makes the intent of "$@" clear. If you do re-roll for some reason, it might make sense to move the new comment (the one starting "The pattern of...") into the function itself just before the use of "$@". The reasons I suggest this are: (1) the comment explains an implementation detail, thus is intended for people who might change this function, whereas all the text above the new paragraph is API documentation for callers of the function who need only the black-box description; (2) it places the comment closer to the relevant code, thus is less likely to be overlooked by someone changing the function.
On Tue, Apr 20 2021, Eric Sunshine wrote: > On Tue, Apr 20, 2021 at 8:29 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> [...] >> The goal here is to get rid of the verbosity of having e.g. a "test 2 >> -ne 2" line for every "test_cmp". We use "$@" as an argument to "test" >> to intentionally feed the "test" operator too many arguments if the >> functions are called with too many arguments, thus piggy-backing on it >> to check the number of arguments we get. >> [...] >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh >> @@ -756,41 +756,43 @@ test_external_without_stderr () { >> # debugging-friendly alternatives to "test [-f|-d|-e]" >> +# The commands test the existence or non-existence of >> +# a given argument. >> +# >> +# The pattern of using "$@" to "test" instead of "$1" is not a bug. We >> +# are counting on "test" to error on too many arguments if more than >> +# one is given. Checking "$#" explicitly would lead to overly verbose >> +# -x output. >> test_path_is_file () { >> + if ! test -f "$@" > > Thanks. The new comment makes the intent of "$@" clear. > > If you do re-roll for some reason, it might make sense to move the new > comment (the one starting "The pattern of...") into the function > itself just before the use of "$@". The reasons I suggest this are: > (1) the comment explains an implementation detail, thus is intended > for people who might change this function, whereas all the text above > the new paragraph is API documentation for callers of the function who > need only the black-box description; (2) it places the comment closer > to the relevant code, thus is less likely to be overlooked by someone > changing the function. I think moving it would be worse / wouldn't make sense. The comment doesn't just apply to test_path_is_file(), but the next N functions that use the "$@" pattern. Just like the existing "debugging-friendly alternatives" comment does. I.e. test_path_is_file is just the "-f" part of thta comment, the following test_path_is_dir is the "-d" etc.
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 0232cc9f46..f8f5bf9de1 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -756,41 +756,43 @@ test_external_without_stderr () { } # debugging-friendly alternatives to "test [-f|-d|-e]" -# The commands test the existence or non-existence of $1 +# The commands test the existence or non-existence of +# a given argument. +# +# The pattern of using "$@" to "test" instead of "$1" is not a bug. We +# are counting on "test" to error on too many arguments if more than +# one is given. Checking "$#" explicitly would lead to overly verbose +# -x output. test_path_is_file () { - test "$#" -ne 1 && BUG "1 param" - if ! test -f "$1" + if ! test -f "$@" then - echo "File $1 doesn't exist" - false + echo "File $* doesn't exist" + return 1 fi } test_path_is_dir () { - test "$#" -ne 1 && BUG "1 param" - if ! test -d "$1" + if ! test -d "$@" then - echo "Directory $1 doesn't exist" - false + echo "Directory $* doesn't exist" + return 1 fi } test_path_exists () { - test "$#" -ne 1 && BUG "1 param" - if ! test -e "$1" + if ! test -e "$@" then - echo "Path $1 doesn't exist" - false + echo "Path $* doesn't exist" + return 1 fi } # Check if the directory exists and is empty as expected, barf otherwise. test_dir_is_empty () { - test "$#" -ne 1 && BUG "1 param" - test_path_is_dir "$1" && - if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')" + test_path_is_dir "$@" && + if test -n "$(ls -a1 "$@" | egrep -v '^\.\.?$')" then - echo "Directory '$1' is not empty, it contains:" + echo "Directory '$*' is not empty, it contains:" ls -la "$1" return 1 fi @@ -798,19 +800,17 @@ test_dir_is_empty () { # 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" + if ! test -s "$@" then - echo "'$1' is not a non-empty file." - false + echo "'$*' is not a non-empty file." + return 1 fi } test_path_is_missing () { - test "$#" -ne 1 && BUG "1 param" - if test -e "$1" + if test -e "$@" then - echo "Path $1 exists!" + echo "Path $* exists!" false fi } @@ -1012,7 +1012,6 @@ test_expect_code () { # - not all diff versions understand "-u" test_cmp () { - test "$#" -ne 2 && BUG "2 param" eval "$GIT_TEST_CMP" '"$@"' } @@ -1042,7 +1041,6 @@ test_cmp_config () { # test_cmp_bin - helper to compare binary files test_cmp_bin () { - test "$#" -ne 2 && BUG "2 param" cmp "$@" } @@ -1103,12 +1101,11 @@ verbose () { # otherwise. test_must_be_empty () { - test "$#" -ne 1 && BUG "1 param" - test_path_is_file "$1" && - if test -s "$1" + test_path_is_file "$@" && + if test -s "$@" then - echo "'$1' is not empty, it contains:" - cat "$1" + echo "'$*' is not empty, it contains:" + cat "$@" return 1 fi }
This reverts and amends my my own e7884b353b7 (test-lib-functions: assert correct parameter count, 2021-02-12) in order to improve the -x output. The goal here is to get rid of the verbosity of having e.g. a "test 2 -ne 2" line for every "test_cmp". We use "$@" as an argument to "test" to intentionally feed the "test" operator too many arguments if the functions are called with too many arguments, thus piggy-backing on it to check the number of arguments we get. Before this for each test_cmp invocation we'd emit: + test_cmp expect actual + test 2 -ne 2 + eval diff -u "$@" + diff -u expect actual That "test 2 -ne 2" line is new in my e7884b353b7. As noted in 45a2686441b (test-lib-functions: remove bug-inducing "diagnostics" helper param, 2021-02-12) we had buggy invocations of some of these functions with too many parameters. Now we'll get just: + test_cmp expect actual + eval diff -u "$@" + diff -u expect actual This does not to the "right" thing in cases like: test_path_is_file x -a y Which will now turn into: test -f x -a y I consider that to be OK given the trade-off that any extra checking would produce more verbose trace output. As shown in 45a2686441b we had issues with these functions being invoked with multiple parameters (e.g. a glob) by accident, we don't need to be paranoid in guarding against hostile misuse from our own test suite. While I'm at it change a few functions that relied on a "false" being the last statement in the function to use an explicit "return 1" like the other functions in this file. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/test-lib-functions.sh | 59 +++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 31 deletions(-)