{"thread":{"id":"24702","subject":"[PATCH] test-lib: Multi-prereq support only checked the last prereq","startedAt":"2010-08-10T23:21:51Z","lastAt":"2010-08-24T19:01:21Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Jonathan Nieder","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"147702","messageId":"1281482511-2461-1-git-send-email-avarab@gmail.com","threadId":"24702","inReplyTo":null,"subject":"[PATCH] test-lib: Multi-prereq support only checked the last prereq","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-10T23:21:51Z","receivedAt":"2010-08-10T23:21:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The support for multiple test prerequisites added by me in \"test-lib:\nAdd support for multiple test prerequisites\" was broken.\n\nThe clever for-loop (which I blindly copied from Junio) iterated over\neach prerequisite and returned true/false within a case statement.\nThus only the last prerequisite in the list of prerequisites was ever\nconsidered, the rest were ignored.\n\nFix that by changing the test_have_prereq code to something less\nclever that keeps a count of the total prereqs and the ones we have\nand compares the count at the end.\n\nThis comes with the added advantage that it's easy to list the missing\nprerequisites in the test output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nI should have spotted this earlier.\n\n t/t0000-basic.sh |    6 +++++-\n t/test-lib.sh    |   21 ++++++++++++++++++---\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 2887677..9602085 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -84,7 +84,11 @@ donthaveit=yes\n test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '\n     donthaveit=no\n '\n-if test $haveit$donthaveit != yesyes\n+donthaveiteither=yes\n+test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' '\n+    donthaveiteither=no\n+'\n+if test $haveit$donthaveit$donthaveiteither != yesyesyes\n then\n \tsay \"bug in test framework: multiple prerequisite tags do not work reliably\"\n \texit 1\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 4e73fff..8c8e129 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -337,15 +337,30 @@ test_have_prereq () {\n \tIFS=,\n \tset -- $*\n \tIFS=$save_IFS\n+\n+\ttotal_prereq=0\n+\tok_prereq=0\n+\tmissing_prereq=\n+\n \tfor prerequisite\n \tdo\n+\t\ttotal_prereq=$(($total_prereq + 1))\n \t\tcase $satisfied in\n \t\t*\" $prerequisite \"*)\n-\t\t\t: yes, have it ;;\n+\t\t\tok_prereq=$(($ok_prereq + 1))\n+\t\t\t;;\n \t\t*)\n-\t\t\t! : nope ;;\n+\t\t\t# Keep a list of missing prerequisites\n+\t\t\tif test -z \"$missing_prereq\"\n+\t\t\tthen\n+\t\t\t\tmissing_prereq=$prerequisite\n+\t\t\telse\n+\t\t\t\tmissing_prereq=\"$prerequisite,$missing_prereq\"\n+\t\t\tfi\n \t\tesac\n \tdone\n+\n+\ttest $total_prereq = $ok_prereq\n }\n \n # You are not expected to call test_ok_ and test_failure_ directly, use\n@@ -408,7 +423,7 @@ test_skip () {\n \tcase \"$to_skip\" in\n \tt)\n \t\tsay_color skip >&3 \"skipping test: $@\"\n-\t\tsay_color skip \"ok $test_count # skip $1 (prereqs: $prereq)\"\n+\t\tsay_color skip \"ok $test_count # skip $1 (missing $missing_prereq of $prereq)\"\n \t\t: true\n \t\t;;\n \t*)\n-- \n1.7.2.1.295.gd03d\n"},{"id":"147712","messageId":"20100811014325.GA4169@burratino","threadId":"24702","inReplyTo":"1281482511-2461-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH] test-lib: Multi-prereq support only checked the last prereq","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-11T01:43:25Z","receivedAt":"2010-08-11T01:43:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> The clever for-loop (which I blindly copied from Junio)\n\nYou did not copy his “return” statement, though. :)\n\n> +++ b/t/test-lib.sh\n> @@ -337,15 +337,30 @@ test_have_prereq () {\n>  \tIFS=,\n>  \tset -- $*\n>  \tIFS=$save_IFS\n> +\n> +\ttotal_prereq=0\n> +\tok_prereq=0\n> +\tmissing_prereq=\n> +\n>  \tfor prerequisite\n>  \tdo\n> +\t\ttotal_prereq=$(($total_prereq + 1))\n>  \t\tcase $satisfied in\n>  \t\t*\" $prerequisite \"*)\n> -\t\t\t: yes, have it ;;\n> +\t\t\tok_prereq=$(($ok_prereq + 1))\n> +\t\t\t;;\n>  \t\t*)\n> -\t\t\t! : nope ;;\n> +\t\t\t# Keep a list of missing prerequisites\n> +\t\t\tif test -z \"$missing_prereq\"\n> +\t\t\tthen\n> +\t\t\t\tmissing_prereq=$prerequisite\n> +\t\t\telse\n> +\t\t\t\tmissing_prereq=\"$prerequisite,$missing_prereq\"\n> +\t\t\tfi\n>  \t\tesac\n>  \tdone\n> +\n> +\ttest $total_prereq = $ok_prereq\n>  }\n\nWouldn’t\n\n-\t\t! : nope ;;\n+\t\treturn 1 ;;\n\nbe simpler?\n\nThis and a small cleanup, including a test that catches this, are at\nhttp://thread.gmane.org/gmane.comp.version-control.git/152814/focus=152875\n\nHope that helps,\nJonathan\n"},{"id":"147755","messageId":"1281528278-15659-1-git-send-email-avarab@gmail.com","threadId":"24702","inReplyTo":"20100811014325.GA4169@burratino","subject":"[PATCH v2] test-lib: Multi-prereq support only checked the last prereq","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-11T12:04:38Z","receivedAt":"2010-08-11T12:04:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The support for multiple test prerequisites added by me in \"test-lib:\nAdd support for multiple test prerequisites\" was broken.\n\nThe for iterated over each prerequisite and returned true/false within\na case statement, but since it missed a return statement only the last\nprerequisite in the list of prerequisites was ever considered, the\nrest were ignored.\n\nFix that by changing the test_have_prereq code to something less\nclever that keeps a count of the total prereqs and the ones we have\nand compares the count at the end.\n\nThis comes with the added advantage that it's easy to list the missing\nprerequisites in the test output, implement that while I'm at it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThis v2 patch has a new commit message.\n\nOn Wed, Aug 11, 2010 at 01:43, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ævar Arnfjörð Bjarmason wrote:\n>\n>> The clever for-loop (which I blindly copied from Junio)\n>\n> You did not copy his “return” statement, though. :)\n\nOops, yeah. I copied it from some mix-match of different\npatches. Corrected the patch message for that.\n\n> Wouldn’t\n>\n> -               ! : nope ;;\n> +               return 1 ;;\n>\n> be simpler?\n\nYeah, that'd make it work, but I wanted to add something that spewed a\ncomplete listing of missing prereqs for ecah test, bailing out with a\nreturn wouldn't do that.\n\n t/t0000-basic.sh |    6 +++++-\n t/test-lib.sh    |   21 ++++++++++++++++++---\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 2887677..9602085 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -84,7 +84,11 @@ donthaveit=yes\n test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '\n     donthaveit=no\n '\n-if test $haveit$donthaveit != yesyes\n+donthaveiteither=yes\n+test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' '\n+    donthaveiteither=no\n+'\n+if test $haveit$donthaveit$donthaveiteither != yesyesyes\n then\n \tsay \"bug in test framework: multiple prerequisite tags do not work reliably\"\n \texit 1\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 4e73fff..8c8e129 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -337,15 +337,30 @@ test_have_prereq () {\n \tIFS=,\n \tset -- $*\n \tIFS=$save_IFS\n+\n+\ttotal_prereq=0\n+\tok_prereq=0\n+\tmissing_prereq=\n+\n \tfor prerequisite\n \tdo\n+\t\ttotal_prereq=$(($total_prereq + 1))\n \t\tcase $satisfied in\n \t\t*\" $prerequisite \"*)\n-\t\t\t: yes, have it ;;\n+\t\t\tok_prereq=$(($ok_prereq + 1))\n+\t\t\t;;\n \t\t*)\n-\t\t\t! : nope ;;\n+\t\t\t# Keep a list of missing prerequisites\n+\t\t\tif test -z \"$missing_prereq\"\n+\t\t\tthen\n+\t\t\t\tmissing_prereq=$prerequisite\n+\t\t\telse\n+\t\t\t\tmissing_prereq=\"$prerequisite,$missing_prereq\"\n+\t\t\tfi\n \t\tesac\n \tdone\n+\n+\ttest $total_prereq = $ok_prereq\n }\n \n # You are not expected to call test_ok_ and test_failure_ directly, use\n@@ -408,7 +423,7 @@ test_skip () {\n \tcase \"$to_skip\" in\n \tt)\n \t\tsay_color skip >&3 \"skipping test: $@\"\n-\t\tsay_color skip \"ok $test_count # skip $1 (prereqs: $prereq)\"\n+\t\tsay_color skip \"ok $test_count # skip $1 (missing $missing_prereq of $prereq)\"\n \t\t: true\n \t\t;;\n \t*)\n-- \n1.7.2.1.295.gdf931\n"},{"id":"148019","messageId":"4C65AAF8.80009@kdbg.org","threadId":"24702","inReplyTo":"1281528278-15659-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH v2] test-lib: Multi-prereq support only checked the last prereq","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-08-13T20:28:40Z","receivedAt":"2010-08-13T20:28:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.08.2010 14:04, schrieb Ævar Arnfjörð Bjarmason:\n> -\t\tsay_color skip \"ok $test_count # skip $1 (prereqs: $prereq)\"\n> +\t\tsay_color skip \"ok $test_count # skip $1 (missing $missing_prereq of $prereq)\"\n\nThis needs fixing: When a test is skipped due to being listed in \nGIT_SKIP_TESTS, then the output looks like this:\n\nok 47 # skip --ignore-submodules=untracked suppresses submodules with \nuntracked content (missing SANITY,POSIXPERM of )\n\n(In this case, there was a test before number 47 that was skipped due to \nmissing prerequisite.)\n\nPlease also note that - when a prerequisite is not satisfied - the text in \nparentheses does not say \"prerequisite\" or similar anymore. IMO, this \nshould be added back.\n\n-- Hannes\n"},{"id":"148832","messageId":"20100824073410.GA5562@burratino","threadId":"24702","inReplyTo":"1281528278-15659-1-git-send-email-avarab@gmail.com","subject":"[PATCH] tests: simplify \"missing PREREQ\" message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-24T07:34:10Z","receivedAt":"2010-08-24T07:34:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When a test has no prerequisites satisfied (the usual case), instead\nof \"missing THING of THING\", just say \"missing THING\".  This does not\naffect the output when a test is skipped due to a missing\nprerequisites if another prerequisite is satisfied.\n\nFor example: instead of\n\n ok 8 # skip notes work (missing EXPENSIVE of EXPENSIVE)\n ok 9 # skip notes timing with /usr/bin/time (missing EXPENSIVE of USR_BIN_TIME,EXPENSIVE)\n\nwrite\n\n ok 8 # skip notes work (missing EXPENSIVE)\n ok 9 # skip notes timing with /usr/bin/time (missing EXPENSIVE of USR_BIN_TIME,EXPENSIVE)\n\nCc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nNoticed this when building \"next\".  Thoughts?\n\n t/test-lib.sh |    8 +++++++-\n 1 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 4617998..c9193cb 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -417,8 +417,14 @@ test_skip () {\n \tfi\n \tcase \"$to_skip\" in\n \tt)\n+\t\tof_prereq=\n+\t\tif test \"$missing_prereq\" != \"$prereq\"\n+\t\tthen\n+\t\t\tof_prereq=\" of $prereq\"\n+\t\tfi\n+\n \t\tsay_color skip >&3 \"skipping test: $@\"\n-\t\tsay_color skip \"ok $test_count # skip $1 (missing $missing_prereq of $prereq)\"\n+\t\tsay_color skip \"ok $test_count # skip $1 (missing $missing_prereq${of_prereq})\"\n \t\t: true\n \t\t;;\n \t*)\n-- \n1.7.2.2\n"},{"id":"148834","messageId":"4C739BE2.5060200@viscovery.net","threadId":"24702","inReplyTo":"20100824073410.GA5562@burratino","subject":"Re: [PATCH] tests: simplify \"missing PREREQ\" message","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-08-24T10:16:02Z","receivedAt":"2010-08-24T10:16:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 8/24/2010 9:34, schrieb Jonathan Nieder:\n> @@ -417,8 +417,14 @@ test_skip () {\n>  \tfi\n>  \tcase \"$to_skip\" in\n>  \tt)\n> +\t\tof_prereq=\n> +\t\tif test \"$missing_prereq\" != \"$prereq\"\n> +\t\tthen\n> +\t\t\tof_prereq=\" of $prereq\"\n> +\t\tfi\n> +\n>  \t\tsay_color skip >&3 \"skipping test: $@\"\n> -\t\tsay_color skip \"ok $test_count # skip $1 (missing $missing_prereq of $prereq)\"\n> +\t\tsay_color skip \"ok $test_count # skip $1 (missing $missing_prereq${of_prereq})\"\n>  \t\t: true\n>  \t\t;;\n>  \t*)\n\nWhile you are touching this area, would also fix messages like these:\n\nok 70 # skip set --path (missing  of )\nok 71 # skip get --path (missing  of HOMEVAR)\nok 72 # skip get --path copes with unset $HOME (missing  of )\n\nwhich are due to GIT_SKIP_TESTS=t1300.7[012]\n\n-- Hannes\n"},{"id":"148860","messageId":"7v4oejvnr2.fsf@alter.siamese.dyndns.org","threadId":"24702","inReplyTo":"20100824073410.GA5562@burratino","subject":"Re: [PATCH] tests: simplify \"missing PREREQ\" message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-24T19:01:21Z","receivedAt":"2010-08-24T19:01:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Noticed this when building \"next\".  Thoughts?\n\nDefinitely nicer ;-)\n"}]}