{"thread":{"id":"24327","subject":"[PATCH] tests: Use skip_all=* to skip tests","startedAt":"2010-07-08T01:16:06Z","lastAt":"2010-07-09T23:28:35Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Michael J Gruber","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"145074","messageId":"1278551766-27569-1-git-send-email-avarab@gmail.com","threadId":"24327","inReplyTo":null,"subject":"[PATCH] tests: Use skip_all=* to skip tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-08T01:16:06Z","receivedAt":"2010-07-08T01:16:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change tests to skip with skip_all=* + test_done instead of using say\n+ test_done.\n\nThis is a follow-up to \"tests: Skip tests in a way that makes sense\nunder TAP\" (fadb5156e4). I missed these cases when prepearing that\npatch, hopefully this is all of them.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/gitweb-lib.sh           |    4 ++--\n t/lib-cvs.sh              |    6 +++---\n t/lib-git-svn.sh          |   11 +++++------\n t/lib-httpd.sh            |    8 ++++----\n t/lib-patch-mode.sh       |    2 +-\n t/t5800-remote-helpers.sh |    2 +-\n t/t7005-editor.sh         |    2 +-\n 7 files changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh\nindex b70b891..81ef2a0 100644\n--- a/t/gitweb-lib.sh\n+++ b/t/gitweb-lib.sh\n@@ -76,12 +76,12 @@ gitweb_run () {\n . ./test-lib.sh\n \n if ! test_have_prereq PERL; then\n-\tsay 'skipping gitweb tests, perl not available'\n+\tskip_all='skipping gitweb tests, perl not available'\n \ttest_done\n fi\n \n perl -MEncode -e 'decode_utf8(\"\", Encode::FB_CROAK)' >/dev/null 2>&1 || {\n-    say 'skipping gitweb tests, perl version is too old'\n+    skip_all='skipping gitweb tests, perl version is too old'\n     test_done\n }\n \ndiff --git a/t/lib-cvs.sh b/t/lib-cvs.sh\nindex 4b3b793..648d161 100644\n--- a/t/lib-cvs.sh\n+++ b/t/lib-cvs.sh\n@@ -9,7 +9,7 @@ export HOME\n \n if ! type cvs >/dev/null 2>&1\n then\n-\tsay 'skipping cvsimport tests, cvs not found'\n+\tskip_all='skipping cvsimport tests, cvs not found'\n \ttest_done\n fi\n \n@@ -21,11 +21,11 @@ case \"$cvsps_version\" in\n 2.1 | 2.2*)\n \t;;\n '')\n-\tsay 'skipping cvsimport tests, cvsps not found'\n+\tskip_all='skipping cvsimport tests, cvsps not found'\n \ttest_done\n \t;;\n *)\n-\tsay 'skipping cvsimport tests, unsupported cvsps version'\n+\tskip_all='skipping cvsimport tests, unsupported cvsps version'\n \ttest_done\n \t;;\n esac\ndiff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\nindex 344785d..f62f617 100644\n--- a/t/lib-git-svn.sh\n+++ b/t/lib-git-svn.sh\n@@ -21,7 +21,7 @@ PERL=${PERL:-perl}\n svn >/dev/null 2>&1\n if test $? -ne 1\n then\n-    say 'skipping git svn tests, svn not found'\n+    skip_all='skipping git svn tests, svn not found'\n     test_done\n fi\n \n@@ -40,13 +40,12 @@ x=$?\n if test $x -ne 0\n then\n \tif test $x -eq 42; then\n-\t\terr='Perl SVN libraries must be >= 1.1.0'\n+\t\tskip_all='Perl SVN libraries must be >= 1.1.0'\n \telif test $x -eq 41; then\n-\t\terr='svnadmin failed to create fsfs repository'\n+\t\tskip_all='svnadmin failed to create fsfs repository'\n \telse\n-\t\terr='Perl SVN libraries not found or unusable, skipping test'\n+\t\tskip_all='Perl SVN libraries not found or unusable'\n \tfi\n-\tsay \"$err\"\n \ttest_done\n fi\n \n@@ -159,7 +158,7 @@ EOF\n require_svnserve () {\n     if test -z \"$SVNSERVE_PORT\"\n     then\n-        say 'skipping svnserve test. (set $SVNSERVE_PORT to enable)'\n+        skip_all='skipping svnserve test. (set $SVNSERVE_PORT to enable)'\n         test_done\n     fi\n }\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex a0944d6..71effc5 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -45,7 +45,7 @@ HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www\n \n if ! test -x \"$LIB_HTTPD_PATH\"\n then\n-\tsay \"skipping test, no web server found at '$LIB_HTTPD_PATH'\"\n+\tskip_all=\"skipping test, no web server found at '$LIB_HTTPD_PATH'\"\n \ttest_done\n fi\n \n@@ -58,12 +58,12 @@ then\n \tthen\n \t\tif ! test $HTTPD_VERSION -ge 2\n \t\tthen\n-\t\t\tsay \"skipping test, at least Apache version 2 is required\"\n+\t\t\tskip_all=\"skipping test, at least Apache version 2 is required\"\n \t\t\ttest_done\n \t\tfi\n \t\tif ! test -d \"$DEFAULT_HTTPD_MODULE_PATH\"\n \t\tthen\n-\t\t\tsay \"Apache module directory not found.  Skipping tests.\"\n+\t\t\tskip_all=\"Apache module directory not found.  Skipping tests.\"\n \t\t\ttest_done\n \t\tfi\n \n@@ -118,7 +118,7 @@ start_httpd() {\n \t\t>&3 2>&4\n \tif test $? -ne 0\n \tthen\n-\t\tsay \"skipping test, web server setup failed\"\n+\t\tskip_all=\"skipping test, web server setup failed\"\n \t\ttrap 'die' EXIT\n \t\ttest_done\n \tfi\ndiff --git a/t/lib-patch-mode.sh b/t/lib-patch-mode.sh\nindex ce36f34..375e248 100644\n--- a/t/lib-patch-mode.sh\n+++ b/t/lib-patch-mode.sh\n@@ -3,7 +3,7 @@\n . ./test-lib.sh\n \n if ! test_have_prereq PERL; then\n-\tsay 'skipping --patch tests, perl not available'\n+\tskip_all='skipping --patch tests, perl not available'\n \ttest_done\n fi\n \ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex 4ee7b65..637d8e9 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -15,7 +15,7 @@ if sys.hexversion < 0x02040000:\n then\n \t:\n else\n-\tsay 'skipping git remote-testgit tests: requires Python 2.4 or newer'\n+\tskip_all='skipping git remote-testgit tests: requires Python 2.4 or newer'\n \ttest_done\n fi\n \ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex fe60d69..26ddf9d 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -113,7 +113,7 @@ done\n \n if ! echo 'echo space > \"$1\"' > \"e space.sh\"\n then\n-\tsay \"Skipping; FS does not support spaces in filenames\"\n+\tskip_all=\"Skipping; FS does not support spaces in filenames\"\n \ttest_done\n fi\n \n-- \n1.7.0.4\n"},{"id":"145199","messageId":"4C36ECDB.9090707@drmicha.warpmail.net","threadId":"24327","inReplyTo":"1278551766-27569-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH] tests: Use skip_all=* to skip tests","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-07-09T09:33:15Z","receivedAt":"2010-07-09T09:33:15Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"After trying out tests with prove (I like it!) I was just about to make\na patch before I saw this...\n\nSo, this does what it should, at least with my set of prerequisites.\n\nÆvar Arnfjörð Bjarmason venit, vidit, dixit 08.07.2010 03:16:\n> Change tests to skip with skip_all=* + test_done instead of using say\n> + test_done.\n\nI didn't understand this at all at first, only after I was about to\nwrite that patch myself. Maybe 'with skip_all=\"reason\"' or\n'skip_all=<reason>' etc.?\n\n> \n> This is a follow-up to \"tests: Skip tests in a way that makes sense\n> under TAP\" (fadb5156e4). I missed these cases when prepearing that\n\n\"preparing\", although that last sentence sounds more like \"after ---\nmaterial\".\n\n> patch, hopefully this is all of them.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nNo comments to the patch itself, it works :)\n\nMichael\n"},{"id":"145203","messageId":"1278675688-4702-1-git-send-email-avarab@gmail.com","threadId":"24327","inReplyTo":"4C36ECDB.9090707@drmicha.warpmail.net","subject":"[PATCH v2] tests: Use skip_all=<reason> to skip tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-09T11:41:28Z","receivedAt":"2010-07-09T11:41:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change tests to skip with skip_all=<reason> + test_done, instead of\nusing say <reason> + test_done.\n\nThis is a follow-up to \"tests: Skip tests in a way that makes sense\nunder TAP\" (fadb5156e4). I missed these cases when preparing that\npatch, hopefully this is all of them.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Fri, Jul 9, 2010 at 09:33, Michael J Gruber <git@drmicha.warpmail.net> wrote:\n> After trying out tests with prove (I like it!) I was just about to make\n> a patch before I saw this...\n\nGreat, good to know that you find the TAP support useful.\n\n> So, this does what it should, at least with my set of prerequisites.\n>\n> Ævar Arnfjörð Bjarmason venit, vidit, dixit 08.07.2010 03:16:\n>> Change tests to skip with skip_all=* + test_done instead of using say\n>> + test_done.\n>\n> I didn't understand this at all at first, only after I was about to\n> write that patch myself. Maybe 'with skip_all=\"reason\"' or\n> 'skip_all=<reason>' etc.?\n\nChanged the subject + body of the patch to use <reason>\n\n>> This is a follow-up to \"tests: Skip tests in a way that makes sense\n>> under TAP\" (fadb5156e4). I missed these cases when prepearing that\n>\n> \"preparing\", although that last sentence sounds more like \"after ---\n> material\".\n\nFixed the spelling error. But I think it'd be useful to have this when\nbrowsing the history of t/ in the future, so I kept the note in the\npatch, not just in the E-Mail.\n\n>> patch, hopefully this is all of them.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>\n> No comments to the patch itself, it works :)\n\n\n t/gitweb-lib.sh           |    4 ++--\n t/lib-cvs.sh              |    6 +++---\n t/lib-git-svn.sh          |   11 +++++------\n t/lib-httpd.sh            |    8 ++++----\n t/lib-patch-mode.sh       |    2 +-\n t/t5800-remote-helpers.sh |    2 +-\n t/t7005-editor.sh         |    2 +-\n 7 files changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh\nindex b70b891..81ef2a0 100644\n--- a/t/gitweb-lib.sh\n+++ b/t/gitweb-lib.sh\n@@ -76,12 +76,12 @@ gitweb_run () {\n . ./test-lib.sh\n \n if ! test_have_prereq PERL; then\n-\tsay 'skipping gitweb tests, perl not available'\n+\tskip_all='skipping gitweb tests, perl not available'\n \ttest_done\n fi\n \n perl -MEncode -e 'decode_utf8(\"\", Encode::FB_CROAK)' >/dev/null 2>&1 || {\n-    say 'skipping gitweb tests, perl version is too old'\n+    skip_all='skipping gitweb tests, perl version is too old'\n     test_done\n }\n \ndiff --git a/t/lib-cvs.sh b/t/lib-cvs.sh\nindex 4b3b793..648d161 100644\n--- a/t/lib-cvs.sh\n+++ b/t/lib-cvs.sh\n@@ -9,7 +9,7 @@ export HOME\n \n if ! type cvs >/dev/null 2>&1\n then\n-\tsay 'skipping cvsimport tests, cvs not found'\n+\tskip_all='skipping cvsimport tests, cvs not found'\n \ttest_done\n fi\n \n@@ -21,11 +21,11 @@ case \"$cvsps_version\" in\n 2.1 | 2.2*)\n \t;;\n '')\n-\tsay 'skipping cvsimport tests, cvsps not found'\n+\tskip_all='skipping cvsimport tests, cvsps not found'\n \ttest_done\n \t;;\n *)\n-\tsay 'skipping cvsimport tests, unsupported cvsps version'\n+\tskip_all='skipping cvsimport tests, unsupported cvsps version'\n \ttest_done\n \t;;\n esac\ndiff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\nindex 344785d..f62f617 100644\n--- a/t/lib-git-svn.sh\n+++ b/t/lib-git-svn.sh\n@@ -21,7 +21,7 @@ PERL=${PERL:-perl}\n svn >/dev/null 2>&1\n if test $? -ne 1\n then\n-    say 'skipping git svn tests, svn not found'\n+    skip_all='skipping git svn tests, svn not found'\n     test_done\n fi\n \n@@ -40,13 +40,12 @@ x=$?\n if test $x -ne 0\n then\n \tif test $x -eq 42; then\n-\t\terr='Perl SVN libraries must be >= 1.1.0'\n+\t\tskip_all='Perl SVN libraries must be >= 1.1.0'\n \telif test $x -eq 41; then\n-\t\terr='svnadmin failed to create fsfs repository'\n+\t\tskip_all='svnadmin failed to create fsfs repository'\n \telse\n-\t\terr='Perl SVN libraries not found or unusable, skipping test'\n+\t\tskip_all='Perl SVN libraries not found or unusable'\n \tfi\n-\tsay \"$err\"\n \ttest_done\n fi\n \n@@ -159,7 +158,7 @@ EOF\n require_svnserve () {\n     if test -z \"$SVNSERVE_PORT\"\n     then\n-        say 'skipping svnserve test. (set $SVNSERVE_PORT to enable)'\n+        skip_all='skipping svnserve test. (set $SVNSERVE_PORT to enable)'\n         test_done\n     fi\n }\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex a0944d6..71effc5 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -45,7 +45,7 @@ HTTPD_DOCUMENT_ROOT_PATH=$HTTPD_ROOT_PATH/www\n \n if ! test -x \"$LIB_HTTPD_PATH\"\n then\n-\tsay \"skipping test, no web server found at '$LIB_HTTPD_PATH'\"\n+\tskip_all=\"skipping test, no web server found at '$LIB_HTTPD_PATH'\"\n \ttest_done\n fi\n \n@@ -58,12 +58,12 @@ then\n \tthen\n \t\tif ! test $HTTPD_VERSION -ge 2\n \t\tthen\n-\t\t\tsay \"skipping test, at least Apache version 2 is required\"\n+\t\t\tskip_all=\"skipping test, at least Apache version 2 is required\"\n \t\t\ttest_done\n \t\tfi\n \t\tif ! test -d \"$DEFAULT_HTTPD_MODULE_PATH\"\n \t\tthen\n-\t\t\tsay \"Apache module directory not found.  Skipping tests.\"\n+\t\t\tskip_all=\"Apache module directory not found.  Skipping tests.\"\n \t\t\ttest_done\n \t\tfi\n \n@@ -118,7 +118,7 @@ start_httpd() {\n \t\t>&3 2>&4\n \tif test $? -ne 0\n \tthen\n-\t\tsay \"skipping test, web server setup failed\"\n+\t\tskip_all=\"skipping test, web server setup failed\"\n \t\ttrap 'die' EXIT\n \t\ttest_done\n \tfi\ndiff --git a/t/lib-patch-mode.sh b/t/lib-patch-mode.sh\nindex ce36f34..375e248 100644\n--- a/t/lib-patch-mode.sh\n+++ b/t/lib-patch-mode.sh\n@@ -3,7 +3,7 @@\n . ./test-lib.sh\n \n if ! test_have_prereq PERL; then\n-\tsay 'skipping --patch tests, perl not available'\n+\tskip_all='skipping --patch tests, perl not available'\n \ttest_done\n fi\n \ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex 4ee7b65..637d8e9 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -15,7 +15,7 @@ if sys.hexversion < 0x02040000:\n then\n \t:\n else\n-\tsay 'skipping git remote-testgit tests: requires Python 2.4 or newer'\n+\tskip_all='skipping git remote-testgit tests: requires Python 2.4 or newer'\n \ttest_done\n fi\n \ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex fe60d69..26ddf9d 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -113,7 +113,7 @@ done\n \n if ! echo 'echo space > \"$1\"' > \"e space.sh\"\n then\n-\tsay \"Skipping; FS does not support spaces in filenames\"\n+\tskip_all=\"Skipping; FS does not support spaces in filenames\"\n \ttest_done\n fi\n \n-- \n1.7.0.4\n"},{"id":"145232","messageId":"7v4og8mtpd.fsf@alter.siamese.dyndns.org","threadId":"24327","inReplyTo":"1278675688-4702-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH v2] tests: Use skip_all=<reason> to skip tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-09T17:49:18Z","receivedAt":"2010-07-09T17:49:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change tests to skip with skip_all=<reason> + test_done, instead of\n> using say <reason> + test_done.\n>\n> This is a follow-up to \"tests: Skip tests in a way that makes sense\n> under TAP\" (fadb5156e4). I missed these cases when preparing that\n> patch, hopefully this is all of them.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>\n> On Fri, Jul 9, 2010 at 09:33, Michael J Gruber <git@drmicha.warpmail.net> wrote:\n>> After trying out tests with prove (I like it!) I was just about to make\n>> a patch before I saw this...\n>\n> Great, good to know that you find the TAP support useful.\n>\n>> So, this does what it should, at least with my set of prerequisites.\n>>\n>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 08.07.2010 03:16:\n>>> Change tests to skip with skip_all=* + test_done instead of using say\n>>> + test_done.\n>>\n>> I didn't understand this at all at first, only after I was about to\n>> write that patch myself. Maybe 'with skip_all=\"reason\"' or\n>> 'skip_all=<reason>' etc.?\n>\n> Changed the subject + body of the patch to use <reason>\n\nThat makes sense, but I am beginning to hate this skip_all business.\n\nThere is no \"skip\"-ness to what the variable does from the semantic point\nof view.  In the context of test helper library whose service consists of\ncounting the number of tests that succeeded, failed, and are known to be\nstill broken, one would expect that the word \"skip\" would mean it somehow\nwould help counting the tests that are prepared by the test author but\nwere not run for some reason, but this variable is not about that (note\nthat I am not suggesting us to actually count and say \"N tests skipped\").\nIt is only about giving parting words at the end before the test script\nexits.\n\nThe variable is not about skip_ALL either.  In the majority of cases, it\nis more like \"finishing here, telling the user that we are doing so\nwithout running the remainder of the script\", and in one case, it is more\nlike \"skipped one test, reporting after the fact\".\n\nAmong 63 assignments to $skip_all that are all over in t/*.sh scripts, the\nonly ones that are not immediately followed by test_done are in\nlib-git-svn.sh (chooses one among 3 messages), lib-httpd.sh (sets a trap\nbefore calling test_done), and t3600-rm (makes a mental note to report\nthat one test was skipped long before all the tests run).\n\nI suspect that it might be much easier in the long run for test writers if\nwe made test_done take optional \"parting words\" parameter instead of using\na global variable \"$skip_all\" and forcing them to carefully set it.  Then\nwe can lose special meaning to the global variable $skip_all, most of the\nscripts that currently assign to the variable do not need the assignments,\nand only the very few special cases can use $skip_all as their local\nconvention to decide what optional \"parting words\" parameter to give to\nthe test_done helper.\n\nThe change would look something like this (I just did a few as a\ndemonstration).\n\nWhat do you think?\n\n t/lib-git-svn.sh            |   11 ++++-------\n t/t2007-checkout-symlink.sh |    3 +--\n t/test-lib.sh               |    5 +----\n 3 files changed, 6 insertions(+), 13 deletions(-)\n\ndiff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\nindex c3f6676..cfc0d5b 100644\n--- a/t/lib-git-svn.sh\n+++ b/t/lib-git-svn.sh\n@@ -5,12 +5,10 @@ git_svn_id=git\"\"-svn-id\n \n if test -n \"$NO_SVN_TESTS\"\n then\n-\tskip_all='skipping git svn tests, NO_SVN_TESTS defined'\n-\ttest_done\n+\ttest_done 'skipping git svn tests, NO_SVN_TESTS defined'\n fi\n if ! test_have_prereq PERL; then\n-\tskip_all='skipping git svn tests, perl not available'\n-\ttest_done\n+\ttest_done 'skipping git svn tests, perl not available'\n fi\n \n GIT_DIR=$PWD/.git\n@@ -21,8 +19,7 @@ PERL=${PERL:-perl}\n svn >/dev/null 2>&1\n if test $? -ne 1\n then\n-    skip_all='skipping git svn tests, svn not found'\n-    test_done\n+    test_done 'skipping git svn tests, svn not found'\n fi\n \n svnrepo=$PWD/svnrepo\n@@ -46,7 +43,7 @@ then\n \telse\n \t\tskip_all='Perl SVN libraries not found or unusable'\n \tfi\n-\ttest_done\n+\ttest_done \"$skip_all\"\n fi\n \n rawsvnrepo=\"$svnrepo\"\ndiff --git a/t/t2007-checkout-symlink.sh b/t/t2007-checkout-symlink.sh\nindex 05cc8fd..fd3d7be 100755\n--- a/t/t2007-checkout-symlink.sh\n+++ b/t/t2007-checkout-symlink.sh\n@@ -8,8 +8,7 @@ test_description='git checkout to switch between branches with symlink<->dir'\n \n if ! test_have_prereq SYMLINKS\n then\n-\tskip_all=\"symbolic links not supported - skipping tests\"\n-\ttest_done\n+\ttest_done \"symbolic links not supported - skipping tests\"\n fi\n \n test_expect_success setup '\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ac496aa..2c4474d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -657,12 +657,9 @@ test_done () {\n \tfi\n \tcase \"$test_failure\" in\n \t0)\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\tsay \"1..$test_count${1:+\" # SKIP $1\"}\"\n \t\tfi\n \n \t\ttest -d \"$remove_trash\" &&\n"},{"id":"145234","messageId":"AANLkTilPkKI9KdEc84Zhh3Vn7_B7CYT3UDSkSv7JwGJG@mail.gmail.com","threadId":"24327","inReplyTo":"7v4og8mtpd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] tests: Use skip_all=<reason> to skip tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-09T19:14:10Z","receivedAt":"2010-07-09T19:14:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jul 9, 2010 at 17:49, Junio C Hamano <gitster@pobox.com> wrote:\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Change tests to skip with skip_all=<reason> + test_done, instead of\n>> using say <reason> + test_done.\n>>\n>> This is a follow-up to \"tests: Skip tests in a way that makes sense\n>> under TAP\" (fadb5156e4). I missed these cases when preparing that\n>> patch, hopefully this is all of them.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>\n>> On Fri, Jul 9, 2010 at 09:33, Michael J Gruber <git@drmicha.warpmail.net> wrote:\n>>> After trying out tests with prove (I like it!) I was just about to make\n>>> a patch before I saw this...\n>>\n>> Great, good to know that you find the TAP support useful.\n>>\n>>> So, this does what it should, at least with my set of prerequisites.\n>>>\n>>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 08.07.2010 03:16:\n>>>> Change tests to skip with skip_all=* + test_done instead of using say\n>>>> + test_done.\n>>>\n>>> I didn't understand this at all at first, only after I was about to\n>>> write that patch myself. Maybe 'with skip_all=\"reason\"' or\n>>> 'skip_all=<reason>' etc.?\n>>\n>> Changed the subject + body of the patch to use <reason>\n>\n> That makes sense, but I am beginning to hate this skip_all business.\n\nI'm not sure I like the interface either. It's just something that did\nthe job. But I do have some grand plans to make the test-lib.sh more\nTAP-ish, including adding more advanced skip features.\n\n> There is no \"skip\"-ness to what the variable does from the semantic point\n> of view.  In the context of test helper library whose service consists of\n> counting the number of tests that succeeded, failed, and are known to be\n> still broken, one would expect that the word \"skip\" would mean it somehow\n> would help counting the tests that are prepared by the test author but\n> were not run for some reason, but this variable is not about that (note\n> that I am not suggesting us to actually count and say \"N tests skipped\").\n> It is only about giving parting words at the end before the test script\n> exits.\n\nThe reason this variable exists is that there is a semantic\ndifference. The test-results/* aggregator that ships with Git doesn't\ncare about whether a passing test was a TODO test, or record the\nreasons for why something was skipped. But TAP harnesses do record and\nreport this.\n\nThe idea is that we'll be able to set up smoke testing machines for\ngit, and they'll be able to report passing/failed/TODO passing/TODO\nfailing/skipped (why?) tests.\n\n> The variable is not about skip_ALL either.  In the majority of cases, it\n> is more like \"finishing here, telling the user that we are doing so\n> without running the remainder of the script\", and in one case, it is more\n> like \"skipped one test, reporting after the fact\".\n\nCuriously enough, I initially called it \"skip_all_remaining=<reason>\",\nbut thought it was too verbose. That's what it does now, anyway. Skips\nall remaining tests.\n\n> Among 63 assignments to $skip_all that are all over in t/*.sh scripts, the\n> only ones that are not immediately followed by test_done are in\n> lib-git-svn.sh (chooses one among 3 messages), lib-httpd.sh (sets a trap\n> before calling test_done), and t3600-rm (makes a mental note to report\n> that one test was skipped long before all the tests run).\n>\n> I suspect that it might be much easier in the long run for test writers if\n> we made test_done take optional \"parting words\" parameter instead of using\n> a global variable \"$skip_all\" and forcing them to carefully set it.\n\nHaving a magic string like you suggest is overloading the API a\nbit. But my opinion differs from yours on this I suspect because I'm\ntrying to convey a semantic difference. I.e. the skip message isn't\njust a custom \"bye\" message.\n\nRather than this:\n\n    test_done 'skipping git svn tests, perl not available'\n\nWhich doesn't indicate that the \"skipping\" part is magic. Perhaps this\nis better:\n\n    test_done \"bye bye now\"\n    test_done \"skip_remaining\" \"can't test SVN here\"\n\nOr something. Anyway, personally I think that's overloading the API a\nbit. I experimented with doing that before devising skip_all. And\ndecided to make test_done just work like Test::More's done_testing()\ninstead.\n\n> Then we can lose special meaning to the global variable $skip_all,\n> most of the scripts that currently assign to the variable do not\n> need the assignments, and only the very few special cases can use\n> $skip_all as their local convention to decide what optional \"parting\n> words\" parameter to give to the test_done helper.\n\n> The change would look something like this (I just did a few as a\n> demonstration).\n>\n> What do you think?\n\nThat should work. Personally I don't really care what the API ends up\nlooking like (beyond the slight grumpiness associated with having to\namend existing code).\n\nI just care that it's clear & concise. Which admittedly the current\nskip_all=<reason> & test_done combo isn't a good example of.\n\nAnyway. I suppose now is as good a time as any to share some of my\nevil plans.\n\nWhen I submitted the TAP series I really just did the bare minimum to\nget things working. But there's a lot more that could be done.\n\nFor example, in the test case attached to my gettext patch I skip\ntests on platforms that don't have the prerequisite locale files. But\nthere's no indication to the user that something is amiss.\n\nI was planning to add support for skipping tests in the middle of the\ntest files, so you could do this:\n\n    $ perl -MTest::More -E 'plan tests => 4; pass \"gettext stuff ok\"\nfor 1..2; SKIP: { skip \"Can not test without locale files\", 2 }'\n    1..4\n    ok 1 - gettext stuff ok\n    ok 2 - gettext stuff ok\n    ok 3 # skip Can not test without locale files\n    ok 4 # skip Can not test without locale files\n\nAnd a TAP harness could subsequently report on how many tests were\nskipped, and we might e.g. see that HP-UX is really unloved because\nwe're skipping 500/6000 tests or something.\n\nThis also allows for skipping tests in the middle of the file for some\nreason:\n\n    $ perl -MTest::More -E 'plan tests => 6; pass \"gettext stuff ok\"\nfor 1..2; SKIP: { skip \"Can not test without locale files\", 2 } pass\n\"moo\" for 1..2'\n    1..6\n    ok 1 - gettext stuff ok\n    ok 2 - gettext stuff ok\n    ok 3 # skip Can not test without locale files\n    ok 4 # skip Can not test without locale files\n    ok 5 - moo\n    ok 6 - moo\n\nYou'll also notice that I planned the number of tests /before/ I\nstarted running them. We don't do this now, but some tests can really\nbenefit from that (although I haven't spotted anything like that in\nGit). At worst you'll get accurate test progress meters when running\nprove.\n\n</braindump>\n\n>  t/lib-git-svn.sh            |   11 ++++-------\n>  t/t2007-checkout-symlink.sh |    3 +--\n>  t/test-lib.sh               |    5 +----\n>  3 files changed, 6 insertions(+), 13 deletions(-)\n>\n> diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh\n> index c3f6676..cfc0d5b 100644\n> --- a/t/lib-git-svn.sh\n> +++ b/t/lib-git-svn.sh\n> @@ -5,12 +5,10 @@ git_svn_id=git\"\"-svn-id\n>\n>  if test -n \"$NO_SVN_TESTS\"\n>  then\n> -       skip_all='skipping git svn tests, NO_SVN_TESTS defined'\n> -       test_done\n> +       test_done 'skipping git svn tests, NO_SVN_TESTS defined'\n>  fi\n>  if ! test_have_prereq PERL; then\n> -       skip_all='skipping git svn tests, perl not available'\n> -       test_done\n> +       test_done 'skipping git svn tests, perl not available'\n>  fi\n>\n>  GIT_DIR=$PWD/.git\n> @@ -21,8 +19,7 @@ PERL=${PERL:-perl}\n>  svn >/dev/null 2>&1\n>  if test $? -ne 1\n>  then\n> -    skip_all='skipping git svn tests, svn not found'\n> -    test_done\n> +    test_done 'skipping git svn tests, svn not found'\n>  fi\n>\n>  svnrepo=$PWD/svnrepo\n> @@ -46,7 +43,7 @@ then\n>        else\n>                skip_all='Perl SVN libraries not found or unusable'\n>        fi\n> -       test_done\n> +       test_done \"$skip_all\"\n>  fi\n>\n>  rawsvnrepo=\"$svnrepo\"\n> diff --git a/t/t2007-checkout-symlink.sh b/t/t2007-checkout-symlink.sh\n> index 05cc8fd..fd3d7be 100755\n> --- a/t/t2007-checkout-symlink.sh\n> +++ b/t/t2007-checkout-symlink.sh\n> @@ -8,8 +8,7 @@ test_description='git checkout to switch between branches with symlink<->dir'\n>\n>  if ! test_have_prereq SYMLINKS\n>  then\n> -       skip_all=\"symbolic links not supported - skipping tests\"\n> -       test_done\n> +       test_done \"symbolic links not supported - skipping tests\"\n>  fi\n>\n>  test_expect_success setup '\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index ac496aa..2c4474d 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -657,12 +657,9 @@ test_done () {\n>        fi\n>        case \"$test_failure\" in\n>        0)\n> -               # Maybe print SKIP message\n> -               [ -z \"$skip_all\" ] || skip_all=\" # SKIP $skip_all\"\n> -\n>                if test $test_external_has_tap -eq 0; then\n>                        say_color pass \"# passed all $msg\"\n> -                       say \"1..$test_count$skip_all\"\n> +                       say \"1..$test_count${1:+\" # SKIP $1\"}\"\n>                fi\n>\n>                test -d \"$remove_trash\" &&\n>\n"},{"id":"145247","messageId":"7vlj9kl1bv.fsf@alter.siamese.dyndns.org","threadId":"24327","inReplyTo":"AANLkTilPkKI9KdEc84Zhh3Vn7_B7CYT3UDSkSv7JwGJG@mail.gmail.com","subject":"Re: [PATCH v2] tests: Use skip_all=<reason> to skip tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-09T22:47:32Z","receivedAt":"2010-07-09T22:47:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Fri, Jul 9, 2010 at 17:49, Junio C Hamano <gitster@pobox.com> wrote:\n> ...\n>> Among 63 assignments to $skip_all that are all over in t/*.sh scripts, the\n>> only ones that are not immediately followed by test_done are in\n>> lib-git-svn.sh (chooses one among 3 messages), lib-httpd.sh (sets a trap\n>> before calling test_done), and t3600-rm (makes a mental note to report\n>> that one test was skipped long before all the tests run).\n> ...\n>     $ perl -MTest::More -E 'plan tests => 6; pass \"gettext stuff ok\"\n> for 1..2; SKIP: { skip \"Can not test without locale files\", 2 } pass\n> \"moo\" for 1..2'\n>     1..6\n>     ok 1 - gettext stuff ok\n>     ok 2 - gettext stuff ok\n>     ok 3 # skip Can not test without locale files\n>     ok 4 # skip Can not test without locale files\n>     ok 5 - moo\n>     ok 6 - moo\n\nNow you are talking.\n\nWhat t3600-rm does becomes a lot more natural to express with something\nlike this.  Any test with \"prerequisite\" missing will automatically get\n\"this test was skipped because you lack this prerequisite\" for free.  We\ncan lose skip_all= assignment there but move the logic to test-lib.sh,\nwhich is a good thing.\n\nBut that is orthogonal to what you call an API, i.e. your assignment to\nthe global variable $skip_all that is immediately followed by test_done,\nno?  The conversion you did for that does not help counting the remainder\nof the tests that are skipped anyway, so you will need to redo that\nconversion altogether if you ever want to be able to show \"these tests\nthrough the end of the scripts were skipped\".\n"},{"id":"145249","messageId":"AANLkTinAG4FgzsWyLtcVPu9Lz3e9Jp_PLlGRCuJeIWPN@mail.gmail.com","threadId":"24327","inReplyTo":"7vlj9kl1bv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] tests: Use skip_all=<reason> to skip tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-09T23:28:35Z","receivedAt":"2010-07-09T23:28:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jul 9, 2010 at 22:47, Junio C Hamano <gitster@pobox.com> wrote:\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> On Fri, Jul 9, 2010 at 17:49, Junio C Hamano <gitster@pobox.com> wrote:\n>> ...\n>>> Among 63 assignments to $skip_all that are all over in t/*.sh scripts, the\n>>> only ones that are not immediately followed by test_done are in\n>>> lib-git-svn.sh (chooses one among 3 messages), lib-httpd.sh (sets a trap\n>>> before calling test_done), and t3600-rm (makes a mental note to report\n>>> that one test was skipped long before all the tests run).\n>> ...\n>>     $ perl -MTest::More -E 'plan tests => 6; pass \"gettext stuff ok\"\n>> for 1..2; SKIP: { skip \"Can not test without locale files\", 2 } pass\n>> \"moo\" for 1..2'\n>>     1..6\n>>     ok 1 - gettext stuff ok\n>>     ok 2 - gettext stuff ok\n>>     ok 3 # skip Can not test without locale files\n>>     ok 4 # skip Can not test without locale files\n>>     ok 5 - moo\n>>     ok 6 - moo\n>\n> Now you are talking.\n>\n> What t3600-rm does becomes a lot more natural to express with something\n> like this.  Any test with \"prerequisite\" missing will automatically get\n> \"this test was skipped because you lack this prerequisite\" for free.  We\n> can lose skip_all= assignment there but move the logic to test-lib.sh,\n> which is a good thing.\n\nRight we already do this in the case of t3600-rm.sh. I think in the\ngeneral case that using test_set_prereq + 3 arg test_expect_success\n(i.e. declare prereq) is a lot better than arranging tests so that you\ncan skip a lot of dependency-heavy tests at the end.\n\n> But that is orthogonal to what you call an API, i.e. your assignment to\n> the global variable $skip_all that is immediately followed by test_done,\n> no?  The conversion you did for that does not help counting the remainder\n> of the tests that are skipped anyway, so you will need to redo that\n> conversion altogether if you ever want to be able to show \"these tests\n> through the end of the scripts were skipped\".\n\nRight, those two are completely orthogonal. That sort of API would\nlook something like this:\n\n    #!/bin/sh\n\n    plan=6\n\n    . ./test-lib.sh\n\n    for i in {1..2}; do\n        test_expect_success 'gettext stuff ok' 'true'\n    done\n\n    # something came up, skip the remainder\n    skip_all=\"Oh noes\"\n    test_done\n\n(or test_done 'Oh noes', or something).\n\nAt that point it knows to output 4 skip lines with the message \"Oh\nnoes\" because we've only run two tests so far.\n"}]}