{"thread":{"id":"28275","subject":"[PATCH] test-lib: save test counts across invocations","startedAt":"2011-09-01T13:08:45Z","lastAt":"2011-09-02T12:39:39Z","messageCount":6,"participants":["Thomas Rast","Junio C Hamano","Jeff King","Alex Vandiver"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174666","messageId":"8fe5381a6b69079b8c20452fd4d99a128764dd52.1314882443.git.trast@student.ethz.ch","threadId":"28275","inReplyTo":null,"subject":"[PATCH] test-lib: save test counts across invocations","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-01T13:08:45Z","receivedAt":"2011-09-01T13:08:45Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Under 'prove' we can get progress status for tests running in\nparallel.  However we never told it the total number of tests in a\nfile in advance, and thus the progress only showed the tests already\nexecuted.\n\nSave the number of tests run ($test_count) in a file under\ntest-counts/.  Then when sourcing test-lib.sh the next time, compare\nthe timestamps.  If the counts file is older than the test, discard.\nOtherwise use the count that we saved and give prove the test plan\n(\"1..N\") up front.\n\nThis results in 'make prove' giving progress output like\n\n  ===(    2884;121   8/12   24/147  1/8  1/3  )============================\n\nif you have already run the tests before.\n\nPrerequisite changes can mean that a whole test group is skipped\n(e.g. NO_SVN_TESTS=1).  We thus need to be somewhat careful to only\nemit the \"full\" plan once we know we're not going to $skip_all.\n\nt9700 needs special treatment on top of $test_external_has_tap because\nthe latter can only be set once we know that the external test will\nrun.  If a prerequisite fails, we still need to emit the plan.\n\nThe Makefile changes are required because we want to keep that\ntest-count subdirectory unless the *user* invokes 'make clean', but we\npreviously ran the latter ourselves after every successful test run.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nSparked by a discussion on G+.  I think this is the \"simple\" approach.\nThe \"cute\" approach would be to let test-lib.sh define test_* as\ntest-counting dummies once, source the test script itself (avoiding\nthe sourcing loop with test-lib) to count what it does, then do the\nreal work.\n\n\n t/.gitignore        |    1 +\n t/Makefile          |    9 ++++++---\n t/t9700-perl-git.sh |    1 +\n t/test-lib.sh       |   27 +++++++++++++++++++++++++--\n 4 files changed, 33 insertions(+), 5 deletions(-)\n\ndiff --git a/t/.gitignore b/t/.gitignore\nindex 4e731dc..7de845f 100644\n--- a/t/.gitignore\n+++ b/t/.gitignore\n@@ -1,3 +1,4 @@\n /trash directory*\n /test-results\n+/test-counts\n /.prove\ndiff --git a/t/Makefile b/t/Makefile\nindex 9046ec9..c70de07 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -36,11 +36,14 @@ $(T):\n pre-clean:\n \t$(RM) -r test-results\n \n-clean:\n+post-clean:\n \t$(RM) -r 'trash directory'.* test-results\n \t$(RM) -r valgrind/bin\n \t$(RM) .prove\n \n+clean: post-clean\n+\t$(RM) -r test-counts\n+\n test-lint: test-lint-duplicates test-lint-executable\n \n test-lint-duplicates:\n@@ -55,7 +58,7 @@ test-lint-executable:\n \n aggregate-results-and-cleanup: $(T)\n \t$(MAKE) aggregate-results\n-\t$(MAKE) clean\n+\t$(MAKE) post-clean\n \n aggregate-results:\n \tfor f in test-results/t*-*.counts; do \\\n@@ -111,4 +114,4 @@ smoke_report: smoke\n \t\thttp://smoke.git.nix.is/app/projects/process_add_report/1 \\\n \t| grep -v ^Redirecting\n \n-.PHONY: pre-clean $(T) aggregate-results clean valgrind smoke smoke_report\n+.PHONY: pre-clean $(T) aggregate-results clean valgrind smoke smoke_report post-clean\ndiff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\nindex 3787186..20ec097 100755\n--- a/t/t9700-perl-git.sh\n+++ b/t/t9700-perl-git.sh\n@@ -4,6 +4,7 @@\n #\n \n test_description='perl interface (Git.pm)'\n+test_disable_saved_count=1\n . ./test-lib.sh\n \n if ! test_have_prereq PERL; then\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex bdd9513..374cdb2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -522,11 +522,19 @@ test_skip () {\n \tesac\n }\n \n+test_emit_plan () {\n+\tif [ -z \"$test_plan_emitted\" -a -n \"$test_count_saved\" ]; then\n+\t\tsay \"1..$test_count_saved\"\n+\t\ttest_plan_emitted=y\n+\tfi\n+}\n+\n test_expect_failure () {\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_emit_plan\n \tif ! test_skip \"$@\"\n \tthen\n \t\tsay >&3 \"checking known breakage: $2\"\n@@ -545,6 +553,7 @@ test_expect_success () {\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_emit_plan\n \tif ! test_skip \"$@\"\n \tthen\n \t\tsay >&3 \"expecting success: $2\"\n@@ -860,12 +869,14 @@ test_done () {\n \tfi\n \tcase \"$test_failure\" in\n \t0)\n+\t\t[ -z \"$skip_all\" ] && echo \"$test_count\" > \"$test_count_file\"\n+\n \t\t# Maybe print SKIP message\n \t\t[ -z \"$skip_all\" ] || skip_all=\" # SKIP $skip_all\"\n \n \t\tif test $test_external_has_tap -eq 0; then\n \t\t\tsay_color pass \"# passed all $msg\"\n-\t\t\tsay \"1..$test_count$skip_all\"\n+\t\t\t[ -z \"$test_plan_emitted\" ] && say \"1..$test_count$skip_all\"\n \t\tfi\n \n \t\ttest -d \"$remove_trash\" &&\n@@ -877,7 +888,7 @@ test_done () {\n \t*)\n \t\tif test $test_external_has_tap -eq 0; then\n \t\t\tsay_color error \"# failed $test_failure among $msg\"\n-\t\t\tsay \"1..$test_count\"\n+\t\t\t[ -z \"$test_plan_emitted\" ] && say \"1..$test_count\"\n \t\tfi\n \n \t\texit 1 ;;\n@@ -896,6 +907,18 @@ then\n fi\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n \n+mkdir -p \"$TEST_DIRECTORY\"/test-counts\n+test_count_file=\"$TEST_DIRECTORY\"/test-counts/$(basename \"$0\" .sh)\n+test_count_saved=$(\n+\tif [ -n \"$test_disable_saved_count\" ]; then\n+\t\t:\n+\t# the saved count is only valid if the file is newer than the test\n+\telif [ -f \"$test_count_file\" -a \"$test_count_file\" -nt \"$0\" ]; then\n+\t\tcat \"$test_count_file\" 2>/dev/null\n+\tfi\n+\t# otherwise we leave the variable empty\n+)\n+\n if test -n \"$valgrind\"\n then\n \tmake_symlink () {\n-- \n1.7.7.rc0.420.g468b7\n"},{"id":"174674","messageId":"7v62lcxmsd.fsf@alter.siamese.dyndns.org","threadId":"28275","inReplyTo":"8fe5381a6b69079b8c20452fd4d99a128764dd52.1314882443.git.trast@student.ethz.ch","subject":"Re: [PATCH] test-lib: save test counts across invocations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-01T16:14:26Z","receivedAt":"2011-09-01T16:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> Save the number of tests run ($test_count) in a file under\n> test-counts/.  Then when sourcing test-lib.sh the next time, compare\n> the timestamps.\n\n... which is this logic ...\n\n> +test_count_file=\"$TEST_DIRECTORY\"/test-counts/$(basename \"$0\" .sh)\n> +test_count_saved=$(\n> +\tif [ -n \"$test_disable_saved_count\" ]; then\n> +\t\t:\n> +\t# the saved count is only valid if the file is newer than the test\n> +\telif [ -f \"$test_count_file\" -a \"$test_count_file\" -nt \"$0\" ]; then\n> +\t\tcat \"$test_count_file\" 2>/dev/null\n> +\tfi\n> +\t# otherwise we leave the variable empty\n> +)\n\nI think the patch is cute, but I however do not think this is sufficient\nto catch prerequisite changes, unfortunately. I'd rather leave the total\nunknown than giving incorrect numbers.\n"},{"id":"174676","messageId":"20110901163846.GD15018@sigill.intra.peff.net","threadId":"28275","inReplyTo":"8fe5381a6b69079b8c20452fd4d99a128764dd52.1314882443.git.trast@student.ethz.ch","subject":"Re: [PATCH] test-lib: save test counts across invocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-01T16:38:46Z","receivedAt":"2011-09-01T16:38:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2011 at 03:08:45PM +0200, Thomas Rast wrote:\n\n> Save the number of tests run ($test_count) in a file under\n> test-counts/.  Then when sourcing test-lib.sh the next time, compare\n> the timestamps.  If the counts file is older than the test, discard.\n> Otherwise use the count that we saved and give prove the test plan\n> (\"1..N\") up front.\n\nHmm. What happens when we're wrong? Does our eye-candy just print\nsomething non-sensical like \"13/12\", or does prove actually care that we\nrun the right number of tests?\n\n> Sparked by a discussion on G+.  I think this is the \"simple\" approach.\n> The \"cute\" approach would be to let test-lib.sh define test_* as\n> test-counting dummies once, source the test script itself (avoiding\n> the sourcing loop with test-lib) to count what it does, then do the\n> real work.\n\nI don't think the \"cute\" approach will ever be accurate. Deciding\nwhether to run later tests sometimes depends on the results of earlier\ntests, in at least two cases:\n\n  1. Some tests find out which capabilities the system has, and set\n     prerequisites. You need to actually run those tests, not make them\n     counting dummies.\n\n  2. Some tests create state that we then iterate on. For example, I\n     think the mailinfo tests do something like:\n\n        test_expect_success 'split' '\n                git mailsplit -o patches mbox\n        '\n        for i in patches/*; do\n          test_expect_success \"check patch $i\" '\n                  git mailinfo $i >output\n                  ...\n          '\n        done\n\n      You'd get an inaccurate count if you didn't actually run the\n      mailsplit command.\n\nAnyway, this whole thing is a cute idea, and I do love eye candy, but I\nwonder if it's worth the complexity. All this is telling us is how far\ninto each of the scripts it is. But we have literally hundreds of test\nscripts, all with varying numbers of tests of varying speeds, and you're\nprobably running 16 or more at one time. So it doesn't tell you what you\nreally want to know, which is: how soon will the test suite probably be\ndone running.\n\n-Peff\n"},{"id":"174697","messageId":"1314902334.5371.17.camel@umgah.localdomain","threadId":"28275","inReplyTo":"20110901163846.GD15018@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: save test counts across invocations","fromName":"Alex Vandiver","fromEmail":"alex@chmrr.net","sentAt":"2011-09-01T18:38:54Z","receivedAt":"2011-09-01T18:38:54Z","isPatch":true,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"On Thu, 2011-09-01 at 12:38 -0400, Jeff King wrote:\n> Hmm. What happens when we're wrong? Does our eye-candy just print\n> something non-sensical like \"13/12\", or does prove actually care that we\n> run the right number of tests?\n\nprove very much does care -- having a mismatch between the number of\ntests planned and the number of tests run is an error in the testfile,\nand is reported as such in big red text.  This is because stating how\nmany tests you plan to run gives prove a way (in addition to the exit\nstatus) to know if the test stopped prematurely, so all mismatches\nbetween plan and actual test counts are reported as testfile failures.\n\nAs far as I know prove doesn't have a way to print the estimated time\nremaining, though using the contents of the .prove file (if you ran\nprove --state=save) to guess it wouldn't be all that hard of a change.\n - Alex\n"},{"id":"174699","messageId":"20110901184554.GA18737@sigill.intra.peff.net","threadId":"28275","inReplyTo":"1314902334.5371.17.camel@umgah.localdomain","subject":"Re: [PATCH] test-lib: save test counts across invocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-01T18:45:54Z","receivedAt":"2011-09-01T18:45:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2011 at 02:38:54PM -0400, Alex Vandiver wrote:\n\n> On Thu, 2011-09-01 at 12:38 -0400, Jeff King wrote:\n> > Hmm. What happens when we're wrong? Does our eye-candy just print\n> > something non-sensical like \"13/12\", or does prove actually care that we\n> > run the right number of tests?\n> \n> prove very much does care -- having a mismatch between the number of\n> tests planned and the number of tests run is an error in the testfile,\n> and is reported as such in big red text.  This is because stating how\n> many tests you plan to run gives prove a way (in addition to the exit\n> status) to know if the test stopped prematurely, so all mismatches\n> between plan and actual test counts are reported as testfile failures.\n\nThanks. I suspected something like that, but was too lazy to look. :)\n\nGiven that our methods for automatically determining the number of tests\nare so flaky, and that prove will treat it so seriously, it doesn't seem\nworth pursuing to me.\n\nWe already handle the premature abort case by trapping exit from the\nshell before the script calls test_done. So I don't think that is a\nfeature of prove that we particularly care about.\n\n> As far as I know prove doesn't have a way to print the estimated time\n> remaining, though using the contents of the .prove file (if you ran\n> prove --state=save) to guess it wouldn't be all that hard of a change.\n\nThat would be a neat feature. In practice, I know about how many tests\nthere are total (~7500), and how long it takes to run on my system (~60\nseconds), so I can do the math myself. Still, a little more eye candy\ncouldn't hurt. ;)\n\nIf I underestand the code correctly, we could even write our own custom\n\"formatter\" for git and use it via \"prove --formatter\".\n\n-Peff\n"},{"id":"174744","messageId":"201109021439.39916.trast@student.ethz.ch","threadId":"28275","inReplyTo":"20110901163846.GD15018@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: save test counts across invocations","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-02T12:39:39Z","receivedAt":"2011-09-02T12:39:39Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> Anyway, this whole thing is a cute idea, and I do love eye candy, but I\n> wonder if it's worth the complexity. All this is telling us is how far\n> into each of the scripts it is. But we have literally hundreds of test\n> scripts, all with varying numbers of tests of varying speeds, and you're\n> probably running 16 or more at one time. So it doesn't tell you what you\n> really want to know, which is: how soon will the test suite probably be\n> done running.\n\nI guess you're right.  Let's drop this, then.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}