Message ID | patch-2.3-6f9e09a2017-20210417T125540Z-avarab@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | test-lib-functions.sh: trickery to make -x less verbose | expand |
On Sat, Apr 17, 2021 at 8:58 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. My one concern about this change is that it enters the realm of "huh?". Although it's dead-simple to understand what this code is doing: test "$#" -ne 1 && BUG "1 param" if ! test -f "$1" the replacement code: if ! test -f "$@" can easily lead to head-scratching, wondering why the author chose to use "$@" rather than the more obvious "$1". This sort of unusual local idiom might normally deserve an in-code comment explaining why the more obvious code is not employed. However, adding a comment at each site would be overkill, so it might be the sort of thing to explain in documentation somewhere ("In order to make -x output less noisy, employ "$@" rather than explicitly checking function arguments when writing new test functions...".) Otherwise, the reader is forced to consult the commit message. Not a major objection; just voicing the bit of unease I feel about it. > This does not to the "right" thing in cases like: Channeling Dscho's inner Eric Sunshine[1]: s/to/do/ [1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103222235150.50@tvgsbejvaqbjf.bet/
On Sun, Apr 18 2021, Eric Sunshine wrote: > On Sat, Apr 17, 2021 at 8:58 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. > > My one concern about this change is that it enters the realm of > "huh?". Although it's dead-simple to understand what this code is > doing: > > test "$#" -ne 1 && BUG "1 param" > if ! test -f "$1" > > the replacement code: > > if ! test -f "$@" > > can easily lead to head-scratching, wondering why the author chose to > use "$@" rather than the more obvious "$1". This sort of unusual local > idiom might normally deserve an in-code comment explaining why the > more obvious code is not employed. However, adding a comment at each > site would be overkill, so it might be the sort of thing to explain in > documentation somewhere ("In order to make -x output less noisy, > employ "$@" rather than explicitly checking function arguments when > writing new test functions...".) Otherwise, the reader is forced to > consult the commit message. > > Not a major objection; just voicing the bit of unease I feel about it. I'm including a well-placed comment in the re-roll of this. FWIW this would be much easier to follow with my rejected split-up of test-lib-functions.sh, then we could group these functions into one file: https://lore.kernel.org/git/20210209214159.22815-13-avarab@gmail.com/ >> This does not to the "right" thing in cases like: > > Channeling Dscho's inner Eric Sunshine[1]: s/to/do/ Sunshine recursion error? :) > [1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103222235150.50@tvgsbejvaqbjf.bet/
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 0232cc9f46d..9f6d46b8b1b 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -758,39 +758,35 @@ test_external_without_stderr () { # debugging-friendly alternatives to "test [-f|-d|-e]" # The commands test the existence or non-existence of $1 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 +794,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 +1006,6 @@ test_expect_code () { # - not all diff versions understand "-u" test_cmp () { - test "$#" -ne 2 && BUG "2 param" eval "$GIT_TEST_CMP" '"$@"' } @@ -1042,7 +1035,6 @@ test_cmp_config () { # test_cmp_bin - helper to compare binary files test_cmp_bin () { - test "$#" -ne 2 && BUG "2 param" cmp "$@" } @@ -1103,12 +1095,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 | 51 +++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 30 deletions(-)