{"thread":{"id":"34946","subject":"[PATCH 1/2] perf-lib: split starting the test from the execution","startedAt":"2013-09-17T12:10:41Z","lastAt":"2013-09-23T21:08:46Z","messageCount":12,"participants":["Thomas Gummerer","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"227747","messageId":"1379419842-32627-1-git-send-email-t.gummerer@gmail.com","threadId":"34946","inReplyTo":null,"subject":"[PATCH 1/2] perf-lib: split starting the test from the execution","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-09-17T12:10:41Z","receivedAt":"2013-09-17T12:10:41Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Separate the execution part to make future changes to the tests simpler.\n\nSigned-off-by: Thomas Gummerer \n---\n t/perf/perf-lib.sh | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex c61d535..95e483c 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -148,13 +148,8 @@ exit $ret' >&3 2>&4\n \treturn \"$eval_ret\"\n }\n \n-\n-test_perf () {\n+perf_test_ () {\n \ttest_start_\n-\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n-\ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n-\texport test_prereq\n \tif ! test_skip \"$@\"\n \tthen\n \t\tbase=$(basename \"$0\" .sh)\n@@ -191,6 +186,14 @@ test_perf () {\n \ttest_finish_\n }\n \n+test_perf () {\n+\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n+\ttest \"$#\" = 2 ||\n+\terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n+\texport test_prereq\n+\tperf_test_ \"$1\" \"$2\"\n+}\n+\n # We extend test_done to print timings at the end (./run disables this\n # and does it after running everything)\n test_at_end_hook_ () {\n-- \n1.8.3.4.1238.ga800761\n"},{"id":"227748","messageId":"1379419842-32627-2-git-send-email-t.gummerer@gmail.com","threadId":"34946","inReplyTo":"1379419842-32627-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH 2/2] perf-lib: add test_perf_cleanup target","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-09-17T12:10:42Z","receivedAt":"2013-09-17T12:10:42Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently there is no way to clean up the changes that have been made\nwith test_perf for the next run. Add a way to reset the repository to\nthe state before the test for testing commands that modify the git\nrepository, e.g. for perf testing git add.\n\nSigned-off-by: Thomas Gummerer \n---\n\nThis enables me to do something like (hint, hint, hint ;-)):\n\nTest HEAD~1 HEAD \n------------------------------------------------------------------------------------\n....\n0003.16: v5 update-index file 0.19(0.12+0.06) 0.08(0.06+0.01) -57.9%\n\nThere are no performance tests currently using this, but since I have\nit anyway for a POC of partial writing of index-v5 (which is ugly and\nwill have to wait a bit until I'm ready to send it to the list) I think\nthis may be a worthwhile addition others can use in the meantime.\n\n t/perf/README | 11 +++++++++++\n t/perf/perf-lib.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/t/perf/README b/t/perf/README\nindex 8848c14..72f8a7b 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -123,6 +123,17 @@ tests, use\n \t\tcommand2\n \t'\n \n+For performance tests that need cleaning up after them that should not\n+be timed, use\n+\n+\ttest_perf_cleanup 'descriptive string' '\n+\t\tcommand1 &&\n+\t\tcommand2\n+\t' '\n+\t\tcleanupcommand1 &&\n+\t\tcleanupcommand2\n+\t'\n+\n test_perf spawns a subshell, for lack of better options. This means\n that\n \ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 95e483c..11a93f1 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -174,6 +174,22 @@ perf_test_ () {\n \t\t\t\ttest_failure_ \"$@\"\n \t\t\t\tbreak\n \t\t\tfi\n+\t\t\tsay >&3 \"cleaning up: $3\"\n+\t\t\tif test \"$#\" = 3\n+\t\t\tthen\n+\t\t\t\tif test_run_ \"$3\"\n+\t\t\t\tthen\n+\t\t\t\t\tif test -z \"$verbose\"; then\n+\t\t\t\t\t\techo -n \" c$i\"\n+\t\t\t\t\telse\n+\t\t\t\t\t\techo \"* cleaning up run $i/$GIT_PERF_REPEAT_COUNT:\"\n+\t\t\t\t\tfi\n+\t\t\t\telse\n+\t\t\t\t\ttest -z \"$verbose\" && echo\n+\t\t\t\t\ttest_failure_ \"$@\"\n+\t\t\t\t\tbreak\n+\t\t\t\tfi\n+\t\t\tfi\n \t\tdone\n \t\tif test -z \"$verbose\"; then\n \t\t\techo \" ok\"\n@@ -194,6 +210,15 @@ test_perf () {\n \tperf_test_ \"$1\" \"$2\"\n }\n \n+test_perf_cleanup () {\n+\ttest_start_\n+\ttest \"$#\" = 4 && { test_prereq=$1; shift; } || test_prereq=\n+\ttest \"$#\" = 3 ||\n+\terror \"bug in the test script: not 3 or 4 parameters to test-expect-success\"\n+\texport test_prereq\n+\tperf_test_ \"$1\" \"$2\" \"$3\"\n+}\n+\n # We extend test_done to print timings at the end (./run disables this\n # and does it after running everything)\n test_at_end_hook_ () {\n-- \n1.8.3.4.1238.ga800761\n"},{"id":"227766","messageId":"xmqq7gefz6m0.fsf@gitster.dls.corp.google.com","threadId":"34946","inReplyTo":"1379419842-32627-2-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH 2/2] perf-lib: add test_perf_cleanup target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-17T17:43:35Z","receivedAt":"2013-09-17T17:43:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> +For performance tests that need cleaning up after them that should not\n> +be timed, use\n> +\n> +\ttest_perf_cleanup 'descriptive string' '\n> +\t\tcommand1 &&\n> +\t\tcommand2\n> +\t' '\n> +\t\tcleanupcommand1 &&\n> +\t\tcleanupcommand2\n> +\t'\n> +\n\nHmm, if \"not timing the clean-up actions\" is the primary goal, is it\nan option to reuse test_when_finished for this (you may have to make\nsure that the commands run inside it are not timed; I didn't check).\n\nOne thing I felt uneasy about the above construct is that it makes\ncleanupcommand2 responsible for handling cases where not just\ncommand2 but also command1 might have failed.\n\nCompared to that, test-when-finished allows you to control what\nclean-up to do depending on what have already been done, i.e.\n\n test_when_finished 'undo what command1 would have done' &&\n\tcommand1 &&\n test_when_finished 'undo what command2 would have done' &&\n\tcommand2 &&\n ...\n\nThe second \"undo command2\" must be prepared for the case where\ncommand2 may have failed in the middle, but it can at least rely on\ncommand1 having succeeded when it is run.\n"},{"id":"227882","messageId":"87fvt1ghr4.fsf@gmail.com","threadId":"34946","inReplyTo":"xmqq7gefz6m0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] perf-lib: add test_perf_cleanup target","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-09-19T11:42:23Z","receivedAt":"2013-09-19T11:42:23Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Gummerer writes:\n>\n>> +For performance tests that need cleaning up after them that should not\n>> +be timed, use\n>> +\n>> +\ttest_perf_cleanup 'descriptive string' '\n>> +\t\tcommand1 &&\n>> +\t\tcommand2\n>> +\t' '\n>> +\t\tcleanupcommand1 &&\n>> +\t\tcleanupcommand2\n>> +\t'\n>> +\n>\n> Hmm, if \"not timing the clean-up actions\" is the primary goal, is it\n> an option to reuse test_when_finished for this (you may have to make\n> sure that the commands run inside it are not timed; I didn't check).\n\nMaybe I wasn't clear enough. The goal is to allow tests to have\neverything cleaned up after every run. This can then be used for\ncommands that change the index (or other things) and get back to the\noriginal state after that. For example something like this:\n\nfile=$(git ls-files | tail -n 30 | head -1)\n\ntest_expect_success \"change a file\" \"\n\techo 'something' >>$file\n\"\n\ntest_perf_cleanup \"v5 update-index file\" \"\n\tgit update-index $file\n\" \"\n\tgit reset\n\"\n\ntest_when_finished on the other hand only cleans up when the whole test\nis finished.\n\n> One thing I felt uneasy about the above construct is that it makes\n> cleanupcommand2 responsible for handling cases where not just\n> command2 but also command1 might have failed.\n>\n> Compared to that, test-when-finished allows you to control what\n> clean-up to do depending on what have already been done, i.e.\n>\n> test_when_finished 'undo what command1 would have done' &&\n> \tcommand1 &&\n> test_when_finished 'undo what command2 would have done' &&\n> \tcommand2 &&\n> ...\n>\n> The second \"undo command2\" must be prepared for the case where\n> command2 may have failed in the middle, but it can at least rely on\n> command1 having succeeded when it is run.\n\nWhen one performance test fails, the testing is aborted and the cleanup\ncommands are not executed anymore, leaving the trash directory in the\nfailed state. This consistent with the normal tests with the immediate\nflag passed to them. (All performance tests have the --immediate flag\non implicitly).\n"},{"id":"227890","messageId":"xmqqk3icu3u8.fsf@gitster.dls.corp.google.com","threadId":"34946","inReplyTo":"87fvt1ghr4.fsf@gmail.com","subject":"Re: [PATCH 2/2] perf-lib: add test_perf_cleanup target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-19T17:19:11Z","receivedAt":"2013-09-19T17:19:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> When one performance test fails, the testing is aborted and the cleanup\n> commands are not executed anymore, leaving the trash directory in the\n> failed state.\n\nAh, that I overlooked. In that case, the comments in my previous\nmessage do not apply.\n\nThanks.\n"},{"id":"227897","messageId":"xmqqtxhgsi5p.fsf@gitster.dls.corp.google.com","threadId":"34946","inReplyTo":"xmqqk3icu3u8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] perf-lib: add test_perf_cleanup target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-19T19:52:50Z","receivedAt":"2013-09-19T19:52:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Gummerer writes:\n>\n>> When one performance test fails, the testing is aborted and the cleanup\n>> commands are not executed anymore, leaving the trash directory in the\n>> failed state.\n>\n> Ah, that I overlooked. In that case, the comments in my previous\n> message do not apply.\n\nHaving said that, it was unclear to me why we would want a new\ntest_perf_cleanup added.\n\nThe name is misleading (does it define only clean-up actions?) to\nsay the least, and one way of fixing it would be to call it\ntest_perf_with_cleanup.\n\nI wondered why this clean-up section cannot be an optional parameter\nto test_perf, but that would not fly well because we won't know if\n3-arg form is with one prerequisite and no clean-up, or no prereq\nwith a clean-up, so perhaps adding a new function may be the best\nyou could do.\n\nBut in the longer term, I think we would be better off to allow two\nstyles (one traditional, another modern) and add new features only\nto the \"modern\" (aka \"more easily extensible\") form:\n\n\ttest_perf [PREREQ] BODY\n\ttest_perf [--prereq PREREQ] [--cleanup CLEANUP] ... BODY\n\nperhaps like this (this is without --cleanup but it should be\nobvious how to add a support for it to the command line parser).\n\nThe patch to t0008 is primarily to adjust for test labels that begin\nwith double-dashes that will be mistaken as a new-style option, but\nit is unnecessarily big because it uses random custom shell functions\nto hide the real calls to underlying test_expect_success X-<.\n\n\n t/test-lib-functions.sh | 72 ++++++++++++++++++++++++++++++++++++++-----------\n t/perf/perf-lib.sh | 17 +++++-------\n t/t0008-ignores.sh | 43 +++++++++++++++--------------\n 3 files changed, 87 insertions(+), 45 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex a7e9aac..10202dc 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -342,20 +342,65 @@ test_declared_prereq () {\n \treturn 1\n }\n \n+test_expect_parse () {\n+\twhoami=$1\n+\tshift\n+\ttest_expect_new_style=\n+\twhile case $# in 0) false ;; esac\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t--prereq)\n+\t\t\ttest $# -gt 1 ||\n+\t\t\terror \"bug in the test script: --prereq needs a parameter\"\n+\t\t\ttest_prereq=$2\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--)\n+\t\t\tshift\n+\t\t\tbreak\n+\t\t\t;;\n+\t\t--*)\n+\t\t\terror \"bug in the test script: unknown option '$1'\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\ttest_expect_new_style=yes\n+\t\tshift\n+\tdone\n+\n+\t# Traditional \"test_expect_what [PREREQ] BODY\"\n+\tif test -z \"$test_expect_new_style\"\n+\tthen\n+\t\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n+\tfi\n+\n+\tif test $# != 2\n+\tthen\n+\t\tif test -z \"$test_expect_new_style\"\n+\t\tthen\n+\t\t\terror \"bug in the test script: not 2 or 3 parameters to $whoami\"\n+\t\telse\n+\t\t\terror \"bug in the test script: not 2 parameters to $whoami\"\n+\t\tfi\n+\tfi\n+\ttest_label=$1 test_body=$2\n+\n+\texport test_prereq\n+}\n+\n test_expect_failure () {\n \ttest_start_\n-\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n-\ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 or 3 parameters to test-expect-failure\"\n-\texport test_prereq\n+\ttest_expect_parse test_expect_failure \"$@\"\n \tif ! test_skip \"$@\"\n \tthen\n-\t\tsay >&3 \"checking known breakage: $2\"\n-\t\tif test_run_ \"$2\" expecting_failure\n+\t\tsay >&3 \"checking known breakage: $test_body\"\n+\t\tif test_run_ \"$test_body\" expecting_failure\n \t\tthen\n-\t\t\ttest_known_broken_ok_ \"$1\"\n+\t\t\ttest_known_broken_ok_ \"$test_label\"\n \t\telse\n-\t\t\ttest_known_broken_failure_ \"$1\"\n+\t\t\ttest_known_broken_failure_ \"$test_label\"\n \t\tfi\n \tfi\n \ttest_finish_\n@@ -363,16 +408,13 @@ test_expect_failure () {\n \n test_expect_success () {\n \ttest_start_\n-\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n-\ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n-\texport test_prereq\n+\ttest_expect_parse test_expect_success \"$@\"\n \tif ! test_skip \"$@\"\n \tthen\n-\t\tsay >&3 \"expecting success: $2\"\n-\t\tif test_run_ \"$2\"\n+\t\tsay >&3 \"expecting success: $test_body\"\n+\t\tif test_run_ \"$test_body\"\n \t\tthen\n-\t\t\ttest_ok_ \"$1\"\n+\t\t\ttest_ok_ \"$test_label\"\n \t\telse\n \t\t\ttest_failure_ \"$@\"\n \t\tfi\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex f4eecaa..6477d38 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -151,23 +151,20 @@ exit $ret' >&3 2>&4\n \n test_perf () {\n \ttest_start_\n-\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n-\ttest \"$#\" = 2 ||\n-\terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n-\texport test_prereq\n+\ttest_expect_parse test_perf \"$@\"\n \tif ! test_skip \"$@\"\n \tthen\n \t\tbase=$(basename \"$0\" .sh)\n \t\techo \"$test_count\" >>\"$perf_results_dir\"/$base.subtests\n-\t\techo \"$1\" >\"$perf_results_dir\"/$base.$test_count.descr\n+\t\techo \"$test_label\" >\"$perf_results_dir\"/$base.$test_count.descr\n \t\tif test -z \"$verbose\"; then\n-\t\t\tprintf \"%s\" \"perf $test_count - $1:\"\n+\t\t\tprintf \"%s\" \"perf $test_count - $test_label:\"\n \t\telse\n-\t\t\techo \"perf $test_count - $1:\"\n+\t\t\techo \"perf $test_count - $test_label:\"\n \t\tfi\n \t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n-\t\t\tsay >&3 \"running: $2\"\n-\t\t\tif test_run_perf_ \"$2\"\n+\t\t\tsay >&3 \"running: $test_body\"\n+\t\t\tif test_run_perf_ \"$test_body\"\n \t\t\tthen\n \t\t\t\tif test -z \"$verbose\"; then\n \t\t\t\t\tprintf \" %s\" \"$i\"\n@@ -183,7 +180,7 @@ test_perf () {\n \t\tif test -z \"$verbose\"; then\n \t\t\techo \" ok\"\n \t\telse\n-\t\t\ttest_ok_ \"$1\"\n+\t\t\ttest_ok_ \"$test_label\"\n \t\tfi\n \t\tbase=\"$perf_results_dir\"/\"$perf_results_prefix$(basename \"$0\" .sh)\".\"$test_count\"\n \t\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".times\n\n\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 96f40fe..795de61 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -106,7 +106,7 @@ test_expect_success_multi () {\n \texpect_verbose=$( echo \"$expect_all\" | grep -v '^::\t' )\n \texpect=$( echo \"$expect_verbose\" | sed -e 's/.*\t//' )\n \n-\ttest_expect_success $prereq \"$testname\" '\n+\ttest_expect_success ${prereq:+--prereq $prereq} -- \"$testname\" '\n \t\texpect \"$expect\" &&\n \t\teval \"$code\"\n \t'\n@@ -116,10 +116,11 @@ test_expect_success_multi () {\n \tthen\n \t\tfor quiet_opt in '-q' '--quiet'\n \t\tdo\n-\t\t\ttest_expect_success $prereq \"$testname${quiet_opt:+ with $quiet_opt}\" \"\n-\t\t\texpect '' &&\n-\t\t\t$code\n-\t\t\"\n+\t\t\ttest_expect_success ${prereq:+--prereq $prereq} -- \\\n+\t\t\t\t\"$testname${quiet_opt:+ with $quiet_opt}\" '\n+\t\t\t\texpect \"\" &&\n+\t\t\t\teval \"$code\"\n+\t\t\t'\n \t\tdone\n \t\tquiet_opt=\n \tfi\n@@ -140,7 +141,9 @@ test_expect_success_multi () {\n \t\t\t\t$code\n \t\t\t\"\n \t\t\topts=\"$verbose_opt$non_matching_opt\"\n-\t\t\ttest_expect_success $prereq \"$testname${opts:+ with $opts}\" \"$test_code\"\n+\n+\t\t\ttest_expect_success ${prereq:+--prereq $prereq} -- \\\n+\t\t\t\t\"$testname${opts:+ with $opts}\" \"$test_code\"\n \t\tdone\n \tdone\n \tverbose_opt=\n@@ -225,7 +228,7 @@ test_expect_success '-q with multiple args' '\n \tstderr_contains \"fatal: --quiet is only valid with a single pathname\"\n '\n \n-test_expect_success '--quiet with multiple args' '\n+test_expect_success -- '--quiet with multiple args' '\n \texpect \"\" &&\n \ttest_check_ignore \"--quiet one two\" 128 &&\n \tstderr_contains \"fatal: --quiet is only valid with a single pathname\"\n@@ -235,7 +238,7 @@ for verbose_opt in '-v' '--verbose'\n do\n \tfor quiet_opt in '-q' '--quiet'\n \tdo\n-\t\ttest_expect_success \"$quiet_opt $verbose_opt\" \"\n+\t\ttest_expect_success -- \"$quiet_opt $verbose_opt\" \"\n \t\t\texpect '' &&\n \t\t\ttest_check_ignore '$quiet_opt $verbose_opt foo' 128 &&\n \t\t\tstderr_contains 'fatal: cannot have both --quiet and --verbose'\n@@ -243,7 +246,7 @@ do\n \tdone\n done\n \n-test_expect_success '--quiet with multiple args' '\n+test_expect_success -- '--quiet with multiple args' '\n \texpect \"\" &&\n \ttest_check_ignore \"--quiet one two\" 128 &&\n \tstderr_contains \"fatal: --quiet is only valid with a single pathname\"\n@@ -559,34 +562,34 @@ sed -e 's/^\"//' -e 's/\\\\//' -e 's/\"$//' expected-default | \\\n sed -e 's/\t\"/\t/' -e 's/\\\\//' -e 's/\"$//' expected-verbose | \\\n \ttr \":\\t\\n\" \"\\0\" >expected-verbose0\n \n-test_expect_success '--stdin' '\n+test_expect_success -- '--stdin' '\n \texpect_from_stdin expected-verbose0\n \n-test_expect_success '--stdin from subdirectory' '\n+test_expect_success -- '--stdin from subdirectory' '\n \texpect_from_stdin writes:\n\n> I wondered why this clean-up section cannot be an optional parameter\n> to test_perf, but that would not fly well because we won't know if\n> 3-arg form is with one prerequisite and no clean-up, or no prereq\n> with a clean-up, so perhaps adding a new function may be the best\n> you could do.\n>\n> But in the longer term, I think we would be better off ...\n\nIn any case, will queue your version as-is, at least for now.\n\nThanks.\n"},{"id":"227952","messageId":"87d2o3gmyl.fsf@gmail.com","threadId":"34946","inReplyTo":"xmqqtxhgsi5p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] perf-lib: add test_perf_cleanup target","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-09-20T22:14:26Z","receivedAt":"2013-09-20T22:14:26Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Thomas Gummerer writes:\n>>\n>>> When one performance test fails, the testing is aborted and the cleanup\n>>> commands are not executed anymore, leaving the trash directory in the\n>>> failed state.\n>>\n>> Ah, that I overlooked. In that case, the comments in my previous\n>> message do not apply.\n>\n> Having said that, it was unclear to me why we would want a new\n> test_perf_cleanup added.\n>\n> The name is misleading (does it define only clean-up actions?) to\n> say the least, and one way of fixing it would be to call it\n> test_perf_with_cleanup.\n\nIt defines any actions you want to happen after each round of tests that\nshould not be timed. Usually that would mean any action that resets a\nmodified repository to its original state. I'm not really sure about\nthe name, but test_perf_with_cleanup seems good to me.\n\n> I wondered why this clean-up section cannot be an optional parameter\n> to test_perf, but that would not fly well because we won't know if\n> 3-arg form is with one prerequisite and no clean-up, or no prereq\n> with a clean-up, so perhaps adding a new function may be the best\n> you could do.\n\nYeah, that was my first thought too, but as you explained that's not\npossible. Just thinking out loud, we could drop the prerequisite check\nfor performance tests, as no performance tests currently use it. Future\nperformance tests may use it though and it would be inconsistent with\nthe rest of the test-suite.\n\n> But in the longer term, I think we would be better off to allow two\n> styles (one traditional, another modern) and add new features only\n> to the \"modern\" (aka \"more easily extensible\") form:\n>\n> \ttest_perf [PREREQ] BODY\n> \ttest_perf [--prereq PREREQ] [--cleanup CLEANUP] ... BODY\n>\n> perhaps like this (this is without --cleanup but it should be\n> obvious how to add a support for it to the command line parser).\n\nI think this makes most sense. The cleanup part would only be needed\nfor the perf tests, but the flexibility of this wouldn't hurt. The\ncleanup would then look something like this:\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex b2a277b..4dfdd28 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -176,6 +176,21 @@ test_perf () {\n test_failure_ \"$@\"\n break\n fi\n+ if ! test -z \"$cleanup_action\"; then\n+ say >&3 \"cleaning up: $cleanup_action\"\n+ if test_run_ \"$cleanup_action\"\n+ then\n+ if test -z \"$verbose\"; then\n+ echo -n \" c$i\"\n+ else\n+ echo \"* cleaning up run $i/$GIT_PERF_REPEAT_COUNT:\"\n+ fi\n+ else\n+ test -z $verbose && echo\n+ test_failure_ \"$@\"\n+ break\n+ fi\n+ fi\n done\n if test -z \"$verbose\"; then\n echo \" ok\"\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 473b21d..4bad14f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -360,6 +360,12 @@ test_expect_parse () {\n test_prereq=$2\n shift\n ;;\n+ --cleanup)\n+ test $# -gt 1 ||\n+ error \"bug in the test script: --cleanup needs a parameter\"\n+ cleanup_action=$2\n+ shift\n+ ;;\n --)\n shift\n break\n\nI'll try to send an updated patch series, probably over the weekend with\nthis suggestions included (It's time to go to bed now).\n\n> The patch to t0008 is primarily to adjust for test labels that begin\n> with double-dashes that will be mistaken as a new-style option, but\n> it is unnecessarily big because it uses random custom shell functions\n> to hide the real calls to underlying test_expect_success X-<.\n>\n>\n> t/test-lib-functions.sh | 72 ++++++++++++++++++++++++++++++++++++++-----------\n> t/perf/perf-lib.sh | 17 +++++-------\n> t/t0008-ignores.sh | 43 +++++++++++++++--------------\n> 3 files changed, 87 insertions(+), 45 deletions(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index a7e9aac..10202dc 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -342,20 +342,65 @@ test_declared_prereq () {\n> \treturn 1\n> }\n>\n> +test_expect_parse () {\n> +\twhoami=$1\n> +\tshift\n> +\ttest_expect_new_style=\n> +\twhile case $# in 0) false ;; esac\n> +\tdo\n> +\t\tcase \"$1\" in\n> +\t\t--prereq)\n> +\t\t\ttest $# -gt 1 ||\n> +\t\t\terror \"bug in the test script: --prereq needs a parameter\"\n> +\t\t\ttest_prereq=$2\n> +\t\t\tshift\n> +\t\t\t;;\n> +\t\t--)\n> +\t\t\tshift\n> +\t\t\tbreak\n> +\t\t\t;;\n> +\t\t--*)\n> +\t\t\terror \"bug in the test script: unknown option '$1'\"\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tbreak\n> +\t\t\t;;\n> +\t\tesac\n> +\t\ttest_expect_new_style=yes\n> +\t\tshift\n> +\tdone\n> +\n> +\t# Traditional \"test_expect_what [PREREQ] BODY\"\n> +\tif test -z \"$test_expect_new_style\"\n> +\tthen\n> +\t\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n> +\tfi\n> +\n> +\tif test $# != 2\n> +\tthen\n> +\t\tif test -z \"$test_expect_new_style\"\n> +\t\tthen\n> +\t\t\terror \"bug in the test script: not 2 or 3 parameters to $whoami\"\n> +\t\telse\n> +\t\t\terror \"bug in the test script: not 2 parameters to $whoami\"\n> +\t\tfi\n> +\tfi\n> +\ttest_label=$1 test_body=$2\n> +\n> +\texport test_prereq\n> +}\n> +\n> test_expect_failure () {\n> \ttest_start_\n> -\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n> -\ttest \"$#\" = 2 ||\n> -\terror \"bug in the test script: not 2 or 3 parameters to test-expect-failure\"\n> -\texport test_prereq\n> +\ttest_expect_parse test_expect_failure \"$@\"\n> \tif ! test_skip \"$@\"\n> \tthen\n> -\t\tsay >&3 \"checking known breakage: $2\"\n> -\t\tif test_run_ \"$2\" expecting_failure\n> +\t\tsay >&3 \"checking known breakage: $test_body\"\n> +\t\tif test_run_ \"$test_body\" expecting_failure\n> \t\tthen\n> -\t\t\ttest_known_broken_ok_ \"$1\"\n> +\t\t\ttest_known_broken_ok_ \"$test_label\"\n> \t\telse\n> -\t\t\ttest_known_broken_failure_ \"$1\"\n> +\t\t\ttest_known_broken_failure_ \"$test_label\"\n> \t\tfi\n> \tfi\n> \ttest_finish_\n> @@ -363,16 +408,13 @@ test_expect_failure () {\n>\n> test_expect_success () {\n> \ttest_start_\n> -\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n> -\ttest \"$#\" = 2 ||\n> -\terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n> -\texport test_prereq\n> +\ttest_expect_parse test_expect_success \"$@\"\n> \tif ! test_skip \"$@\"\n> \tthen\n> -\t\tsay >&3 \"expecting success: $2\"\n> -\t\tif test_run_ \"$2\"\n> +\t\tsay >&3 \"expecting success: $test_body\"\n> +\t\tif test_run_ \"$test_body\"\n> \t\tthen\n> -\t\t\ttest_ok_ \"$1\"\n> +\t\t\ttest_ok_ \"$test_label\"\n> \t\telse\n> \t\t\ttest_failure_ \"$@\"\n> \t\tfi\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index f4eecaa..6477d38 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -151,23 +151,20 @@ exit $ret' >&3 2>&4\n>\n> test_perf () {\n> \ttest_start_\n> -\ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n> -\ttest \"$#\" = 2 ||\n> -\terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n> -\texport test_prereq\n> +\ttest_expect_parse test_perf \"$@\"\n> \tif ! test_skip \"$@\"\n> \tthen\n> \t\tbase=$(basename \"$0\" .sh)\n> \t\techo \"$test_count\" >>\"$perf_results_dir\"/$base.subtests\n> -\t\techo \"$1\" >\"$perf_results_dir\"/$base.$test_count.descr\n> +\t\techo \"$test_label\" >\"$perf_results_dir\"/$base.$test_count.descr\n> \t\tif test -z \"$verbose\"; then\n> -\t\t\tprintf \"%s\" \"perf $test_count - $1:\"\n> +\t\t\tprintf \"%s\" \"perf $test_count - $test_label:\"\n> \t\telse\n> -\t\t\techo \"perf $test_count - $1:\"\n> +\t\t\techo \"perf $test_count - $test_label:\"\n> \t\tfi\n> \t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n> -\t\t\tsay >&3 \"running: $2\"\n> -\t\t\tif test_run_perf_ \"$2\"\n> +\t\t\tsay >&3 \"running: $test_body\"\n> +\t\t\tif test_run_perf_ \"$test_body\"\n> \t\t\tthen\n> \t\t\t\tif test -z \"$verbose\"; then\n> \t\t\t\t\tprintf \" %s\" \"$i\"\n> @@ -183,7 +180,7 @@ test_perf () {\n> \t\tif test -z \"$verbose\"; then\n> \t\t\techo \" ok\"\n> \t\telse\n> -\t\t\ttest_ok_ \"$1\"\n> +\t\t\ttest_ok_ \"$test_label\"\n> \t\tfi\n> \t\tbase=\"$perf_results_dir\"/\"$perf_results_prefix$(basename \"$0\" .sh)\".\"$test_count\"\n> \t\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".times\n>\n>\n> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\n> index 96f40fe..795de61 100755\n> --- a/t/t0008-ignores.sh\n> +++ b/t/t0008-ignores.sh\n> @@ -106,7 +106,7 @@ test_expect_success_multi () {\n> \texpect_verbose=$( echo \"$expect_all\" | grep -v '^::\t' )\n> \texpect=$( echo \"$expect_verbose\" | sed -e 's/.*\t//' )\n>\n> -\ttest_expect_success $prereq \"$testname\" '\n> +\ttest_expect_success ${prereq:+--prereq $prereq} -- \"$testname\" '\n> \t\texpect \"$expect\" &&\n> \t\teval \"$code\"\n> \t'\n> @@ -116,10 +116,11 @@ test_expect_success_multi () {\n> \tthen\n> \t\tfor quiet_opt in '-q' '--quiet'\n> \t\tdo\n> -\t\t\ttest_expect_success $prereq \"$testname${quiet_opt:+ with $quiet_opt}\" \"\n> -\t\t\texpect '' &&\n> -\t\t\t$code\n> -\t\t\"\n> +\t\t\ttest_expect_success ${prereq:+--prereq $prereq} -- \\\n> +\t\t\t\t\"$testname${quiet_opt:+ with $quiet_opt}\" '\n> +\t\t\t\texpect \"\" &&\n> +\t\t\t\teval \"$code\"\n> +\t\t\t'\n> \t\tdone\n> \t\tquiet_opt=\n> \tfi\n> @@ -140,7 +141,9 @@ test_expect_success_multi () {\n> \t\t\t\t$code\n> \t\t\t\"\n> \t\t\topts=\"$verbose_opt$non_matching_opt\"\n> -\t\t\ttest_expect_success $prereq \"$testname${opts:+ with $opts}\" \"$test_code\"\n> +\n> +\t\t\ttest_expect_success ${prereq:+--prereq $prereq} -- \\\n> +\t\t\t\t\"$testname${opts:+ with $opts}\" \"$test_code\"\n> \t\tdone\n> \tdone\n> \tverbose_opt=\n> @@ -225,7 +228,7 @@ test_expect_success '-q with multiple args' '\n> \tstderr_contains \"fatal: --quiet is only valid with a single pathname\"\n> '\n>\n> -test_expect_success '--quiet with multiple args' '\n> +test_expect_success -- '--quiet with multiple args' '\n> \texpect \"\" &&\n> \ttest_check_ignore \"--quiet one two\" 128 &&\n> \tstderr_contains \"fatal: --quiet is only valid with a single pathname\"\n> @@ -235,7 +238,7 @@ for verbose_opt in '-v' '--verbose'\n> do\n> \tfor quiet_opt in '-q' '--quiet'\n> \tdo\n> -\t\ttest_expect_success \"$quiet_opt $verbose_opt\" \"\n> +\t\ttest_expect_success -- \"$quiet_opt $verbose_opt\" \"\n> \t\t\texpect '' &&\n> \t\t\ttest_check_ignore '$quiet_opt $verbose_opt foo' 128 &&\n> \t\t\tstderr_contains 'fatal: cannot have both --quiet and --verbose'\n> @@ -243,7 +246,7 @@ do\n> \tdone\n> done\n>\n> -test_expect_success '--quiet with multiple args' '\n> +test_expect_success -- '--quiet with multiple args' '\n> \texpect \"\" &&\n> \ttest_check_ignore \"--quiet one two\" 128 &&\n> \tstderr_contains \"fatal: --quiet is only valid with a single pathname\"\n> @@ -559,34 +562,34 @@ sed -e 's/^\"//' -e 's/\\\\//' -e 's/\"$//' expected-default | \\\n> sed -e 's/\t\"/\t/' -e 's/\\\\//' -e 's/\"$//' expected-verbose | \\\n> \ttr \":\\t\\n\" \"\\0\" >expected-verbose0\n>\n> -test_expect_success '--stdin' '\n> +test_expect_success -- '--stdin' '\n> \texpect_from_stdin \ttest_check_ignore \"--stdin\" '\n>\n> -test_expect_success '--stdin -q' '\n> +test_expect_success -- '--stdin -q' '\n> \texpect \"\" &&\n> \ttest_check_ignore \"-q --stdin\" '\n>\n> -test_expect_success '--stdin -v' '\n> +test_expect_success -- '--stdin -v' '\n> \texpect_from_stdin \ttest_check_ignore \"-v --stdin\" '\n>\n> for opts in '--stdin -z' '-z --stdin'\n> do\n> -\ttest_expect_success \"$opts\" \"\n> +\ttest_expect_success -- \"$opts\" \"\n> \t\texpect_from_stdin \t\ttest_check_ignore '$opts' \t\"\n>\n> -\ttest_expect_success \"$opts -q\" \"\n> +\ttest_expect_success -- \"$opts -q\" \"\n> \t\texpect \"\" &&\n> \t\ttest_check_ignore '-q $opts' \t\"\n>\n> -\ttest_expect_success \"$opts -v\" \"\n> +\ttest_expect_success -- \"$opts -v\" \"\n> \t\texpect_from_stdin \t\ttest_check_ignore '-v $opts' \t\"\n> @@ -645,7 +648,7 @@ sed -e 's/^\"//' -e 's/\\\\//' -e 's/\"$//' expected-default | \\\n> sed -e 's/\t\"/\t/' -e 's/\\\\//' -e 's/\"$//' expected-verbose | \\\n> \ttr \":\\t\\n\" \"\\0\" >expected-verbose0\n>\n> -test_expect_success '--stdin from subdirectory' '\n> +test_expect_success -- '--stdin from subdirectory' '\n> \texpect_from_stdin \t(\n> \t\tcd a &&\n> @@ -653,7 +656,7 @@ test_expect_success '--stdin from subdirectory' '\n> \t)\n> '\n>\n> -test_expect_success '--stdin from subdirectory with -v' '\n> +test_expect_success -- '--stdin from subdirectory with -v' '\n> \texpect_from_stdin \t(\n> \t\tcd a &&\n> @@ -661,7 +664,7 @@ test_expect_success '--stdin from subdirectory with -v' '\n> \t)\n> '\n>\n> -test_expect_success '--stdin from subdirectory with -v -n' '\n> +test_expect_success -- '--stdin from subdirectory with -v -n' '\n> \texpect_from_stdin \t(\n> \t\tcd a &&\n> @@ -671,7 +674,7 @@ test_expect_success '--stdin from subdirectory with -v -n' '\n>\n> for opts in '--stdin -z' '-z --stdin'\n> do\n> -\ttest_expect_success \"$opts from subdirectory\" '\n> +\ttest_expect_success -- \"$opts from subdirectory\" '\n> \t\texpect_from_stdin \t\t(\n> \t\t\tcd a &&\n> @@ -679,7 +682,7 @@ do\n> \t\t)\n> \t'\n>\n> -\ttest_expect_success \"$opts from subdirectory with -v\" '\n> +\ttest_expect_success -- \"$opts from subdirectory with -v\" '\n> \t\texpect_from_stdin \t\t(\n> \t\t\tcd a &&\n"},{"id":"228104","messageId":"1379970526-27997-1-git-send-email-t.gummerer@gmail.com","threadId":"34946","inReplyTo":"xmqqtxhgsi5p.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2 0/3] Add cleanup action to perf-lib","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-09-23T21:08:43Z","receivedAt":"2013-09-23T21:08:43Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"This patch series comes out of the discussion at $gmane/234874, adding\na new (modern) form of writing tests. This form allows easier\nextensibility of test cases. In the next patch a --cleanup option is\nadded for performance tests. The option does nothing for normal\ntests, as test_when_finished is the better option for them.\n\nThe last patch adds a few simple tests to show the capabilities of the\nnew --cleanup option.\n\nJunio C Hamano (1):\n test-lib: introduce \"modern\" style tests\n\nThomas Gummerer (1):\n perf-lib: add cleanup option\n\nThomas Rast (1):\n p0003-index.sh: add perf test for the index formats\n\n t/README | 24 +++++++++++++--\n t/perf/README | 21 ++++++++++++-\n t/perf/p0003-index.sh | 56 +++++++++++++++++++++++++++++++++++\n t/perf/perf-lib.sh | 32 +++++++++++++-------\n t/t0008-ignores.sh | 34 ++++++++++-----------\n t/test-lib-functions.sh | 78 +++++++++++++++++++++++++++++++++++++++----------\n 6 files changed, 200 insertions(+), 45 deletions(-)\n create mode 100755 t/perf/p0003-index.sh\n\n-- \n1.8.3.4.1241.g1ce9896\n"},{"id":"228105","messageId":"1379970526-27997-2-git-send-email-t.gummerer@gmail.com","threadId":"34946","inReplyTo":"1379970526-27997-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v2 1/3] test-lib: introduce \"modern\" style tests","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-09-23T21:08:44Z","receivedAt":"2013-09-23T21:08:44Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"From: Junio C Hamano \n\nAdd a new, extensible style for tests that allows the addition of new\nparameters other than the prerequitites\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Thomas Gummerer \n---\n t/README | 24 +++++++++++++++--\n t/perf/README | 12 ++++++++-\n t/perf/perf-lib.sh | 17 +++++-------\n t/t0008-ignores.sh | 34 +++++++++++------------\n t/test-lib-functions.sh | 72 ++++++++++++++++++++++++++++++++++++++-----------\n 5 files changed, 114 insertions(+), 45 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 2167125..bf41c29 100644\n--- a/t/README\n+++ b/t/README\n@@ -390,6 +390,12 @@ below), e.g.:\n \"$PERL_PATH\" -e \"hlagh() if unf_unf()\"\n '\n \n+or in the \"modern\" form, specifying the prerequisite as a parameter, e.g.:\n+\n+ test_expect_success --prereq PERL 'I need Perl' '\n+ \"$PERL_PATH\" -e \"hlagh() if unf_unf()\"\n+ '\n+\n The advantage of skipping tests like this is that platforms that don't\n have the PERL and other optional dependencies get an indication of how\n many tests they're missing.\n@@ -422,6 +428,7 @@ There are a handful helper functions defined in the test harness\n library for your script to use.\n \n - test_expect_success []