Re: [PATCH] tests: turn on test-lint-shell-syntax by default
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(+)
Show changes to t/test-lib.sh +46 −0
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
Torsten Bögershausen
