{"thread":{"id":"56708","subject":"[PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak","startedAt":"2021-10-14T00:47:36Z","lastAt":"2021-10-14T17:28:26Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"438742","messageId":"cover-0.2-00000000000-20211014T004542Z-avarab@gmail.com","threadId":"56708","inReplyTo":null,"subject":"[PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:47:27Z","receivedAt":"2021-10-14T00:47:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series adds a BAIL_OUT function, and uses it when the new\nGIT_TEST_PASSING_SANITIZE_LEAK=true mode is misused.\n\nOnce we have this function we'll be able to use it for any other error\nthat's a cause for aborting the entire test run.\n\nI experimented with making BUG() and error() always be a \"BAIL_OUT\". I\nthink that's worth pursuing, but e.g. for the error about missing\n\"&&-chains\" we'd need to support emitting multi-line messages.\n\nTAP consumers only understand what follows the \"Bail out!\" message up\nto the first \"\\n\", so we can't quote the entire \"test_expect_success\",\nas the \"&&-chain\" error does. I think emitting them with \"say_error()\"\nbeforehand (piped with \">&7\" in the case of \"BUG()\") should work, but\nlet's leave those #leftoverbits for later.\n\nÆvar Arnfjörð Bjarmason (2):\n  test-lib.sh: de-duplicate error() teardown code\n  test-lib.sh: use \"Bail out!\" syntax on bad SANITIZE=leak use\n\n t/test-lib.sh | 28 +++++++++++++++++++++-------\n 1 file changed, 21 insertions(+), 7 deletions(-)\n\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438743","messageId":"patch-1.2-d4c67829076-20211014T004542Z-avarab@gmail.com","threadId":"56708","inReplyTo":"cover-0.2-00000000000-20211014T004542Z-avarab@gmail.com","subject":"[PATCH 1/2] test-lib.sh: de-duplicate error() teardown code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:47:28Z","receivedAt":"2021-10-14T00:47:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"De-duplicate the \"finalize_junit_xml; GIT_EXIT_OK=t; exit 1\" code\nshared between the \"error()\" and \"--immediate on failure\" code paths,\nin preparation for adding a third user in a subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/test-lib.sh | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8361b5c1c57..c610f09ddb1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -589,13 +589,17 @@ USER_TERM=\"$TERM\"\n TERM=dumb\n export TERM USER_TERM\n \n-error () {\n-\tsay_color error \"error: $*\"\n+_error_exit () {\n \tfinalize_junit_xml\n \tGIT_EXIT_OK=t\n \texit 1\n }\n \n+error () {\n+\tsay_color error \"error: $*\"\n+\t_error_exit\n+}\n+\n BUG () {\n \terror >&7 \"bug in the test script: $*\"\n }\n@@ -720,7 +724,7 @@ test_failure_ () {\n \tsay_color error \"not ok $test_count - $1\"\n \tshift\n \tprintf '%s\\n' \"$*\" | sed -e 's/^/#\t/'\n-\ttest \"$immediate\" = \"\" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }\n+\ttest \"$immediate\" = \"\" || _error_exit\n }\n \n test_known_broken_ok_ () {\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438744","messageId":"patch-2.2-6fd2a64bcfa-20211014T004542Z-avarab@gmail.com","threadId":"56708","inReplyTo":"cover-0.2-00000000000-20211014T004542Z-avarab@gmail.com","subject":"[PATCH 2/2] test-lib.sh: use \"Bail out!\" syntax on bad SANITIZE=leak use","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:47:29Z","receivedAt":"2021-10-14T00:47:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Improve the \"GIT_TEST_PASSING_SANITIZE_LEAK=true\" test mode added in\n956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in CI,\n2021-09-23) to use a TAP \"Bail out!\" message when exiting. This will\ncause the test run to exit immediately under a TAP consumer like\n\"prove(1)\".\n\nSee 614fe015212 (test-lib: bail out when \"-v\" used under \"prove\",\n2016-10-22) for the initial introduction of \"Bail out!\" to the\n--verbose being amended here.\n\nBefore this compiling with \"SANITIZE=\" and running the tests with\n\"prove(1)\" would cause all the tests to be run to the end (output\ntrimmed for fewer columns):\n\n    $ GIT_TEST_PASSING_SANITIZE_LEAK=true make\n    rm -f -r 'test-results'\n    *** prove ***\n    t0000-basic.sh ......... Dubious, test returned 1 (wstat 256, 0x100)\n    No subtests run\n    t0001-init.sh .......... Dubious, test returned 1 (wstat 256, 0x100)\n    No subtests run\n    [...output where we list every single t[0-9]*.sh file as failing snipped]\n\nWhereas now we'll fail early, like this (\"->\" line wrapping added):\n\n    $ GIT_TEST_PASSING_SANITIZE_LEAK=true make\n    [...]\n\n    t0000-basic.sh ..................................... Bailout called.  Further testing stopped:\n    -> GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak\n    FAILED--Further testing stopped: GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except\n    -> when compiled with SANITIZE=leak\n    make: *** [Makefile:53: prove] Error 1\n\nThis change also adds a red color to the \"Bailout called\" line, as\nwe're now using \"say_color error\". That improves existing output in\nthe case of e.g.:\n\n    $ prove -j8 t[0-9]*.sh :: -v\n    Bailout called.  Further testing stopped:  verbose mode forbidden under TAP harness; try --verbose-log\n    FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log\n\nWe don't need to have a \"Bail out! \" prefix when we're not running\nunder a TAP consumer (i.e. if test -n \"$HARNESS_ACTIVE\"), but let's\nnot make the output conditional on that. Showing it under e.g.:\n\n    $ GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0095-bloom.sh\n    Bail out! GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak\n\nDoesn't harm anything, and I don't think the (small) complexity of\nonly adding this if we're under \"$HARNESS_ACTIVE\" is worth it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/test-lib.sh | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c610f09ddb1..617cda36f3a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -604,6 +604,18 @@ BUG () {\n \terror >&7 \"bug in the test script: $*\"\n }\n \n+BAIL_OUT () {\n+\ttest $# -ne 1 && BUG \"1 param\"\n+\n+\t# Do not change \"Bail out! \" string. It's part of TAP syntax:\n+\t# https://testanything.org/tap-specification.html\n+\tlocal bail_out=\"Bail out! \"\n+\tlocal message=\"$1\"\n+\n+\tsay_color error $bail_out \"$message\"\n+\t_error_exit\n+}\n+\n say () {\n \tsay_color info \"$*\"\n }\n@@ -612,9 +624,7 @@ if test -n \"$HARNESS_ACTIVE\"\n then\n \tif test \"$verbose\" = t || test -n \"$verbose_only\"\n \tthen\n-\t\tprintf 'Bail out! %s\\n' \\\n-\t\t 'verbose mode forbidden under TAP harness; try --verbose-log'\n-\t\texit 1\n+\t\tBAIL_OUT 'verbose mode forbidden under TAP harness; try --verbose-log'\n \tfi\n fi\n \n@@ -1402,7 +1412,7 @@ then\n \tfi\n elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n then\n-\terror \"GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak\"\n+\tBAIL_OUT \"GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak\"\n fi\n \n # Last-minute variable setup\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438760","messageId":"xmqqzgrbd12r.fsf@gitster.g","threadId":"56708","inReplyTo":"patch-1.2-d4c67829076-20211014T004542Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-14T16:26:52Z","receivedAt":"2021-10-14T16:26:58Z","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> De-duplicate the \"finalize_junit_xml; GIT_EXIT_OK=t; exit 1\" code\n> shared between the \"error()\" and \"--immediate on failure\" code paths,\n> in preparation for adding a third user in a subsequent commit.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  t/test-lib.sh | 10 +++++++---\n>  1 file changed, 7 insertions(+), 3 deletions(-)\n\nNice.  I do recall seeing the duplication when these finalize-junit\ncalls were introduced and thought about marking it as a leftover\nclean-up candidate.  Nice to see it done here.\n\nThanks.\n\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 8361b5c1c57..c610f09ddb1 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -589,13 +589,17 @@ USER_TERM=\"$TERM\"\n>  TERM=dumb\n>  export TERM USER_TERM\n>  \n> -error () {\n> -\tsay_color error \"error: $*\"\n> +_error_exit () {\n>  \tfinalize_junit_xml\n>  \tGIT_EXIT_OK=t\n>  \texit 1\n>  }\n>  \n> +error () {\n> +\tsay_color error \"error: $*\"\n> +\t_error_exit\n> +}\n> +\n>  BUG () {\n>  \terror >&7 \"bug in the test script: $*\"\n>  }\n> @@ -720,7 +724,7 @@ test_failure_ () {\n>  \tsay_color error \"not ok $test_count - $1\"\n>  \tshift\n>  \tprintf '%s\\n' \"$*\" | sed -e 's/^/#\t/'\n> -\ttest \"$immediate\" = \"\" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }\n> +\ttest \"$immediate\" = \"\" || _error_exit\n>  }\n>  \n>  test_known_broken_ok_ () {\n"},{"id":"438761","messageId":"xmqqsfx3d0i3.fsf@gitster.g","threadId":"56708","inReplyTo":"patch-2.2-6fd2a64bcfa-20211014T004542Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] test-lib.sh: use \"Bail out!\" syntax on bad SANITIZE=leak use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-14T16:39:16Z","receivedAt":"2021-10-14T16:39:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> +BAIL_OUT () {\n> +\ttest $# -ne 1 && BUG \"1 param\"\n> +\n> +\t# Do not change \"Bail out! \" string. It's part of TAP syntax:\n> +\t# https://testanything.org/tap-specification.html\n> +\tlocal bail_out=\"Bail out! \"\n> +\tlocal message=\"$1\"\n> +\n> +\tsay_color error $bail_out \"$message\"\n> +\t_error_exit\n> +}\n\nThis looks like a good addition that can be used in similar cases\nlater.  I'd assume that \"Bail out!\" early of a test sequence will\nmark the test suite as a failure as a whole, but I wonder if there\nis a similar \"early abort\" mechanism that would cause a success?  In\nsome tests we do\n\n\tif .. some condition ..\n\tthen\n\t\ttest_done ;# because we cannot test the remainder\n\tfi\n\n        .. more tests ...\n\nand I've always thought the \"done\" an ugly hack.\n\nBut that has nothing to do with the value of this change.\n\nThanks.\n"},{"id":"438773","messageId":"87v91zfrd7.fsf@evledraar.gmail.com","threadId":"56708","inReplyTo":"xmqqsfx3d0i3.fsf@gitster.g","subject":"Re: [PATCH 2/2] test-lib.sh: use \"Bail out!\" syntax on bad SANITIZE=leak use","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T17:25:30Z","receivedAt":"2021-10-14T17:28:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 14 2021, Junio C Hamano wrote:\n\n>> +BAIL_OUT () {\n>> +\ttest $# -ne 1 && BUG \"1 param\"\n>> +\n>> +\t# Do not change \"Bail out! \" string. It's part of TAP syntax:\n>> +\t# https://testanything.org/tap-specification.html\n>> +\tlocal bail_out=\"Bail out! \"\n>> +\tlocal message=\"$1\"\n>> +\n>> +\tsay_color error $bail_out \"$message\"\n>> +\t_error_exit\n>> +}\n>\n> This looks like a good addition that can be used in similar cases\n> later.  I'd assume that \"Bail out!\" early of a test sequence will\n> mark the test suite as a failure as a whole, but I wonder if there\n> is a similar \"early abort\" mechanism that would cause a success?  In\n> some tests we do\n>\n> \tif .. some condition ..\n> \tthen\n> \t\ttest_done ;# because we cannot test the remainder\n> \tfi\n>\n>         .. more tests ...\n>\n> and I've always thought the \"done\" an ugly hack.\n>\n> But that has nothing to do with the value of this change.\n>\n> Thanks.\n\nThat's:\n\n    skip_all=\"reason\"\n    test_done\n\nWhich we could spell as:\n\n    skip_all \"reason\"\n\nOr whatever, if we wanted such a self-documenting helper.\n\nThere's no facility in TAP syntax to indicate in some special way that\nyou'd like to skip all remaining things once your test is already\nrunning, the best you can do is just exit at that point.\n\nI.e. the (very light amount of) special-ness (such as it is) is that if\nthe very first line of the output is \"1..0 skip [...]\" progarms like\n\"prove\" will emit the skip summary as part of the output. See DIRECTIVES\nat https://testanything.org/tap-specification.html\n"}]}