Re: [PATCH 15/16] Revert and amend "test-lib-functions: assert correct parameter count"
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Apr 15, 2021, 22:05 UTC
- Message-ID
- <20210415220526.GO2947267@szeder.dev>
- In-Reply-To
- <patch-15.16-0cd511206c4-20210412T110456Z-avarab@gmail.com>
On Mon, Apr 12, 2021 at 01:09:04PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 23 quoted lines
> 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. > > When I added these BUG assertions in e7884b353b7 I missed that this > made the -x output much more verbose. > > E.g. for each test_cmp invocation we'd now 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 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. > > Let's instead use "$@" instead of "$1" to achieve the same ends with > no extra -x output verbosity. The "test" operator will die with an > error if given more than one argument in these contexts, so using "$@" > achieves the same goal.
I prefer the current check for its explicitness over the implicit and somewhat cryptic approach introduced in this patch. I hope that sooner or later I'll finish up my patch series to suppress '-x' output from test helper functions, and then this issue will become moot anyway.
Show 118 quoted lines
> The same goes for "cmp" and "diff -u" (which we typically use for
> test_cmp).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> t/test-lib-functions.sh | 41 ++++++++++++++++-------------------------
> 1 file changed, 16 insertions(+), 25 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index c46bf0ff09c..2cf72b56851 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -759,39 +759,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"
> + 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"
> + 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"
> + 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
> @@ -799,17 +795,15 @@ 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."
> + 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!"
> false
> @@ -1013,7 +1007,6 @@ test_expect_code () {
> # - not all diff versions understand "-u"
>
> test_cmp () {
> - test "$#" -ne 2 && BUG "2 param"
> eval "$GIT_TEST_CMP" '"$@"'
> }
>
> @@ -1043,7 +1036,6 @@ test_cmp_config () {
> # test_cmp_bin - helper to compare binary files
>
> test_cmp_bin () {
> - test "$#" -ne 2 && BUG "2 param"
> cmp "$@"
> }
>
> @@ -1104,12 +1096,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
> }
> --
> 2.31.1.634.gb41287a30b0
>