From: Junio C Hamano <gitster@pobox.com>
To: Thomas Gummerer <t.gummerer@gmail.com>
Cc: git@vger.kernel.org, trast@inf.ethz.ch
Subject: Re: [PATCH 2/2] perf-lib: add test_perf_cleanup target
Date: Thu, 19 Sep 2013 12:52:50 -0700 [thread overview]
Message-ID: <xmqqtxhgsi5p.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <xmqqk3icu3u8.fsf@gitster.dls.corp.google.com> (Junio C. Hamano's message of "Thu, 19 Sep 2013 10:19:11 -0700")
Junio C Hamano <gitster@pobox.com> writes:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
>> When one performance test fails, the testing is aborted and the cleanup
>> commands are not executed anymore, leaving the trash directory in the
>> failed state.
>
> Ah, that I overlooked. In that case, the comments in my previous
> message do not apply.
Having said that, it was unclear to me why we would want a new
test_perf_cleanup added.
The name is misleading (does it define only clean-up actions?) to
say the least, and one way of fixing it would be to call it
test_perf_with_cleanup.
I wondered why this clean-up section cannot be an optional parameter
to test_perf, but that would not fly well because we won't know if
3-arg form is with one prerequisite and no clean-up, or no prereq
with a clean-up, so perhaps adding a new function may be the best
you could do.
But in the longer term, I think we would be better off to allow two
styles (one traditional, another modern) and add new features only
to the "modern" (aka "more easily extensible") form:
test_perf [PREREQ] BODY
test_perf [--prereq PREREQ] [--cleanup CLEANUP] ... BODY
perhaps like this (this is without --cleanup but it should be
obvious how to add a support for it to the command line parser).
The patch to t0008 is primarily to adjust for test labels that begin
with double-dashes that will be mistaken as a new-style option, but
it is unnecessarily big because it uses random custom shell functions
to hide the real calls to underlying test_expect_success X-<.
t/test-lib-functions.sh | 72 ++++++++++++++++++++++++++++++++++++++-----------
t/perf/perf-lib.sh | 17 +++++-------
t/t0008-ignores.sh | 43 +++++++++++++++--------------
3 files changed, 87 insertions(+), 45 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index a7e9aac..10202dc 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -342,20 +342,65 @@ test_declared_prereq () {
return 1
}
+test_expect_parse () {
+ whoami=$1
+ shift
+ test_expect_new_style=
+ while case $# in 0) false ;; esac
+ do
+ case "$1" in
+ --prereq)
+ test $# -gt 1 ||
+ error "bug in the test script: --prereq needs a parameter"
+ test_prereq=$2
+ shift
+ ;;
+ --)
+ shift
+ break
+ ;;
+ --*)
+ error "bug in the test script: unknown option '$1'"
+ ;;
+ *)
+ break
+ ;;
+ esac
+ test_expect_new_style=yes
+ shift
+ done
+
+ # Traditional "test_expect_what [PREREQ] BODY"
+ if test -z "$test_expect_new_style"
+ then
+ test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
+ fi
+
+ if test $# != 2
+ then
+ if test -z "$test_expect_new_style"
+ then
+ error "bug in the test script: not 2 or 3 parameters to $whoami"
+ else
+ error "bug in the test script: not 2 parameters to $whoami"
+ fi
+ fi
+ test_label=$1 test_body=$2
+
+ export test_prereq
+}
+
test_expect_failure () {
test_start_
- test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 2 ||
- error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
- export test_prereq
+ test_expect_parse test_expect_failure "$@"
if ! test_skip "$@"
then
- say >&3 "checking known breakage: $2"
- if test_run_ "$2" expecting_failure
+ say >&3 "checking known breakage: $test_body"
+ if test_run_ "$test_body" expecting_failure
then
- test_known_broken_ok_ "$1"
+ test_known_broken_ok_ "$test_label"
else
- test_known_broken_failure_ "$1"
+ test_known_broken_failure_ "$test_label"
fi
fi
test_finish_
@@ -363,16 +408,13 @@ test_expect_failure () {
test_expect_success () {
test_start_
- test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 2 ||
- error "bug in the test script: not 2 or 3 parameters to test-expect-success"
- export test_prereq
+ test_expect_parse test_expect_success "$@"
if ! test_skip "$@"
then
- say >&3 "expecting success: $2"
- if test_run_ "$2"
+ say >&3 "expecting success: $test_body"
+ if test_run_ "$test_body"
then
- test_ok_ "$1"
+ test_ok_ "$test_label"
else
test_failure_ "$@"
fi
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index f4eecaa..6477d38 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -151,23 +151,20 @@ exit $ret' >&3 2>&4
test_perf () {
test_start_
- test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
- test "$#" = 2 ||
- error "bug in the test script: not 2 or 3 parameters to test-expect-success"
- export test_prereq
+ test_expect_parse test_perf "$@"
if ! test_skip "$@"
then
base=$(basename "$0" .sh)
echo "$test_count" >>"$perf_results_dir"/$base.subtests
- echo "$1" >"$perf_results_dir"/$base.$test_count.descr
+ echo "$test_label" >"$perf_results_dir"/$base.$test_count.descr
if test -z "$verbose"; then
- printf "%s" "perf $test_count - $1:"
+ printf "%s" "perf $test_count - $test_label:"
else
- echo "perf $test_count - $1:"
+ echo "perf $test_count - $test_label:"
fi
for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
- say >&3 "running: $2"
- if test_run_perf_ "$2"
+ say >&3 "running: $test_body"
+ if test_run_perf_ "$test_body"
then
if test -z "$verbose"; then
printf " %s" "$i"
@@ -183,7 +180,7 @@ test_perf () {
if test -z "$verbose"; then
echo " ok"
else
- test_ok_ "$1"
+ test_ok_ "$test_label"
fi
base="$perf_results_dir"/"$perf_results_prefix$(basename "$0" .sh)"."$test_count"
"$TEST_DIRECTORY"/perf/min_time.perl test_time.* >"$base".times
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 96f40fe..795de61 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -106,7 +106,7 @@ test_expect_success_multi () {
expect_verbose=$( echo "$expect_all" | grep -v '^:: ' )
expect=$( echo "$expect_verbose" | sed -e 's/.* //' )
- test_expect_success $prereq "$testname" '
+ test_expect_success ${prereq:+--prereq $prereq} -- "$testname" '
expect "$expect" &&
eval "$code"
'
@@ -116,10 +116,11 @@ test_expect_success_multi () {
then
for quiet_opt in '-q' '--quiet'
do
- test_expect_success $prereq "$testname${quiet_opt:+ with $quiet_opt}" "
- expect '' &&
- $code
- "
+ test_expect_success ${prereq:+--prereq $prereq} -- \
+ "$testname${quiet_opt:+ with $quiet_opt}" '
+ expect "" &&
+ eval "$code"
+ '
done
quiet_opt=
fi
@@ -140,7 +141,9 @@ test_expect_success_multi () {
$code
"
opts="$verbose_opt$non_matching_opt"
- test_expect_success $prereq "$testname${opts:+ with $opts}" "$test_code"
+
+ test_expect_success ${prereq:+--prereq $prereq} -- \
+ "$testname${opts:+ with $opts}" "$test_code"
done
done
verbose_opt=
@@ -225,7 +228,7 @@ test_expect_success '-q with multiple args' '
stderr_contains "fatal: --quiet is only valid with a single pathname"
'
-test_expect_success '--quiet with multiple args' '
+test_expect_success -- '--quiet with multiple args' '
expect "" &&
test_check_ignore "--quiet one two" 128 &&
stderr_contains "fatal: --quiet is only valid with a single pathname"
@@ -235,7 +238,7 @@ for verbose_opt in '-v' '--verbose'
do
for quiet_opt in '-q' '--quiet'
do
- test_expect_success "$quiet_opt $verbose_opt" "
+ test_expect_success -- "$quiet_opt $verbose_opt" "
expect '' &&
test_check_ignore '$quiet_opt $verbose_opt foo' 128 &&
stderr_contains 'fatal: cannot have both --quiet and --verbose'
@@ -243,7 +246,7 @@ do
done
done
-test_expect_success '--quiet with multiple args' '
+test_expect_success -- '--quiet with multiple args' '
expect "" &&
test_check_ignore "--quiet one two" 128 &&
stderr_contains "fatal: --quiet is only valid with a single pathname"
@@ -559,34 +562,34 @@ sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
sed -e 's/ "/ /' -e 's/\\//' -e 's/"$//' expected-verbose | \
tr ":\t\n" "\0" >expected-verbose0
-test_expect_success '--stdin' '
+test_expect_success -- '--stdin' '
expect_from_stdin <expected-default &&
test_check_ignore "--stdin" <stdin
'
-test_expect_success '--stdin -q' '
+test_expect_success -- '--stdin -q' '
expect "" &&
test_check_ignore "-q --stdin" <stdin
'
-test_expect_success '--stdin -v' '
+test_expect_success -- '--stdin -v' '
expect_from_stdin <expected-verbose &&
test_check_ignore "-v --stdin" <stdin
'
for opts in '--stdin -z' '-z --stdin'
do
- test_expect_success "$opts" "
+ test_expect_success -- "$opts" "
expect_from_stdin <expected-default0 &&
test_check_ignore '$opts' <stdin0
"
- test_expect_success "$opts -q" "
+ test_expect_success -- "$opts -q" "
expect "" &&
test_check_ignore '-q $opts' <stdin0
"
- test_expect_success "$opts -v" "
+ test_expect_success -- "$opts -v" "
expect_from_stdin <expected-verbose0 &&
test_check_ignore '-v $opts' <stdin0
"
@@ -645,7 +648,7 @@ sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
sed -e 's/ "/ /' -e 's/\\//' -e 's/"$//' expected-verbose | \
tr ":\t\n" "\0" >expected-verbose0
-test_expect_success '--stdin from subdirectory' '
+test_expect_success -- '--stdin from subdirectory' '
expect_from_stdin <expected-default &&
(
cd a &&
@@ -653,7 +656,7 @@ test_expect_success '--stdin from subdirectory' '
)
'
-test_expect_success '--stdin from subdirectory with -v' '
+test_expect_success -- '--stdin from subdirectory with -v' '
expect_from_stdin <expected-verbose &&
(
cd a &&
@@ -661,7 +664,7 @@ test_expect_success '--stdin from subdirectory with -v' '
)
'
-test_expect_success '--stdin from subdirectory with -v -n' '
+test_expect_success -- '--stdin from subdirectory with -v -n' '
expect_from_stdin <expected-all &&
(
cd a &&
@@ -671,7 +674,7 @@ test_expect_success '--stdin from subdirectory with -v -n' '
for opts in '--stdin -z' '-z --stdin'
do
- test_expect_success "$opts from subdirectory" '
+ test_expect_success -- "$opts from subdirectory" '
expect_from_stdin <expected-default0 &&
(
cd a &&
@@ -679,7 +682,7 @@ do
)
'
- test_expect_success "$opts from subdirectory with -v" '
+ test_expect_success -- "$opts from subdirectory with -v" '
expect_from_stdin <expected-verbose0 &&
(
cd a &&
next prev parent reply other threads:[~2013-09-19 19:53 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-17 12:10 [PATCH 1/2] perf-lib: split starting the test from the execution Thomas Gummerer
2013-09-17 12:10 ` [PATCH 2/2] perf-lib: add test_perf_cleanup target Thomas Gummerer
2013-09-17 17:43 ` Junio C Hamano
2013-09-19 11:42 ` Thomas Gummerer
2013-09-19 17:19 ` Junio C Hamano
2013-09-19 19:52 ` Junio C Hamano [this message]
2013-09-19 20:11 ` Junio C Hamano
2013-09-20 22:14 ` Thomas Gummerer
2013-09-23 21:08 ` [PATCH v2 0/3] Add cleanup action to perf-lib Thomas Gummerer
2013-09-23 21:08 ` [PATCH v2 1/3] test-lib: introduce "modern" style tests Thomas Gummerer
2013-09-23 21:08 ` [PATCH v2 2/3] perf-lib: add cleanup option Thomas Gummerer
2013-09-23 21:08 ` [PATCH v2 3/3] p0003-index.sh: add perf test for the index formats Thomas Gummerer
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqtxhgsi5p.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=t.gummerer@gmail.com \
--cc=trast@inf.ethz.ch \
/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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.