From: "Torsten Bögershausen" <tboegi@web.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Torsten Bögershausen" <tboegi@web.de>,
"Jonathan Nieder" <jrnieder@gmail.com>,
git@vger.kernel.org, kraai@ftbfs.org
Subject: Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Date: Tue, 05 Feb 2013 21:36:14 +0100 [thread overview]
Message-ID: <51116D3E.3070409@web.de> (raw)
In-Reply-To: <7v4ni2y1fm.fsf@alter.siamese.dyndns.org>
On 27.01.13 18:34, Junio C Hamano wrote:
> 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
next prev parent reply other threads:[~2013-02-05 20:37 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-12 5:50 [PATCH] tests: turn on test-lint-shell-syntax by default Torsten Bögershausen
2013-01-12 6:00 ` Junio C Hamano
2013-01-13 10:25 ` Torsten Bögershausen
2013-01-13 16:50 ` Matt Kraai
2013-01-13 17:32 ` Jonathan Nieder
2013-01-13 22:38 ` Junio C Hamano
2013-01-15 20:12 ` Torsten Bögershausen
2013-01-15 20:38 ` Junio C Hamano
2013-01-26 6:57 ` Torsten Bögershausen
2013-01-26 21:43 ` Junio C Hamano
2013-01-27 7:43 ` Torsten Bögershausen
2013-01-27 9:31 ` Jonathan Nieder
2013-01-27 13:13 ` Torsten Bögershausen
2013-01-27 17:34 ` Junio C Hamano
2013-01-27 20:25 ` Junio C Hamano
2013-02-05 20:36 ` Torsten Bögershausen [this message]
2013-02-05 20:52 ` Junio C Hamano
2013-02-05 21:56 ` Junio C Hamano
2013-01-27 17:15 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51116D3E.3070409@web.de \
--to=tboegi@web.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=kraai@ftbfs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).