diff mbox series

[2/3] Revert and amend "test-lib-functions: assert correct parameter count"

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

Commit Message

Ævar Arnfjörð Bjarmason April 17, 2021, 12:58 p.m. UTC
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(-)

Comments

Eric Sunshine April 18, 2021, 5:11 a.m. UTC | #1
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/
Ævar Arnfjörð Bjarmason April 20, 2021, 12:25 p.m. UTC | #2
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 mbox series

Patch

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
 }