# [PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak 6 messages from 2021-10-14 to 2021-10-14. Participants: Ævar Arnfjörð Bjarmason, Junio C Hamano. Thread: https://gitlist.dev/t/56708 ## Ævar Arnfjörð Bjarmason, 2021-10-14 00:47 Subject: [PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak Message-ID: URL: https://gitlist.dev/e/cover-0.2-00000000000-20211014T004542Z-avarab%40gmail.com ``` This series adds a BAIL_OUT function, and uses it when the new GIT_TEST_PASSING_SANITIZE_LEAK=true mode is misused. Once we have this function we'll be able to use it for any other error that's a cause for aborting the entire test run. I experimented with making BUG() and error() always be a "BAIL_OUT". I think that's worth pursuing, but e.g. for the error about missing "&&-chains" we'd need to support emitting multi-line messages. TAP consumers only understand what follows the "Bail out!" message up to the first "\n", so we can't quote the entire "test_expect_success", as the "&&-chain" error does. I think emitting them with "say_error()" beforehand (piped with ">&7" in the case of "BUG()") should work, but let's leave those #leftoverbits for later. Ævar Arnfjörð Bjarmason (2): test-lib.sh: de-duplicate error() teardown code test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use t/test-lib.sh | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) -- 2.33.1.1346.g48288c3c089 ``` ## Ævar Arnfjörð Bjarmason, 2021-10-14 00:47 Subject: [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code Message-ID: URL: https://gitlist.dev/e/patch-1.2-d4c67829076-20211014T004542Z-avarab%40gmail.com In-Reply-To: ``` De-duplicate the "finalize_junit_xml; GIT_EXIT_OK=t; exit 1" code shared between the "error()" and "--immediate on failure" code paths, in preparation for adding a third user in a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason --- t/test-lib.sh | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 8361b5c1c57..c610f09ddb1 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -589,13 +589,17 @@ USER_TERM="$TERM" TERM=dumb export TERM USER_TERM -error () { - say_color error "error: $*" +_error_exit () { finalize_junit_xml GIT_EXIT_OK=t exit 1 } +error () { + say_color error "error: $*" + _error_exit +} + BUG () { error >&7 "bug in the test script: $*" } @@ -720,7 +724,7 @@ test_failure_ () { say_color error "not ok $test_count - $1" shift printf '%s\n' "$*" | sed -e 's/^/# /' - test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; } + test "$immediate" = "" || _error_exit } test_known_broken_ok_ () { -- 2.33.1.1346.g48288c3c089 ``` ## Ævar Arnfjörð Bjarmason, 2021-10-14 00:47 Subject: [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use Message-ID: URL: https://gitlist.dev/e/patch-2.2-6fd2a64bcfa-20211014T004542Z-avarab%40gmail.com In-Reply-To: ``` Improve the "GIT_TEST_PASSING_SANITIZE_LEAK=true" test mode added in 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in CI, 2021-09-23) to use a TAP "Bail out!" message when exiting. This will cause the test run to exit immediately under a TAP consumer like "prove(1)". See 614fe015212 (test-lib: bail out when "-v" used under "prove", 2016-10-22) for the initial introduction of "Bail out!" to the --verbose being amended here. Before this compiling with "SANITIZE=" and running the tests with "prove(1)" would cause all the tests to be run to the end (output trimmed for fewer columns): $ GIT_TEST_PASSING_SANITIZE_LEAK=true make rm -f -r 'test-results' *** prove *** t0000-basic.sh ......... Dubious, test returned 1 (wstat 256, 0x100) No subtests run t0001-init.sh .......... Dubious, test returned 1 (wstat 256, 0x100) No subtests run [...output where we list every single t[0-9]*.sh file as failing snipped] Whereas now we'll fail early, like this ("->" line wrapping added): $ GIT_TEST_PASSING_SANITIZE_LEAK=true make [...] t0000-basic.sh ..................................... Bailout called. Further testing stopped: -> GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak FAILED--Further testing stopped: GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except -> when compiled with SANITIZE=leak make: *** [Makefile:53: prove] Error 1 This change also adds a red color to the "Bailout called" line, as we're now using "say_color error". That improves existing output in the case of e.g.: $ prove -j8 t[0-9]*.sh :: -v Bailout called. Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log We don't need to have a "Bail out! " prefix when we're not running under a TAP consumer (i.e. if test -n "$HARNESS_ACTIVE"), but let's not make the output conditional on that. Showing it under e.g.: $ GIT_TEST_PASSING_SANITIZE_LEAK=true ./t0095-bloom.sh Bail out! GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak Doesn't harm anything, and I don't think the (small) complexity of only adding this if we're under "$HARNESS_ACTIVE" is worth it. Signed-off-by: Ævar Arnfjörð Bjarmason --- t/test-lib.sh | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index c610f09ddb1..617cda36f3a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -604,6 +604,18 @@ BUG () { error >&7 "bug in the test script: $*" } +BAIL_OUT () { + test $# -ne 1 && BUG "1 param" + + # Do not change "Bail out! " string. It's part of TAP syntax: + # https://testanything.org/tap-specification.html + local bail_out="Bail out! " + local message="$1" + + say_color error $bail_out "$message" + _error_exit +} + say () { say_color info "$*" } @@ -612,9 +624,7 @@ if test -n "$HARNESS_ACTIVE" then if test "$verbose" = t || test -n "$verbose_only" then - printf 'Bail out! %s\n' \ - 'verbose mode forbidden under TAP harness; try --verbose-log' - exit 1 + BAIL_OUT 'verbose mode forbidden under TAP harness; try --verbose-log' fi fi @@ -1402,7 +1412,7 @@ then fi elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false then - error "GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak" + BAIL_OUT "GIT_TEST_PASSING_SANITIZE_LEAK=true has no effect except when compiled with SANITIZE=leak" fi # Last-minute variable setup -- 2.33.1.1346.g48288c3c089 ``` ## Junio C Hamano, 2021-10-14 16:26 Subject: Re: [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code Message-ID: URL: https://gitlist.dev/e/xmqqzgrbd12r.fsf%40gitster.g In-Reply-To: ``` Ævar Arnfjörð Bjarmason writes: > De-duplicate the "finalize_junit_xml; GIT_EXIT_OK=t; exit 1" code > shared between the "error()" and "--immediate on failure" code paths, > in preparation for adding a third user in a subsequent commit. > > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > t/test-lib.sh | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) Nice. I do recall seeing the duplication when these finalize-junit calls were introduced and thought about marking it as a leftover clean-up candidate. Nice to see it done here. Thanks. > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 8361b5c1c57..c610f09ddb1 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -589,13 +589,17 @@ USER_TERM="$TERM" > TERM=dumb > export TERM USER_TERM > > -error () { > - say_color error "error: $*" > +_error_exit () { > finalize_junit_xml > GIT_EXIT_OK=t > exit 1 > } > > +error () { > + say_color error "error: $*" > + _error_exit > +} > + > BUG () { > error >&7 "bug in the test script: $*" > } > @@ -720,7 +724,7 @@ test_failure_ () { > say_color error "not ok $test_count - $1" > shift > printf '%s\n' "$*" | sed -e 's/^/# /' > - test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; } > + test "$immediate" = "" || _error_exit > } > > test_known_broken_ok_ () { ``` ## Junio C Hamano, 2021-10-14 16:39 Subject: Re: [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use Message-ID: URL: https://gitlist.dev/e/xmqqsfx3d0i3.fsf%40gitster.g In-Reply-To: ``` > +BAIL_OUT () { > + test $# -ne 1 && BUG "1 param" > + > + # Do not change "Bail out! " string. It's part of TAP syntax: > + # https://testanything.org/tap-specification.html > + local bail_out="Bail out! " > + local message="$1" > + > + say_color error $bail_out "$message" > + _error_exit > +} This looks like a good addition that can be used in similar cases later. I'd assume that "Bail out!" early of a test sequence will mark the test suite as a failure as a whole, but I wonder if there is a similar "early abort" mechanism that would cause a success? In some tests we do if .. some condition .. then test_done ;# because we cannot test the remainder fi .. more tests ... and I've always thought the "done" an ugly hack. But that has nothing to do with the value of this change. Thanks. ``` ## Ævar Arnfjörð Bjarmason, 2021-10-14 17:25 Subject: Re: [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use Message-ID: <87v91zfrd7.fsf@evledraar.gmail.com> URL: https://gitlist.dev/e/87v91zfrd7.fsf%40evledraar.gmail.com In-Reply-To: ``` On Thu, Oct 14 2021, Junio C Hamano wrote: >> +BAIL_OUT () { >> + test $# -ne 1 && BUG "1 param" >> + >> + # Do not change "Bail out! " string. It's part of TAP syntax: >> + # https://testanything.org/tap-specification.html >> + local bail_out="Bail out! " >> + local message="$1" >> + >> + say_color error $bail_out "$message" >> + _error_exit >> +} > > This looks like a good addition that can be used in similar cases > later. I'd assume that "Bail out!" early of a test sequence will > mark the test suite as a failure as a whole, but I wonder if there > is a similar "early abort" mechanism that would cause a success? In > some tests we do > > if .. some condition .. > then > test_done ;# because we cannot test the remainder > fi > > .. more tests ... > > and I've always thought the "done" an ugly hack. > > But that has nothing to do with the value of this change. > > Thanks. That's: skip_all="reason" test_done Which we could spell as: skip_all "reason" Or whatever, if we wanted such a self-documenting helper. There's no facility in TAP syntax to indicate in some special way that you'd like to skip all remaining things once your test is already running, the best you can do is just exit at that point. I.e. the (very light amount of) special-ness (such as it is) is that if the very first line of the output is "1..0 skip [...]" progarms like "prove" will emit the skip summary as part of the output. See DIRECTIVES at https://testanything.org/tap-specification.html ```