{"thread":{"id":"56924","subject":"[PATCH v2 0/2] test-lib: improve missing prereq handling","startedAt":"2021-11-17T09:04:26Z","lastAt":"2021-12-01T23:13:44Z","messageCount":29,"participants":["Fabian Stelzer","Junio C Hamano","Ævar Arnfjörð Bjarmason","Adam Dinwoodie"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"441418","messageId":"20211117090410.8013-1-fs@gigacodes.de","threadId":"56924","inReplyTo":null,"subject":"[PATCH v2 0/2] test-lib: improve missing prereq handling","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-17T09:04:08Z","receivedAt":"2021-11-17T09:04:26Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"The ssh signing feature was breaking tests when the broken openssh-8.7\nwas used. We have now fixed that by checking for this exact case in the\nGPGSSH prereq and I will improve that check further in a future patch.\nHowever we are now in a situation where a broken openssh in the future\nwill result in successfull tests but not a working git build afterwards\n(either not compiling in the expected feature or like in the ssh case\nruntime failures) resulting in a false sense of security in the tests.\nThis patches try to improve this situation by showing which prereqs\nfailed in the test summary and by adding an environment variable to\nenforce certain prereqs to succeed or abort the test otherwise.\n\nSee also:\nhttps://public-inbox.org/git/xmqqv916wh7t.fsf@gitster.g/\n\nchanges since v1:\n - use \\012 instead of \\n for possible portability reasons\n - fix typo in commit msg\n\nFabian Stelzer (2):\n  test-lib: show missing prereq summary\n  test-lib: introduce required prereq for test runs\n\n t/README                |  6 ++++++\n t/aggregate-results.sh  | 17 +++++++++++++++++\n t/test-lib-functions.sh | 11 +++++++++++\n t/test-lib.sh           | 11 +++++++++++\n 4 files changed, 45 insertions(+)\n\nRange-diff against v1:\n1:  69e77cd854 ! 1:  775c0e5ef0 test-lib: show missing prereq summary\n    @@ t/aggregate-results.sh: do\n     +then\n     +\tunique_missing_prereq=$(\n     +\t\techo $missing_prereq |\n    -+\t\ttr -s \",\" \"\\n\" |\n    ++\t\ttr -s \",\" \"\\012\" |\n     +\t\tgrep -v '^$' |\n     +\t\tsort -u |\n     +\t\tpaste -s -d ',')\n2:  12bd18c5ce ! 2:  eb1bbb8d01 test-lib: introduce required prereq for test runs\n    @@ Commit message\n         test-lib: introduce required prereq for test runs\n     \n         In certain environments or for specific test scenarios we might expect a\n    -    specific prerequisite check to be succeed. Therefore we would like to\n    +    specific prerequisite check to succeed. Therefore we would like to\n         trigger an error when running our tests if this is not the case.\n     \n         To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n-- \n2.31.1\n\n"},{"id":"441419","messageId":"20211117090410.8013-2-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211117090410.8013-1-fs@gigacodes.de","subject":"[PATCH v2 1/2] test-lib: show missing prereq summary","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-17T09:04:09Z","receivedAt":"2021-11-17T09:04:31Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"When running the full test suite many tests can be skipped because of\nmissing prerequisites. It not easy right now to get an overview of which\nones are missing.\nWhen switching to a new machine or environment some libraries and tools\nmight be missing or maybe a dependency broke completely. In this case\nthe tests would indicate nothing since all dependant tests are simply\nskipped. This could hide broken behaviour or missing features in the\nbuild. Therefore this patch summarizes the missing prereqs at the end of\nthe test run making it easier to spot such cases.\n\n - Add failed prereqs to the test results.\n - Aggregate and then show them with the totals.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/aggregate-results.sh | 17 +++++++++++++++++\n t/test-lib.sh          | 11 +++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\nindex 7913e206ed..ce217b4c0e 100755\n--- a/t/aggregate-results.sh\n+++ b/t/aggregate-results.sh\n@@ -6,6 +6,7 @@ success=0\n failed=0\n broken=0\n total=0\n+missing_prereq=\n \n while read file\n do\n@@ -30,10 +31,26 @@ do\n \t\t\tbroken=$(($broken + $value)) ;;\n \t\ttotal)\n \t\t\ttotal=$(($total + $value)) ;;\n+\t\tmissing_prereq)\n+\t\t\tmissing_prereq=\"$missing_prereq,$value\" ;;\n \t\tesac\n \tdone <\"$file\"\n done\n \n+if test -n \"$missing_prereq\"\n+then\n+\tunique_missing_prereq=$(\n+\t\techo $missing_prereq |\n+\t\ttr -s \",\" \"\\012\" |\n+\t\tgrep -v '^$' |\n+\t\tsort -u |\n+\t\tpaste -s -d ',')\n+\tif test -n $unique_missing_prereq\n+\tthen\n+\t\tprintf \"\\nmissing prereq: $unique_missing_prereq\\n\\n\"\n+\tfi\n+fi\n+\n if test -n \"$failed_tests\"\n then\n \tprintf \"\\nfailed test(s):$failed_tests\\n\\n\"\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2679a7596a..f61da562f6 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -669,6 +669,8 @@ test_fixed=0\n test_broken=0\n test_success=0\n \n+test_missing_prereq=\n+\n test_external_has_tap=0\n \n die () {\n@@ -1069,6 +1071,14 @@ test_skip () {\n \t\t\tof_prereq=\" of $test_prereq\"\n \t\tfi\n \t\tskipped_reason=\"missing $missing_prereq${of_prereq}\"\n+\n+\t\t# Keep a list of all the missing prereq for result aggregation\n+\t\tif test -z \"$missing_prereq\"\n+\t\tthen\n+\t\t\ttest_missing_prereq=$missing_prereq\n+\t\telse\n+\t\t\ttest_missing_prereq=\"$test_missing_prereq,$missing_prereq\"\n+\t\tfi\n \tfi\n \n \tcase \"$to_skip\" in\n@@ -1175,6 +1185,7 @@ test_done () {\n \t\tfixed $test_fixed\n \t\tbroken $test_broken\n \t\tfailed $test_failure\n+\t\tmissing_prereq $test_missing_prereq\n \n \t\tEOF\n \tfi\n-- \n2.31.1\n\n"},{"id":"441420","messageId":"20211117090410.8013-3-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211117090410.8013-1-fs@gigacodes.de","subject":"[PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-17T09:04:10Z","receivedAt":"2021-11-17T09:04:33Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"In certain environments or for specific test scenarios we might expect a\nspecific prerequisite check to succeed. Therefore we would like to\ntrigger an error when running our tests if this is not the case.\n\nTo remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\nwhich can be set to a comma separated list of prereqs. If one of these\nprereq tests fail then the whole test run will abort.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/README                |  6 ++++++\n t/test-lib-functions.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 29f72354bf..18ce75976e 100644\n--- a/t/README\n+++ b/t/README\n@@ -466,6 +466,12 @@ explicitly providing repositories when accessing submodule objects is\n complete or needs to be abandoned for whatever reason (in which case the\n migrated codepaths still retain their performance benefits).\n \n+GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of\n+prereqs that are required to succeed. If a prereq in this list is triggered by\n+a test and then fails then the whole test run will abort. This can help to make\n+sure the expected tests are executed and not silently skipped when their\n+dependency breaks or is simply not present in a new environment.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex eef2262a36..2c8abf3420 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -680,6 +680,17 @@ test_have_prereq () {\n \t\t\t# Keep a list of missing prerequisites; restore\n \t\t\t# the negative marker if necessary.\n \t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n+\n+\t\t\t# Abort if this prereq was marked as required\n+\t\t\tif test -n $GIT_TEST_REQUIRE_PREREQ\n+\t\t\tthen\n+\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n+\t\t\t\t*,$prerequisite,*)\n+\t\t\t\t\terror \"required prereq $prerequisite failed\"\n+\t\t\t\t\t;;\n+\t\t\t\tesac\n+\t\t\tfi\n+\n \t\t\tif test -z \"$missing_prereq\"\n \t\t\tthen\n \t\t\t\tmissing_prereq=$prerequisite\n-- \n2.31.1\n\n"},{"id":"441674","messageId":"xmqqpmqxuj3b.fsf@gitster.g","threadId":"56924","inReplyTo":"20211117090410.8013-3-fs@gigacodes.de","subject":"Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-18T23:42:16Z","receivedAt":"2021-11-18T23:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n> +\t\t\t# Abort if this prereq was marked as required\n> +\t\t\tif test -n $GIT_TEST_REQUIRE_PREREQ\n\nIf GIT_TEST_REQUIRE_PREREQ is an empty string, this will ask\n\n\ttest -n\n\nand \"test\" will say \"yes\" (because \"-n\" is not an empty string).\n\nLet's surround it with a pair of double-quotes.\n\n> +\t\t\tthen\n> +\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n> +\t\t\t\t*,$prerequisite,*)\n> +\t\t\t\t\terror \"required prereq $prerequisite failed\"\n> +\t\t\t\t\t;;\n> +\t\t\t\tesac\n> +\t\t\tfi\n> +\n>  \t\t\tif test -z \"$missing_prereq\"\n>  \t\t\tthen\n>  \t\t\t\tmissing_prereq=$prerequisite\n"},{"id":"441708","messageId":"20211119090755.6noeyarhc3rpzwzx@fs","threadId":"56924","inReplyTo":"xmqqpmqxuj3b.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-19T09:07:55Z","receivedAt":"2021-11-19T09:07:59Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 18.11.2021 15:42, Junio C Hamano wrote:\n>Fabian Stelzer <fs@gigacodes.de> writes:\n>\n>> +\t\t\t# Abort if this prereq was marked as required\n>> +\t\t\tif test -n $GIT_TEST_REQUIRE_PREREQ\n>\n>If GIT_TEST_REQUIRE_PREREQ is an empty string, this will ask\n>\n>\ttest -n\n>\n>and \"test\" will say \"yes\" (because \"-n\" is not an empty string).\n>\n>Let's surround it with a pair of double-quotes.\n\nWill do.\n\nThanks\n\n>\n>> +\t\t\tthen\n>> +\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n>> +\t\t\t\t*,$prerequisite,*)\n>> +\t\t\t\t\terror \"required prereq $prerequisite failed\"\n>> +\t\t\t\t\t;;\n>> +\t\t\t\tesac\n>> +\t\t\tfi\n>> +\n>>  \t\t\tif test -z \"$missing_prereq\"\n>>  \t\t\tthen\n>>  \t\t\t\tmissing_prereq=$prerequisite\n"},{"id":"441715","messageId":"211119.865yso4a9y.gmgdl@evledraar.gmail.com","threadId":"56924","inReplyTo":"20211117090410.8013-3-fs@gigacodes.de","subject":"Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-19T11:13:43Z","receivedAt":"2021-11-19T12:09:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 17 2021, Fabian Stelzer wrote:\n\n> In certain environments or for specific test scenarios we might expect a\n> specific prerequisite check to succeed. Therefore we would like to\n> trigger an error when running our tests if this is not the case.\n\ntrigger an error but...\n\n> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n> which can be set to a comma separated list of prereqs. If one of these\n> prereq tests fail then the whole test run will abort.\n\n..here it's \"abort the whole test run\". If that's what you want use\nBAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use \"Bail out!\"\nsyntax on bad SANITIZE=leak use, 2021-10-14)\n\n> +GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of\n> +prereqs that are required to succeed. If a prereq in this list is triggered by\n> +a test and then fails then the whole test run will abort. This can help to make\n> +sure the expected tests are executed and not silently skipped when their\n> +dependency breaks or is simply not present in a new environment.\n> +\n>  Naming Tests\n>  ------------\n\nFor other things we specify via lists such as GIT_SKIP_TESTS that's\nspace-separated, but here it's comma-separated, isn't that just a leaky\nabstraction in this case? I.e. this is exposing a previously\ninternal-only implementation detail of the prereq code.\n\nIt's less painful in shellscript if anything like this supports\nspace-separated parameters, as you can interpolate them more easily in\nany wrapper script without using \"tr\" or the like...\n"},{"id":"441728","messageId":"20211119134826.i66sufxzxotadxlb@fs","threadId":"56924","inReplyTo":"211119.865yso4a9y.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-19T13:48:26Z","receivedAt":"2021-11-19T13:48:33Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:\n>\n>On Wed, Nov 17 2021, Fabian Stelzer wrote:\n>\n>> In certain environments or for specific test scenarios we might expect a\n>> specific prerequisite check to succeed. Therefore we would like to\n>> trigger an error when running our tests if this is not the case.\n>\n>trigger an error but...\n>\n>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n>> which can be set to a comma separated list of prereqs. If one of these\n>> prereq tests fail then the whole test run will abort.\n>\n>..here it's \"abort the whole test run\". If that's what you want use\n>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use \"Bail out!\"\n>syntax on bad SANITIZE=leak use, 2021-10-14)\n>\n\nok, thanks. BAIL_OUT seems better. i grepped through the tests and\ndidn't find anything like it, so i used error.\n\n>> +GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of\n>> +prereqs that are required to succeed. If a prereq in this list is triggered by\n>> +a test and then fails then the whole test run will abort. This can help to make\n>> +sure the expected tests are executed and not silently skipped when their\n>> +dependency breaks or is simply not present in a new environment.\n>> +\n>>  Naming Tests\n>>  ------------\n>\n>For other things we specify via lists such as GIT_SKIP_TESTS that's\n>space-separated, but here it's comma-separated, isn't that just a leaky\n>abstraction in this case? I.e. this is exposing a previously\n>internal-only implementation detail of the prereq code.\n>\n>It's less painful in shellscript if anything like this supports\n>space-separated parameters, as you can interpolate them more easily in\n>any wrapper script without using \"tr\" or the like...\n\nOk. easy enough to change. Should the listing of missing prereq at the\nend of a test run be space separated as well? (maybe helps with word wrapping)\n\n"},{"id":"441732","messageId":"20211119140953.cgdppgv3f64hqbdx@fs","threadId":"56924","inReplyTo":"211119.865yso4a9y.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-19T14:09:53Z","receivedAt":"2021-11-19T14:09:57Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:\n>\n>On Wed, Nov 17 2021, Fabian Stelzer wrote:\n>\n>> In certain environments or for specific test scenarios we might expect a\n>> specific prerequisite check to succeed. Therefore we would like to\n>> trigger an error when running our tests if this is not the case.\n>\n>trigger an error but...\n>\n>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n>> which can be set to a comma separated list of prereqs. If one of these\n>> prereq tests fail then the whole test run will abort.\n>\n>..here it's \"abort the whole test run\". If that's what you want use\n>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use \"Bail out!\"\n>syntax on bad SANITIZE=leak use, 2021-10-14)\n>\n\nHm, while testing this change i noticed another problem that i really\nhave no idea how to fix.\nWhen a test uses test_have_prereq then the error/BAIL_OUT message will only be printed\nwhen run with '-v'. This is not the case when the prereq is specified\nin the test header. The test run will abort, but no error will be\nprinted which can be quite confusing :/\nI guess this has something to do with how tests are run in subshells and\ntheir outputs only printed with -v. Maybe there should be some kind of\noverride for BAIL_OUT at least? Not sure if/how this could be done.\n"},{"id":"441733","messageId":"211119.86sfvs2p9w.gmgdl@evledraar.gmail.com","threadId":"56924","inReplyTo":"20211119140953.cgdppgv3f64hqbdx@fs","subject":"Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-19T14:26:00Z","receivedAt":"2021-11-19T14:28:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 19 2021, Fabian Stelzer wrote:\n\n> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>On Wed, Nov 17 2021, Fabian Stelzer wrote:\n>>\n>>> In certain environments or for specific test scenarios we might expect a\n>>> specific prerequisite check to succeed. Therefore we would like to\n>>> trigger an error when running our tests if this is not the case.\n>>\n>>trigger an error but...\n>>\n>>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n>>> which can be set to a comma separated list of prereqs. If one of these\n>>> prereq tests fail then the whole test run will abort.\n>>\n>>..here it's \"abort the whole test run\". If that's what you want use\n>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use \"Bail out!\"\n>>syntax on bad SANITIZE=leak use, 2021-10-14)\n>>\n>\n> Hm, while testing this change i noticed another problem that i really\n> have no idea how to fix.\n> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed\n> when run with '-v'. This is not the case when the prereq is specified\n> in the test header. The test run will abort, but no error will be\n> printed which can be quite confusing :/\n> I guess this has something to do with how tests are run in subshells and\n> their outputs only printed with -v. Maybe there should be some kind of\n> override for BAIL_OUT at least? Not sure if/how this could be done.\n\nIt has to do with how we juggle file descriptors around, see test_eval_\nin test-lib.sh.\n\nSo the \"real\" stdout is fd 5, not 1 when you're in a prereq.\n\nJust:\n\n    BAIL_OUT \"bad\" >&5\n\nWill work, maybe it's a good idea to have:\n\n\tBAIL_OUT_PREREQ () {\n\t\tBAIL_OUT $@ >&5\n\t}\n\nSorry, I forgot about that caveat when suggesting it.\n"},{"id":"441748","messageId":"20211119154036.5n5kpecgnptzkaqn@fs","threadId":"56924","inReplyTo":"211119.86sfvs2p9w.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-19T15:40:36Z","receivedAt":"2021-11-19T15:40:46Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 19.11.2021 15:26, Ævar Arnfjörð Bjarmason wrote:\n>\n>On Fri, Nov 19 2021, Fabian Stelzer wrote:\n>\n>> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>>On Wed, Nov 17 2021, Fabian Stelzer wrote:\n>>>\n>>>> In certain environments or for specific test scenarios we might expect a\n>>>> specific prerequisite check to succeed. Therefore we would like to\n>>>> trigger an error when running our tests if this is not the case.\n>>>\n>>>trigger an error but...\n>>>\n>>>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n>>>> which can be set to a comma separated list of prereqs. If one of these\n>>>> prereq tests fail then the whole test run will abort.\n>>>\n>>>..here it's \"abort the whole test run\". If that's what you want use\n>>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use \"Bail out!\"\n>>>syntax on bad SANITIZE=leak use, 2021-10-14)\n>>>\n>>\n>> Hm, while testing this change i noticed another problem that i really\n>> have no idea how to fix.\n>> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed\n>> when run with '-v'. This is not the case when the prereq is specified\n>> in the test header. The test run will abort, but no error will be\n>> printed which can be quite confusing :/\n>> I guess this has something to do with how tests are run in subshells and\n>> their outputs only printed with -v. Maybe there should be some kind of\n>> override for BAIL_OUT at least? Not sure if/how this could be done.\n>\n>It has to do with how we juggle file descriptors around, see test_eval_\n>in test-lib.sh.\n>\n>So the \"real\" stdout is fd 5, not 1 when you're in a prereq.\n>\n>Just:\n>\n>    BAIL_OUT \"bad\" >&5\n>\n>Will work, maybe it's a good idea to have:\n>\n>\tBAIL_OUT_PREREQ () {\n>\t\tBAIL_OUT $@ >&5\n>\t}\n>\n>Sorry, I forgot about that caveat when suggesting it.\n\nHm. Any reason to not do this in BAIL_OUT itself?  As far as i can see\nthe setup of the additional fd's would only need to move up a few lines.\n"},{"id":"441756","messageId":"211119.86o86g2j4h.gmgdl@evledraar.gmail.com","threadId":"56924","inReplyTo":"20211119154036.5n5kpecgnptzkaqn@fs","subject":"Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-19T16:37:13Z","receivedAt":"2021-11-19T16:41:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 19 2021, Fabian Stelzer wrote:\n\n> On 19.11.2021 15:26, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>On Fri, Nov 19 2021, Fabian Stelzer wrote:\n>>\n>>> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:\n>>>>\n>>>>On Wed, Nov 17 2021, Fabian Stelzer wrote:\n>>>>\n>>>>> In certain environments or for specific test scenarios we might expect a\n>>>>> specific prerequisite check to succeed. Therefore we would like to\n>>>>> trigger an error when running our tests if this is not the case.\n>>>>\n>>>>trigger an error but...\n>>>>\n>>>>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n>>>>> which can be set to a comma separated list of prereqs. If one of these\n>>>>> prereq tests fail then the whole test run will abort.\n>>>>\n>>>>..here it's \"abort the whole test run\". If that's what you want use\n>>>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use \"Bail out!\"\n>>>>syntax on bad SANITIZE=leak use, 2021-10-14)\n>>>>\n>>>\n>>> Hm, while testing this change i noticed another problem that i really\n>>> have no idea how to fix.\n>>> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed\n>>> when run with '-v'. This is not the case when the prereq is specified\n>>> in the test header. The test run will abort, but no error will be\n>>> printed which can be quite confusing :/\n>>> I guess this has something to do with how tests are run in subshells and\n>>> their outputs only printed with -v. Maybe there should be some kind of\n>>> override for BAIL_OUT at least? Not sure if/how this could be done.\n>>\n>>It has to do with how we juggle file descriptors around, see test_eval_\n>>in test-lib.sh.\n>>\n>>So the \"real\" stdout is fd 5, not 1 when you're in a prereq.\n>>\n>>Just:\n>>\n>>    BAIL_OUT \"bad\" >&5\n>>\n>>Will work, maybe it's a good idea to have:\n>>\n>>\tBAIL_OUT_PREREQ () {\n>>\t\tBAIL_OUT $@ >&5\n>>\t}\n>>\n>>Sorry, I forgot about that caveat when suggesting it.\n>\n> Hm. Any reason to not do this in BAIL_OUT itself?  As far as i can see\n> the setup of the additional fd's would only need to move up a few lines.\n\nThat does look like a better solution, I've tried it just now locally &\nit works for me. Perhaps there's some subtlety I'm missing, but that\nshould Just Work.\n\nThis is by far not the first time I've poked at something in test-lib.sh\nonly to discover that its pattern of doing setup A, setup C, setup B\netc. caused a problem solved by moving B & C around :(\n\nIt could really do with a change to move everything it's now doing to\nfunctions, which we'd then call, so what setup we do in what order would\nfit on a single screen, but that's a much larger change...\n"},{"id":"441851","messageId":"20211120150401.254408-1-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211117090410.8013-3-fs@gigacodes.de","subject":"[PATCH v3 0/3] test-lib: improve missing prereq handling","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-20T15:03:58Z","receivedAt":"2021-11-20T15:04:20Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"The ssh signing feature was breaking tests when the broken openssh-8.7\nwas used. We have now fixed that by checking for this exact case in the\nGPGSSH prereq and I will improve that check further in a future patch.\nHowever we are now in a situation where a broken openssh in the future\nwill result in successfull tests but not a working git build afterwards\n(either not compiling in the expected feature or like in the ssh case\nruntime failures) resulting in a false sense of security in the tests.\nThis patches try to improve this situation by showing which prereqs\nfailed in the test summary and by adding an environment variable to\nenforce certain prereqs to succeed or abort the test otherwise.\n\nSee also:\nhttps://public-inbox.org/git/xmqqv916wh7t.fsf@gitster.g/\n\nchanges sinve v2:\n - use a space separated list for GIT_TES_REQUIRED_PREREQ like we do for\n   GIT_SKIP_TESTS\n - use BAIL_OUT() insted of just error()\n - make BAIL_OUT() print errors even when used within prereq context\n\nchanges since v1:\n - use \\012 instead of \\n for possible portability reasons\n - fix typo in commit msg\n\nFabian Stelzer (3):\n  test-lib: show missing prereq summary\n  test-lib: introduce required prereq for test runs\n  test-lib: make BAIL_OUT() work in tests and prereq\n\n t/README                |  6 ++++++\n t/aggregate-results.sh  | 17 +++++++++++++++++\n t/test-lib-functions.sh | 11 +++++++++++\n t/test-lib.sh           | 21 +++++++++++++++++----\n 4 files changed, 51 insertions(+), 4 deletions(-)\n\nRange-diff against v2:\n1:  69e77cd854 ! 1:  35c92671e5 test-lib: show missing prereq summary\n    @@ t/aggregate-results.sh: do\n     +\t\ttr -s \",\" \"\\n\" |\n     +\t\tgrep -v '^$' |\n     +\t\tsort -u |\n    -+\t\tpaste -s -d ',')\n    -+\tif test -n $unique_missing_prereq\n    ++\t\tpaste -s -d ' ')\n    ++\tif test -n \"$unique_missing_prereq\"\n     +\tthen\n     +\t\tprintf \"\\nmissing prereq: $unique_missing_prereq\\n\\n\"\n     +\tfi\n2:  12bd18c5ce ! 2:  d6a53f0980 test-lib: introduce required prereq for test runs\n    @@ Commit message\n         test-lib: introduce required prereq for test runs\n     \n         In certain environments or for specific test scenarios we might expect a\n    -    specific prerequisite check to be succeed. Therefore we would like to\n    -    trigger an error when running our tests if this is not the case.\n    +    specific prerequisite check to succeed. Therefore we would like to abort\n    +    running our tests if this is not the case.\n     \n         To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\n    -    which can be set to a comma separated list of prereqs. If one of these\n    +    which can be set to a space separated list of prereqs. If one of these\n         prereq tests fail then the whole test run will abort.\n     \n         Signed-off-by: Fabian Stelzer <fs@gigacodes.de>\n    @@ t/README: explicitly providing repositories when accessing submodule objects is\n      complete or needs to be abandoned for whatever reason (in which case the\n      migrated codepaths still retain their performance benefits).\n      \n    -+GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of\n    ++GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a space speparated list of\n     +prereqs that are required to succeed. If a prereq in this list is triggered by\n     +a test and then fails then the whole test run will abort. This can help to make\n     +sure the expected tests are executed and not silently skipped when their\n    @@ t/test-lib-functions.sh: test_have_prereq () {\n      \t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n     +\n     +\t\t\t# Abort if this prereq was marked as required\n    -+\t\t\tif test -n $GIT_TEST_REQUIRE_PREREQ\n    ++\t\t\tif test -n \"$GIT_TEST_REQUIRE_PREREQ\"\n     +\t\t\tthen\n    -+\t\t\t\tcase \",$GIT_TEST_REQUIRE_PREREQ,\" in\n    -+\t\t\t\t*,$prerequisite,*)\n    -+\t\t\t\t\terror \"required prereq $prerequisite failed\"\n    ++\t\t\t\tcase \" $GIT_TEST_REQUIRE_PREREQ \" in\n    ++\t\t\t\t*\" $prerequisite \"*)\n    ++\t\t\t\t\tBAIL_OUT \"required prereq $prerequisite failed\"\n     +\t\t\t\t\t;;\n     +\t\t\t\tesac\n     +\t\t\tfi\n-:  ---------- > 3:  de21c484d6 test-lib: make BAIL_OUT() work in tests and prereq\n\nbase-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc\n-- \n2.31.1\n\n"},{"id":"441852","messageId":"20211120150401.254408-2-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211120150401.254408-1-fs@gigacodes.de","subject":"[PATCH v3 1/3] test-lib: show missing prereq summary","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-20T15:03:59Z","receivedAt":"2021-11-20T15:04:23Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"When running the full test suite many tests can be skipped because of\nmissing prerequisites. It not easy right now to get an overview of which\nones are missing.\nWhen switching to a new machine or environment some libraries and tools\nmight be missing or maybe a dependency broke completely. In this case\nthe tests would indicate nothing since all dependant tests are simply\nskipped. This could hide broken behaviour or missing features in the\nbuild. Therefore this patch summarizes the missing prereqs at the end of\nthe test run making it easier to spot such cases.\n\n - Add failed prereqs to the test results.\n - Aggregate and then show them with the totals.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/aggregate-results.sh | 17 +++++++++++++++++\n t/test-lib.sh          | 11 +++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\nindex 7913e206ed..7f2b83bdc8 100755\n--- a/t/aggregate-results.sh\n+++ b/t/aggregate-results.sh\n@@ -6,6 +6,7 @@ success=0\n failed=0\n broken=0\n total=0\n+missing_prereq=\n \n while read file\n do\n@@ -30,10 +31,26 @@ do\n \t\t\tbroken=$(($broken + $value)) ;;\n \t\ttotal)\n \t\t\ttotal=$(($total + $value)) ;;\n+\t\tmissing_prereq)\n+\t\t\tmissing_prereq=\"$missing_prereq,$value\" ;;\n \t\tesac\n \tdone <\"$file\"\n done\n \n+if test -n \"$missing_prereq\"\n+then\n+\tunique_missing_prereq=$(\n+\t\techo $missing_prereq |\n+\t\ttr -s \",\" \"\\n\" |\n+\t\tgrep -v '^$' |\n+\t\tsort -u |\n+\t\tpaste -s -d ' ')\n+\tif test -n \"$unique_missing_prereq\"\n+\tthen\n+\t\tprintf \"\\nmissing prereq: $unique_missing_prereq\\n\\n\"\n+\tfi\n+fi\n+\n if test -n \"$failed_tests\"\n then\n \tprintf \"\\nfailed test(s):$failed_tests\\n\\n\"\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2679a7596a..f61da562f6 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -669,6 +669,8 @@ test_fixed=0\n test_broken=0\n test_success=0\n \n+test_missing_prereq=\n+\n test_external_has_tap=0\n \n die () {\n@@ -1069,6 +1071,14 @@ test_skip () {\n \t\t\tof_prereq=\" of $test_prereq\"\n \t\tfi\n \t\tskipped_reason=\"missing $missing_prereq${of_prereq}\"\n+\n+\t\t# Keep a list of all the missing prereq for result aggregation\n+\t\tif test -z \"$missing_prereq\"\n+\t\tthen\n+\t\t\ttest_missing_prereq=$missing_prereq\n+\t\telse\n+\t\t\ttest_missing_prereq=\"$test_missing_prereq,$missing_prereq\"\n+\t\tfi\n \tfi\n \n \tcase \"$to_skip\" in\n@@ -1175,6 +1185,7 @@ test_done () {\n \t\tfixed $test_fixed\n \t\tbroken $test_broken\n \t\tfailed $test_failure\n+\t\tmissing_prereq $test_missing_prereq\n \n \t\tEOF\n \tfi\n-- \n2.31.1\n\n"},{"id":"441853","messageId":"20211120150401.254408-3-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211120150401.254408-1-fs@gigacodes.de","subject":"[PATCH v3 2/3] test-lib: introduce required prereq for test runs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-20T15:04:00Z","receivedAt":"2021-11-20T15:04:26Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"In certain environments or for specific test scenarios we might expect a\nspecific prerequisite check to succeed. Therefore we would like to abort\nrunning our tests if this is not the case.\n\nTo remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\nwhich can be set to a space separated list of prereqs. If one of these\nprereq tests fail then the whole test run will abort.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/README                |  6 ++++++\n t/test-lib-functions.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 29f72354bf..2353a4c5e1 100644\n--- a/t/README\n+++ b/t/README\n@@ -466,6 +466,12 @@ explicitly providing repositories when accessing submodule objects is\n complete or needs to be abandoned for whatever reason (in which case the\n migrated codepaths still retain their performance benefits).\n \n+GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a space speparated list of\n+prereqs that are required to succeed. If a prereq in this list is triggered by\n+a test and then fails then the whole test run will abort. This can help to make\n+sure the expected tests are executed and not silently skipped when their\n+dependency breaks or is simply not present in a new environment.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex eef2262a36..389153e591 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -680,6 +680,17 @@ test_have_prereq () {\n \t\t\t# Keep a list of missing prerequisites; restore\n \t\t\t# the negative marker if necessary.\n \t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n+\n+\t\t\t# Abort if this prereq was marked as required\n+\t\t\tif test -n \"$GIT_TEST_REQUIRE_PREREQ\"\n+\t\t\tthen\n+\t\t\t\tcase \" $GIT_TEST_REQUIRE_PREREQ \" in\n+\t\t\t\t*\" $prerequisite \"*)\n+\t\t\t\t\tBAIL_OUT \"required prereq $prerequisite failed\"\n+\t\t\t\t\t;;\n+\t\t\t\tesac\n+\t\t\tfi\n+\n \t\t\tif test -z \"$missing_prereq\"\n \t\t\tthen\n \t\t\t\tmissing_prereq=$prerequisite\n-- \n2.31.1\n\n"},{"id":"441854","messageId":"20211120150401.254408-4-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211120150401.254408-1-fs@gigacodes.de","subject":"[PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-20T15:04:01Z","receivedAt":"2021-11-20T15:04:28Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"BAIL_OUT() is meant to abort the whole test run and print a message with\na standard prefix that can be parsed to stdout. Since for every test the\nnormal fd`s are redirected in test_eval_ this output would not be seen\nwhen used within the context of a test or prereq like we do in\ntest_have_prereq(). To make this function work in these contexts we move\nthe setup of the fd aliases a few lines up before the first use of\nBAIL_OUT() and then have this function always print to the alias.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/test-lib.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex f61da562f6..96a09a26a1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -589,6 +589,11 @@ USER_TERM=\"$TERM\"\n TERM=dumb\n export TERM USER_TERM\n \n+# Set up additional fds so we can control single test i/o\n+exec 5>&1\n+exec 6<&0\n+exec 7>&2\n+\n _error_exit () {\n \tfinalize_junit_xml\n \tGIT_EXIT_OK=t\n@@ -612,7 +617,7 @@ BAIL_OUT () {\n \tlocal bail_out=\"Bail out! \"\n \tlocal message=\"$1\"\n \n-\tsay_color error $bail_out \"$message\"\n+\tsay_color error $bail_out \"$message\" >&5\n \t_error_exit\n }\n \n@@ -637,9 +642,6 @@ then\n \texit 0\n fi\n \n-exec 5>&1\n-exec 6<&0\n-exec 7>&2\n if test \"$verbose_log\" = \"t\"\n then\n \texec 3>>\"$GIT_TEST_TEE_OUTPUT_FILE\" 4>&3\n-- \n2.31.1\n\n"},{"id":"441929","messageId":"211122.86y25gz9q7.gmgdl@evledraar.gmail.com","threadId":"56924","inReplyTo":"20211120150401.254408-4-fs@gigacodes.de","subject":"Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-22T11:52:42Z","receivedAt":"2021-11-22T11:54:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Nov 20 2021, Fabian Stelzer wrote:\n\n> BAIL_OUT() is meant to abort the whole test run and print a message with\n> a standard prefix that can be parsed to stdout. Since for every test the\n> normal fd`s are redirected in test_eval_ this output would not be seen\n> when used within the context of a test or prereq like we do in\n> test_have_prereq(). To make this function work in these contexts we move\n> the setup of the fd aliases a few lines up before the first use of\n> BAIL_OUT() and then have this function always print to the alias.\n>\n> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>\n> ---\n>  t/test-lib.sh | 10 ++++++----\n>  1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index f61da562f6..96a09a26a1 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -589,6 +589,11 @@ USER_TERM=\"$TERM\"\n>  TERM=dumb\n>  export TERM USER_TERM\n>  \n> +# Set up additional fds so we can control single test i/o\n> +exec 5>&1\n> +exec 6<&0\n> +exec 7>&2\n> +\n>  _error_exit () {\n>  \tfinalize_junit_xml\n>  \tGIT_EXIT_OK=t\n> @@ -612,7 +617,7 @@ BAIL_OUT () {\n>  \tlocal bail_out=\"Bail out! \"\n>  \tlocal message=\"$1\"\n>  \n> -\tsay_color error $bail_out \"$message\"\n> +\tsay_color error $bail_out \"$message\" >&5\n>  \t_error_exit\n>  }\n>  \n> @@ -637,9 +642,6 @@ then\n>  \texit 0\n>  fi\n>  \n> -exec 5>&1\n> -exec 6<&0\n> -exec 7>&2\n\nThis doesn't break (I think) with your change here because you only\nmanipulate >&5, but I think the post-image would be lot clearer if...\n\n>  if test \"$verbose_log\" = \"t\"\n>  then\n>  \texec 3>>\"$GIT_TEST_TEE_OUTPUT_FILE\" 4>&3\n\n...this bit of code were moved up along with the \"exec\". They're\ncurrently intentionally snuggled together as we conditionally set the\n3rd and 4th fd depending on verbose/tee settings right after the setup\nof 5/6/7, keeping them grouped in the post-image makes more sense than\nsplitting them up here.\n\n"},{"id":"441971","messageId":"xmqqh7c4i0jh.fsf@gitster.g","threadId":"56924","inReplyTo":"211122.86y25gz9q7.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T17:05:06Z","receivedAt":"2021-11-22T17:05:12Z","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>> -exec 5>&1\n>> -exec 6<&0\n>> -exec 7>&2\n>\n> This doesn't break (I think) with your change here because you only\n> manipulate >&5, but I think the post-image would be lot clearer if...\n>\n>>  if test \"$verbose_log\" = \"t\"\n>>  then\n>>  \texec 3>>\"$GIT_TEST_TEE_OUTPUT_FILE\" 4>&3\n>\n> ...this bit of code were moved up along with the \"exec\". They're\n> currently intentionally snuggled together as we conditionally set the\n> 3rd and 4th fd depending on verbose/tee settings right after the setup\n> of 5/6/7, keeping them grouped in the post-image makes more sense than\n> splitting them up here.\n\nI actually have to wonder if the handling of 3 and 4 should be moved\ndown, not up like you suggest, so that they are grouped together\nwith the maybe_teardown_verbose and the maybe_setup_verbose helper\nfunctions.  The lines we see here are the file descriptors that are\nalways redirected, which is a bit different.  Raising all of them up\nto group them together is also fine.\n\nIn any case, the comment in front of the block of exec wants to\nbecome a bit more detailed than just \"# Set up additional fds\", with\nan explanation about which FD is used for what.\n\nAnd any change that involves handling of FD #4 probably wants to\nfurther include the BASH_XTRACEFD setting.\n\nThanks.\n"},{"id":"442355","messageId":"20211126095509.weeknmg4p6sx7bdn@fs","threadId":"56924","inReplyTo":"xmqqh7c4i0jh.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-26T09:55:09Z","receivedAt":"2021-11-26T09:57:14Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 22.11.2021 09:05, Junio C Hamano wrote:\n>Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> -exec 5>&1\n>>> -exec 6<&0\n>>> -exec 7>&2\n>>\n>> This doesn't break (I think) with your change here because you only\n>> manipulate >&5, but I think the post-image would be lot clearer if...\n>>\n>>>  if test \"$verbose_log\" = \"t\"\n>>>  then\n>>>  \texec 3>>\"$GIT_TEST_TEE_OUTPUT_FILE\" 4>&3\n>>\n>> ...this bit of code were moved up along with the \"exec\". They're\n>> currently intentionally snuggled together as we conditionally set the\n>> 3rd and 4th fd depending on verbose/tee settings right after the setup\n>> of 5/6/7, keeping them grouped in the post-image makes more sense than\n>> splitting them up here.\n>\n>I actually have to wonder if the handling of 3 and 4 should be moved\n>down, not up like you suggest, so that they are grouped together\n>with the maybe_teardown_verbose and the maybe_setup_verbose helper\n>functions.  The lines we see here are the file descriptors that are\n>always redirected, which is a bit different.  Raising all of them up\n>to group them together is also fine.\n\nSince there is lots of other things happening in between i'd rather\nleave 3 & 4 where they are for now. I'm having a hard time grasping what\ntest-lib.sh does when / where since there is lots of intermingled\nfunction definition / executed code. It could probably benefit from a\nnew include to move functions to (and wrap some code into new ones). At\nthe moment i'm a bit swamped with other work though :/\n\n>\n>In any case, the comment in front of the block of exec wants to\n>become a bit more detailed than just \"# Set up additional fds\", with\n>an explanation about which FD is used for what.\n>\n\nHow about:\n\n# Set up additional fds to allow i/o with the surrounding test\n# harness when redirecting individual test i/o in test_eval_\n# fd 5 -> stdout\n# fd 6 <- stdin\n# fd 7 -> stderr\n\nexec 5>&1\nexec 6<&0\nexec 7>&2\n"},{"id":"442374","messageId":"xmqqy25a636c.fsf@gitster.g","threadId":"56924","inReplyTo":"20211126095509.weeknmg4p6sx7bdn@fs","subject":"Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-26T21:02:35Z","receivedAt":"2021-11-26T21:04:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n>>In any case, the comment in front of the block of exec wants to\n>>become a bit more detailed than just \"# Set up additional fds\", with\n>>an explanation about which FD is used for what.\n>>\n>\n> How about:\n>\n> # Set up additional fds to allow i/o with the surrounding test\n> # harness when redirecting individual test i/o in test_eval_\n\nThis does not quite say how \"setting up additional fds\" helpss the\ntest harness, though.  And this ...\n\n> # fd 5 -> stdout\n> # fd 6 <- stdin\n> # fd 7 -> stderr\n\n... is literal translation of what is written below, without adding\nany new information.\n\n> exec 5>&1\n> exec 6<&0\n> exec 7>&2\n\nI was expecting something along the lines of ...\n\n# What is written by tests to their FD #1 and #2 are sent to\n# different places depending on the test mode (e.g. /dev/null in\n# non-verbose mode, piped to tee with --tee option, etc.)  Original\n# FD #1 and #2 are saved away to #5 and #7, so that test framework\n# can use them to send the output to these low FDs before the\n# mode-specific redirection.\n\n... but this only talks about the output side.  The final version\nneeds to mention the input side, too.\n\nThanks.\n\n\n\n\n"},{"id":"442400","messageId":"20211127124733.ulicqyiudur3s5h4@fs","threadId":"56924","inReplyTo":"xmqqy25a636c.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-27T12:47:33Z","receivedAt":"2021-11-27T12:49:38Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 26.11.2021 13:02, Junio C Hamano wrote:\n>Fabian Stelzer <fs@gigacodes.de> writes:\n>\n>>>In any case, the comment in front of the block of exec wants to\n>>>become a bit more detailed than just \"# Set up additional fds\", with\n>>>an explanation about which FD is used for what.\n>>>\n>>\n>> How about:\n>>\n>> # Set up additional fds to allow i/o with the surrounding test\n>> # harness when redirecting individual test i/o in test_eval_\n>\n>This does not quite say how \"setting up additional fds\" helpss the\n>test harness, though.  And this ...\n>\n>> # fd 5 -> stdout\n>> # fd 6 <- stdin\n>> # fd 7 -> stderr\n>\n>... is literal translation of what is written below, without adding\n>any new information.\n>\n>> exec 5>&1\n>> exec 6<&0\n>> exec 7>&2\n>\n>I was expecting something along the lines of ...\n>\n># What is written by tests to their FD #1 and #2 are sent to\n># different places depending on the test mode (e.g. /dev/null in\n># non-verbose mode, piped to tee with --tee option, etc.)  Original\n># FD #1 and #2 are saved away to #5 and #7, so that test framework\n># can use them to send the output to these low FDs before the\n># mode-specific redirection.\n>\n>... but this only talks about the output side.  The final version\n>needs to mention the input side, too.\n>\n\nI like to use the term stdin/err/out since that is what i would grep for\nwhen trying to find out more about the test i/o behaviour.\n\nI understand that you would like to give exaxples what the fd aliases\nare used for and I think thats a good idea.  But I'm having a bit of a\nhard time understanding the use of the stdin alias. I did not find a\nsingle use in the test suite - but my grep might not have been complete.\nOr if it's just there for the sake of completeness.\n\nAs far as i understand the test framework runs the individual tests in\ntest_eval_ redirecting its stdout/err to fd #3 & #4 (with some xtracefd\nlogic for -x) and passing /dev/null as stdin to the test.  What would FD\n#6 actually be used for then? A test lib function being called from\nwithin a test that expects user input?\n"},{"id":"442427","messageId":"xmqqo8634zrz.fsf@gitster.g","threadId":"56924","inReplyTo":"20211127124733.ulicqyiudur3s5h4@fs","subject":"Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-28T23:38:08Z","receivedAt":"2021-11-28T23:40:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n>>I was expecting something along the lines of ...\n>>\n>># What is written by tests to their FD #1 and #2 are sent to\n>># different places depending on the test mode (e.g. /dev/null in\n>># non-verbose mode, piped to tee with --tee option, etc.)  Original\n>># FD #1 and #2 are saved away to #5 and #7, so that test framework\n>># can use them to send the output to these low FDs before the\n>># mode-specific redirection.\n>>\n>>... but this only talks about the output side.  The final version\n>>needs to mention the input side, too.\n>>\n>\n> I like to use the term stdin/err/out since that is what i would grep for\n> when trying to find out more about the test i/o behaviour.\n\nI do not mind phrasing \"original FD #1\" as \"original standard\noutput\" at all.  I just wanted to make sure it is clear to readers\nwhose FD #1 and FD #5 we are talking about. In other words, the\nreaders should get a clear understanding of where they are writing\nto, when the code they write in test_expect_success block outputs to\nFD #1, and what the code needs to do if it wants to always show\nsomething to the original standard output stream.\n"},{"id":"442666","messageId":"20211130143821.7dz5jj2z2x2q2ytn@fs","threadId":"56924","inReplyTo":"xmqqo8634zrz.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-11-30T14:38:21Z","receivedAt":"2021-11-30T14:38:27Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 28.11.2021 15:38, Junio C Hamano wrote:\n>Fabian Stelzer <fs@gigacodes.de> writes:\n>\n>>>I was expecting something along the lines of ...\n>>>\n>>># What is written by tests to their FD #1 and #2 are sent to\n>>># different places depending on the test mode (e.g. /dev/null in\n>>># non-verbose mode, piped to tee with --tee option, etc.)  Original\n>>># FD #1 and #2 are saved away to #5 and #7, so that test framework\n>>># can use them to send the output to these low FDs before the\n>>># mode-specific redirection.\n>>>\n>>>... but this only talks about the output side.  The final version\n>>>needs to mention the input side, too.\n>>>\n>>\n>> I like to use the term stdin/err/out since that is what i would grep for\n>> when trying to find out more about the test i/o behaviour.\n>\n>I do not mind phrasing \"original FD #1\" as \"original standard\n>output\" at all.  I just wanted to make sure it is clear to readers\n>whose FD #1 and FD #5 we are talking about. In other words, the\n>readers should get a clear understanding of where they are writing\n>to, when the code they write in test_expect_success block outputs to\n>FD #1, and what the code needs to do if it wants to always show\n>something to the original standard output stream.\n\nThe current version in my branch is now:\n\nWhat is written by tests to stdout and stderr is sent so different places\ndepending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee\nwith --tee option, etc.). We save the original stdin to FD #6 and stdout and\nstderr to #5 and #7, so that the test framework can use them (e.g. for\nprinting errors within the test framework) independently of the test mode.\n\nwhich I think should make this sufficiently clear.\nI'm wondering now though if we should write to #7 instead of #5 in \nBAIL_OUT(). The current use in test-lib/test-lib-functions seems a bit \ninconsistent.\n\nFor example:\nerror >&7 \"bug in the test script: $*\"\necho >&7 \"test_must_fail: only 'git' is allowed: $*\"\n\nbut:\necho >&5 \"FATAL: Cannot prepare test area\"\necho >&5 \"FATAL: Unexpected exit with code $code\"\n\nSometimes these errors result in immediate exit 1, but not always.\n\nI'm not sure if the TAP framework that BAIL_OUT() references expects the \nbail out error on a specific fd.\n"},{"id":"442670","messageId":"211130.86wnkpd6ou.gmgdl@evledraar.gmail.com","threadId":"56924","inReplyTo":"20211130143821.7dz5jj2z2x2q2ytn@fs","subject":"Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-30T14:59:27Z","receivedAt":"2021-11-30T15:14:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 30 2021, Fabian Stelzer wrote:\n\n> On 28.11.2021 15:38, Junio C Hamano wrote:\n>>Fabian Stelzer <fs@gigacodes.de> writes:\n>>\n>>>>I was expecting something along the lines of ...\n>>>>\n>>>># What is written by tests to their FD #1 and #2 are sent to\n>>>># different places depending on the test mode (e.g. /dev/null in\n>>>># non-verbose mode, piped to tee with --tee option, etc.)  Original\n>>>># FD #1 and #2 are saved away to #5 and #7, so that test framework\n>>>># can use them to send the output to these low FDs before the\n>>>># mode-specific redirection.\n>>>>\n>>>>... but this only talks about the output side.  The final version\n>>>>needs to mention the input side, too.\n>>>>\n>>>\n>>> I like to use the term stdin/err/out since that is what i would grep for\n>>> when trying to find out more about the test i/o behaviour.\n>>\n>>I do not mind phrasing \"original FD #1\" as \"original standard\n>>output\" at all.  I just wanted to make sure it is clear to readers\n>>whose FD #1 and FD #5 we are talking about. In other words, the\n>>readers should get a clear understanding of where they are writing\n>>to, when the code they write in test_expect_success block outputs to\n>>FD #1, and what the code needs to do if it wants to always show\n>>something to the original standard output stream.\n>\n> The current version in my branch is now:\n>\n> What is written by tests to stdout and stderr is sent so different places\n> depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee\n> with --tee option, etc.). We save the original stdin to FD #6 and stdout and\n> stderr to #5 and #7, so that the test framework can use them (e.g. for\n> printing errors within the test framework) independently of the test mode.\n>\n> which I think should make this sufficiently clear.\n> I'm wondering now though if we should write to #7 instead of #5 in\n> BAIL_OUT(). The current use in test-lib/test-lib-functions seems a bit \n> inconsistent.\n>\n> For example:\n> error >&7 \"bug in the test script: $*\"\n> echo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n>\n> but:\n> echo >&5 \"FATAL: Cannot prepare test area\"\n> echo >&5 \"FATAL: Unexpected exit with code $code\"\n>\n> Sometimes these errors result in immediate exit 1, but not always.\n>\n> I'm not sure if the TAP framework that BAIL_OUT() references expects\n> the bail out error on a specific fd.\n\nAll TAP must be emitted to stdout. You can test that with e.g.:\n    \n    $ cat tap.sh\n    #!/bin/sh \n    echo \"ok 1 one\"\n    echo \"ok 2 two\" >&2\n    echo \"1..1\"\n    $ prove --exec /bin/sh tap.sh\n    tap.sh .. 1/? ok 2 two\n    tap.sh .. ok   \n    All tests successful.\n    Files=1, Tests=1,  0 wallclock secs ( 0.01 usr +  0.00 sys =  0.01 CPU)\n    Result: PASS\n\nNote how the \"ok 2 two\" is emitted to STDERR, and doesn't count towards\nthe number of tests. If it's changed to:\n    \n    $ cat tap.sh\n    #!/bin/sh\n    echo \"ok 1 one\"\n    echo \"ok 2 two\"\n    echo \"1..2\"\n    $ prove --exec /bin/sh tap.sh\n    tap.sh .. ok   \n    All tests successful.\n    Files=1, Tests=2,  0 wallclock secs ( 0.00 usr +  0.01 sys =  0.01 CPU)\n    Result: PASS\n\nYou can see it runs two tests.\n\nThe reason the existing cases are inconsistent are because of various\nreasons, probably none good at this point.\n\nSome are just because the error handling pre-dates the TAP support in\nthe test suite, I think at this point we should just be moving to making\nit first-class in terms of TAP support. I.e. it's clearly the most\ncommonly used test mode (and we use it in CI etc.). So we should emit\nall directives on STDOUT.\n\nAnd some are probably just copy/pasting, or error handling that didn't\nconsider TAP at the time of writing.\n\nNote that not all of these should be \"Bail out!\". We should really\nreserve that for wanting to stall the entire test run, but e.g. not for\n\"cannot prep test area\", which might only be a permission error with one\ntrash directory.\n\n\n\n"},{"id":"442757","messageId":"20211201085315.576865-1-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211120150401.254408-1-fs@gigacodes.de","subject":"[PATCH v4 0/3] test-lib: improve missing prereq handling","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-01T08:53:12Z","receivedAt":"2021-12-01T08:53:24Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"The ssh signing feature was breaking tests when the broken openssh-8.7\nwas used. We have now fixed that by checking for this exact case in the\nGPGSSH prereq and I will improve that check further in a future patch.\nHowever we are now in a situation where a broken openssh in the future\nwill result in successfull tests but not a working git build afterwards\n(either not compiling in the expected feature or like in the ssh case\nruntime failures) resulting in a false sense of security in the tests.\nThis patches try to improve this situation by showing which prereqs\nfailed in the test summary and by adding an environment variable to\nenforce certain prereqs to succeed or abort the test otherwise.\n\nSee also:\nhttps://public-inbox.org/git/xmqqv916wh7t.fsf@gitster.g/\n\nchanges since v3:\n - reword comment about test framework fd setup\n\nchanges sinve v2:\n - use a space separated list for GIT_TES_REQUIRED_PREREQ like we do for\n   GIT_SKIP_TESTS\n - use BAIL_OUT() insted of just error()\n - make BAIL_OUT() print errors even when used within prereq context\n\nchanges since v1:\n - use \\012 instead of \\n for possible portability reasons\n - fix typo in commit msg\n\nFabian Stelzer (3):\n  test-lib: show missing prereq summary\n  test-lib: introduce required prereq for test runs\n  test-lib: make BAIL_OUT() work in tests and prereq\n\n t/README                |  6 ++++++\n t/aggregate-results.sh  | 17 +++++++++++++++++\n t/test-lib-functions.sh | 11 +++++++++++\n t/test-lib.sh           | 25 +++++++++++++++++++++----\n 4 files changed, 55 insertions(+), 4 deletions(-)\n\nRange-diff against v3:\n1:  35c92671e5 = 1:  9617d336c7 test-lib: show missing prereq summary\n2:  d6a53f0980 = 2:  409694823a test-lib: introduce required prereq for test runs\n3:  de21c484d6 ! 3:  3757e4e238 test-lib: make BAIL_OUT() work in tests and prereq\n    @@ t/test-lib.sh: USER_TERM=\"$TERM\"\n      TERM=dumb\n      export TERM USER_TERM\n      \n    -+# Set up additional fds so we can control single test i/o\n    ++# What is written by tests to stdout and stderr is sent so different places\n    ++# depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee\n    ++# with --tee option, etc.). We save the original stdin to FD #6 and stdout and\n    ++# stderr to #5 and #7, so that the test framework can use them (e.g. for\n    ++# printing errors within the test framework) independently of the test mode.\n     +exec 5>&1\n     +exec 6<&0\n     +exec 7>&2\n\nbase-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa\n-- \n2.31.1\n\n"},{"id":"442758","messageId":"20211201085315.576865-2-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211201085315.576865-1-fs@gigacodes.de","subject":"[PATCH v4 1/3] test-lib: show missing prereq summary","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-01T08:53:13Z","receivedAt":"2021-12-01T08:53:26Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"When running the full test suite many tests can be skipped because of\nmissing prerequisites. It not easy right now to get an overview of which\nones are missing.\nWhen switching to a new machine or environment some libraries and tools\nmight be missing or maybe a dependency broke completely. In this case\nthe tests would indicate nothing since all dependant tests are simply\nskipped. This could hide broken behaviour or missing features in the\nbuild. Therefore this patch summarizes the missing prereqs at the end of\nthe test run making it easier to spot such cases.\n\n - Add failed prereqs to the test results.\n - Aggregate and then show them with the totals.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/aggregate-results.sh | 17 +++++++++++++++++\n t/test-lib.sh          | 11 +++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/t/aggregate-results.sh b/t/aggregate-results.sh\nindex 7913e206ed..7f2b83bdc8 100755\n--- a/t/aggregate-results.sh\n+++ b/t/aggregate-results.sh\n@@ -6,6 +6,7 @@ success=0\n failed=0\n broken=0\n total=0\n+missing_prereq=\n \n while read file\n do\n@@ -30,10 +31,26 @@ do\n \t\t\tbroken=$(($broken + $value)) ;;\n \t\ttotal)\n \t\t\ttotal=$(($total + $value)) ;;\n+\t\tmissing_prereq)\n+\t\t\tmissing_prereq=\"$missing_prereq,$value\" ;;\n \t\tesac\n \tdone <\"$file\"\n done\n \n+if test -n \"$missing_prereq\"\n+then\n+\tunique_missing_prereq=$(\n+\t\techo $missing_prereq |\n+\t\ttr -s \",\" \"\\n\" |\n+\t\tgrep -v '^$' |\n+\t\tsort -u |\n+\t\tpaste -s -d ' ')\n+\tif test -n \"$unique_missing_prereq\"\n+\tthen\n+\t\tprintf \"\\nmissing prereq: $unique_missing_prereq\\n\\n\"\n+\tfi\n+fi\n+\n if test -n \"$failed_tests\"\n then\n \tprintf \"\\nfailed test(s):$failed_tests\\n\\n\"\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 57efcc5e97..9090ce1225 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -669,6 +669,8 @@ test_fixed=0\n test_broken=0\n test_success=0\n \n+test_missing_prereq=\n+\n test_external_has_tap=0\n \n die () {\n@@ -1069,6 +1071,14 @@ test_skip () {\n \t\t\tof_prereq=\" of $test_prereq\"\n \t\tfi\n \t\tskipped_reason=\"missing $missing_prereq${of_prereq}\"\n+\n+\t\t# Keep a list of all the missing prereq for result aggregation\n+\t\tif test -z \"$missing_prereq\"\n+\t\tthen\n+\t\t\ttest_missing_prereq=$missing_prereq\n+\t\telse\n+\t\t\ttest_missing_prereq=\"$test_missing_prereq,$missing_prereq\"\n+\t\tfi\n \tfi\n \n \tcase \"$to_skip\" in\n@@ -1175,6 +1185,7 @@ test_done () {\n \t\tfixed $test_fixed\n \t\tbroken $test_broken\n \t\tfailed $test_failure\n+\t\tmissing_prereq $test_missing_prereq\n \n \t\tEOF\n \tfi\n-- \n2.31.1\n\n"},{"id":"442759","messageId":"20211201085315.576865-3-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211201085315.576865-1-fs@gigacodes.de","subject":"[PATCH v4 2/3] test-lib: introduce required prereq for test runs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-01T08:53:14Z","receivedAt":"2021-12-01T08:53:30Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"In certain environments or for specific test scenarios we might expect a\nspecific prerequisite check to succeed. Therefore we would like to abort\nrunning our tests if this is not the case.\n\nTo remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ\nwhich can be set to a space separated list of prereqs. If one of these\nprereq tests fail then the whole test run will abort.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/README                |  6 ++++++\n t/test-lib-functions.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 29f72354bf..2353a4c5e1 100644\n--- a/t/README\n+++ b/t/README\n@@ -466,6 +466,12 @@ explicitly providing repositories when accessing submodule objects is\n complete or needs to be abandoned for whatever reason (in which case the\n migrated codepaths still retain their performance benefits).\n \n+GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a space speparated list of\n+prereqs that are required to succeed. If a prereq in this list is triggered by\n+a test and then fails then the whole test run will abort. This can help to make\n+sure the expected tests are executed and not silently skipped when their\n+dependency breaks or is simply not present in a new environment.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex eef2262a36..389153e591 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -680,6 +680,17 @@ test_have_prereq () {\n \t\t\t# Keep a list of missing prerequisites; restore\n \t\t\t# the negative marker if necessary.\n \t\t\tprerequisite=${negative_prereq:+!}$prerequisite\n+\n+\t\t\t# Abort if this prereq was marked as required\n+\t\t\tif test -n \"$GIT_TEST_REQUIRE_PREREQ\"\n+\t\t\tthen\n+\t\t\t\tcase \" $GIT_TEST_REQUIRE_PREREQ \" in\n+\t\t\t\t*\" $prerequisite \"*)\n+\t\t\t\t\tBAIL_OUT \"required prereq $prerequisite failed\"\n+\t\t\t\t\t;;\n+\t\t\t\tesac\n+\t\t\tfi\n+\n \t\t\tif test -z \"$missing_prereq\"\n \t\t\tthen\n \t\t\t\tmissing_prereq=$prerequisite\n-- \n2.31.1\n\n"},{"id":"442760","messageId":"20211201085315.576865-4-fs@gigacodes.de","threadId":"56924","inReplyTo":"20211201085315.576865-1-fs@gigacodes.de","subject":"[PATCH v4 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-01T08:53:15Z","receivedAt":"2021-12-01T08:53:32Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"BAIL_OUT() is meant to abort the whole test run and print a message with\na standard prefix that can be parsed to stdout. Since for every test the\nnormal fd`s are redirected in test_eval_ this output would not be seen\nwhen used within the context of a test or prereq like we do in\ntest_have_prereq(). To make this function work in these contexts we move\nthe setup of the fd aliases a few lines up before the first use of\nBAIL_OUT() and then have this function always print to the alias.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/test-lib.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9090ce1225..14a7aeae0f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -589,6 +589,15 @@ USER_TERM=\"$TERM\"\n TERM=dumb\n export TERM USER_TERM\n \n+# What is written by tests to stdout and stderr is sent so different places\n+# depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee\n+# with --tee option, etc.). We save the original stdin to FD #6 and stdout and\n+# stderr to #5 and #7, so that the test framework can use them (e.g. for\n+# printing errors within the test framework) independently of the test mode.\n+exec 5>&1\n+exec 6<&0\n+exec 7>&2\n+\n _error_exit () {\n \tfinalize_junit_xml\n \tGIT_EXIT_OK=t\n@@ -612,7 +621,7 @@ BAIL_OUT () {\n \tlocal bail_out=\"Bail out! \"\n \tlocal message=\"$1\"\n \n-\tsay_color error $bail_out \"$message\"\n+\tsay_color error $bail_out \"$message\" >&5\n \t_error_exit\n }\n \n@@ -637,9 +646,6 @@ then\n \texit 0\n fi\n \n-exec 5>&1\n-exec 6<&0\n-exec 7>&2\n if test \"$verbose_log\" = \"t\"\n then\n \texec 3>>\"$GIT_TEST_TEE_OUTPUT_FILE\" 4>&3\n-- \n2.31.1\n\n"},{"id":"442802","messageId":"CA+kUOa=Yh0NdoKWEQbYP4YXv8JmMHkjrKREAj-5YAA_JBTqEbQ@mail.gmail.com","threadId":"56924","inReplyTo":"20211201085315.576865-1-fs@gigacodes.de","subject":"Re: [PATCH v4 0/3] test-lib: improve missing prereq handling","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-12-01T21:05:26Z","receivedAt":"2021-12-01T21:05:55Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Wed, 1 Dec 2021 at 08:53, Fabian Stelzer <fs@gigacodes.de> wrote:\n>\n> The ssh signing feature was breaking tests when the broken openssh-8.7\n> was used. We have now fixed that by checking for this exact case in the\n> GPGSSH prereq and I will improve that check further in a future patch.\n> However we are now in a situation where a broken openssh in the future\n> will result in successfull tests but not a working git build afterwards\n\nNit, purely because I just spotted it: \"successfull\" should be \"successful\".\n\n> (either not compiling in the expected feature or like in the ssh case\n> runtime failures) resulting in a false sense of security in the tests.\n> This patches try to improve this situation by showing which prereqs\n> failed in the test summary and by adding an environment variable to\n> enforce certain prereqs to succeed or abort the test otherwise.\n\nI've not managed to keep up with the ongoing development of this\nfunction, but I've just tested a recent version (specifically, from\nJunio's tree, 1ade7d2334 (test-lib: make BAIL_OUT() work in tests and\nprereq, 2021-11-20)). This looks like it would have been fantastically\nuseful when I was first taking over the maintainership of Git for\nCygwin, and I'm looking forward to having the extra confidence of my\nbuilds and tests after I can add GIT_TEST_REQUIRE_PREREQ to my build\nscripts.\n\nThank you!\n"},{"id":"442827","messageId":"xmqqv907yl40.fsf@gitster.g","threadId":"56924","inReplyTo":"20211201085315.576865-4-fs@gigacodes.de","subject":"Re: [PATCH v4 3/3] test-lib: make BAIL_OUT() work in tests and prereq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-01T23:13:35Z","receivedAt":"2021-12-01T23:13:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n> BAIL_OUT() is meant to abort the whole test run and print a message with\n> a standard prefix that can be parsed to stdout. Since for every test the\n> normal fd`s are redirected in test_eval_ this output would not be seen\n> when used within the context of a test or prereq like we do in\n> test_have_prereq(). To make this function work in these contexts we move\n> the setup of the fd aliases a few lines up before the first use of\n> BAIL_OUT() and then have this function always print to the alias.\n>\n> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>\n> ---\n>  t/test-lib.sh | 14 ++++++++++----\n>  1 file changed, 10 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 9090ce1225..14a7aeae0f 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -589,6 +589,15 @@ USER_TERM=\"$TERM\"\n>  TERM=dumb\n>  export TERM USER_TERM\n>  \n> +# What is written by tests to stdout and stderr is sent so different places\n\n\"sent so\" -> \"sent to\".  I'll tweak locally, so no need to resend.\n\n> +# depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee\n> +# with --tee option, etc.). We save the original stdin to FD #6 and stdout and\n> +# stderr to #5 and #7, so that the test framework can use them (e.g. for\n> +# printing errors within the test framework) independently of the test mode.\n> +exec 5>&1\n> +exec 6<&0\n> +exec 7>&2\n> +\n>  _error_exit () {\n>  \tfinalize_junit_xml\n>  \tGIT_EXIT_OK=t\n> @@ -612,7 +621,7 @@ BAIL_OUT () {\n>  \tlocal bail_out=\"Bail out! \"\n>  \tlocal message=\"$1\"\n>  \n> -\tsay_color error $bail_out \"$message\"\n> +\tsay_color error $bail_out \"$message\" >&5\n\nThis is merely a style thing, but as commands get longer, it becomes\neasier to spot redirection if it is written immediately after the\nverb, i.e.\n\n\tsay_color >&5 error $bail_out \"$message\"\n\n>  \t_error_exit\n>  }\n>  \n> @@ -637,9 +646,6 @@ then\n>  \texit 0\n>  fi\n>  \n> -exec 5>&1\n> -exec 6<&0\n> -exec 7>&2\n>  if test \"$verbose_log\" = \"t\"\n>  then\n>  \texec 3>>\"$GIT_TEST_TEE_OUTPUT_FILE\" 4>&3\n\nLooks good.  Thanks.\n"}]}