git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak
@ 2021-10-14  0:47 Ævar Arnfjörð Bjarmason
  2021-10-14  0:47 ` [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code Ævar Arnfjörð Bjarmason
  2021-10-14  0:47 ` [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14  0:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Æ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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code
  2021-10-14  0:47 [PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak Ævar Arnfjörð Bjarmason
@ 2021-10-14  0:47 ` Ævar Arnfjörð Bjarmason
  2021-10-14 16:26   ` Junio C Hamano
  2021-10-14  0:47 ` [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14  0:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Æ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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use
  2021-10-14  0:47 [PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak Ævar Arnfjörð Bjarmason
  2021-10-14  0:47 ` [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code Ævar Arnfjörð Bjarmason
@ 2021-10-14  0:47 ` Ævar Arnfjörð Bjarmason
  2021-10-14 16:39   ` Junio C Hamano
  1 sibling, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14  0:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Æ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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code
  2021-10-14  0:47 ` [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code Ævar Arnfjörð Bjarmason
@ 2021-10-14 16:26   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-10-14 16:26 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King

Æ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_ () {

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use
  2021-10-14  0:47 ` [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use Ævar Arnfjörð Bjarmason
@ 2021-10-14 16:39   ` Junio C Hamano
  2021-10-14 17:25     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2021-10-14 16:39 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use
  2021-10-14 16:39   ` Junio C Hamano
@ 2021-10-14 17:25     ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 17:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-10-14 17:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14  0:47 [PATCH 0/2] test-lib.sh: add BAIL_OUT function, use it for SANITIZE=leak Ævar Arnfjörð Bjarmason
2021-10-14  0:47 ` [PATCH 1/2] test-lib.sh: de-duplicate error() teardown code Ævar Arnfjörð Bjarmason
2021-10-14 16:26   ` Junio C Hamano
2021-10-14  0:47 ` [PATCH 2/2] test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use Ævar Arnfjörð Bjarmason
2021-10-14 16:39   ` Junio C Hamano
2021-10-14 17:25     ` Ævar Arnfjörð Bjarmason

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).