patch, 2 partstest-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak

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(-)

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 <avarab@gmail.com>
---
 t/test-lib.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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.

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 <avarab@gmail.com>
---
 t/test-lib.sh | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

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.

On Thu, Oct 14 2021, Junio C Hamano wrote:

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