[PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak

[PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak

[Ævar Arnfjörð Bjarmason]

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

[PATCH 1/2] test-lib.sh: de-duplicate error() teardown code

[Ævar Arnfjörð Bjarmason]

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

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

[PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use

[Ævar Arnfjörð Bjarmason]

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

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

Re: [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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 <avarab@gmail.com>
> ---
>  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_ () {

Re: [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use

[Junio C Hamano]

> +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.

Re: [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use

[Ævar Arnfjörð Bjarmason]

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