Re: [PATCH] tests: turn on test-lint-shell-syntax by default

From: Torsten Bögershausen <tboegi@web.de>
Date: Feb 5, 2013, 20:36 UTC
Message-ID: <51116D3E.3070409@web.de>
In-Reply-To: <7v4ni2y1fm.fsf@alter.siamese.dyndns.org>

On 27.01.13 18:34, Junio C Hamano wrote:

Show 68 quoted lines

> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Back to the which:
>> ...
>> and running "make test" gives the following, at least in my system:
>> ...
> I think everybody involved in this discussion already knows that;
> the point is that it can easily give false negative, without the
> scripts working very hard to do so.
>
> If we did not care about incurring runtime performance cost, we
> could arrange:
>
>  - the test framework to define a variable $TEST_ABORT that has a
>    full path to a file that is in somewhere test authors cannot
>    touch unless they really try hard to (i.e. preferrably outside
>    $TRASH_DIRECTORY, as it is not uncommon for to tests to do "rm *"
>    there). This location should be per $(basename "$0" .sh) to allow
>    running multiple tests in paralell;
>
>  - the test framework to "rm -f $TEST_ABORT" at the beginning of
>    test_expect_success/failure;
>
>  - test_expect_success/failure to check $TEST_ABORT and if it
>    exists, abort the execution, showing the contents of the file as
>    an error message.
>
> Then you can wrap commands whose use we want to limit, perhaps like
> this, in the test framework:
>
> 	which () {
> 		cat >"$TEST_ABORT" <<-\EOF
> 		Do not use unportable 'which' in the test script.
>                 "if type $cmd" is a good way to see if $cmd exists.
> 		EOF
> 	}
>
> 	sed () {
> 		saw_wantarg= must_abort=
>                 for arg
>                 do
> 			if test -n "$saw_wantarg"
>                         then
> 				saw_wantarg=
>                                 continue
> 			fi
> 			case "$arg" in
> 			--)	break ;; # end of options
> 			-i)	echo >"$TEST_ABORT" "Do not use 'sed -i'"
> 				must_abort=yes
> 				break ;;
>                         -e)	saw_wantarg=yes ;; # skip next arg
> 			-*)	continue ;; # options without arg
> 			*)	break ;; # filename
> 			esac
> 		done
> 		if test -z "$must_abort"
> 			sed "$@"
> 		fi
> 	}
>
> Then you can check that TEST_ABORT does not appear in test scripts
> (ensuring that they do not attempt to circumvent the mechanis) and
> catch use of unwanted commands or unwanted extended features of
> commands at runtime.
>
> But this will incur runtime performace hit, so I am not sure it
> would be worth it.

Thanks for the detailed suggestion. Instead of using a file for putting out non portable syntax, can we can use a similar logic as test_failure ?

I did some benchmarking, the test suite on a Laptop is 37..38 minutes, including make clean && make both on next, pu, master or with the patch below. I couldn't find a measurable impact on the execution time. What do we think about a patch like this (Not sure if this cut-and-paste data applies, it's for review)

[PATCH] test-lib: which, echo -n and sed -i are not portable

The posix version of sed command supports options -n -e -f The gnu extension -i to edit a file in place is not available on all systems. To catch the usage of non-posix options with sed a wrapper function is added in test-lib.sh. The wrapper checks that only -n -e -f are used. The short form "-ne" for "-n -e" is allowed as well.

echo -n is not portable and not available on all systems, printf can be used instead. Add a wrapper to catch echo -n

which is not portable, the output differs between different implementations, and the return code may not be reliable.

Add a function test_bad_syntax_ in test-lib.sh, which increments the variable test_bad_syntax and works similar to test_failure_

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 t/test-lib.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1a6c4ab..248ed34 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -266,6 +266,7 @@ else
     exec 4>/dev/null 3>/dev/null
 fi
 
+test_bad_syntax=0
 test_failure=0
 test_count=0
 test_fixed=0
@@ -300,6 +301,12 @@ test_ok_ () {
     say_color "" "ok $test_count - $@"
 }
 
+test_bad_syntax_ () {
+    test_bad_syntax=$(($test_bad_syntax + 1))
+    say_color error "$@"
+    test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+}
+
 test_failure_ () {
     test_failure=$(($test_failure + 1))
     say_color error "not ok $test_count - $1"
@@ -402,10 +409,15 @@ test_done () {
         fixed $test_fixed
         broken $test_broken
         failed $test_failure
+        bad_syntax $test_bad_syntax
 
         EOF
     fi
 
+    if test "$test_bad_syntax" != 0
+    then
+        say_color error "# $test_bad_syntax non portable shell syntax"
+    fi
     if test "$test_fixed" != 0
     then
         say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
@@ -645,6 +657,40 @@ yes () {
     done
 }
 
+
+# which is not portable
+which () {
+    test_bad_syntax_ "Do not use unportable 'which' in the test script." \
+            "'if type $1' is a good way to see if '$1' exists."
+    return 1
+}
+
+# catch non-portable usage of sed
+sed () {
+    for arg
+    do
+        case "$arg" in
+    -[efn]) continue ;; # allowed posix options
+    -ne) continue ;; # tolerated
+        -*)test_bad_syntax_ "Do not use 'sed "$arg"'. Valid options for 'sed' are -n -e -f"
+            return 1
+            ;;
+        *) continue ;;
+        esac
+    done
+    command sed "$@"
+}
+
+# catch non portable echo -n
+echo () {
+    if test "$1" = -n
+    then
+        test_bad_syntax_ "Do not use 'echo -n'. Use printf instead"
+    else
+        command echo "$@"
+    fi
+}
+
 # Fix some commands on Windows
 case $(uname -s) in
 *MINGW*)

-- 
1.8.1.1