diff mbox series

[11/13] tests: let --immediate and --write-junit-xml play well together

Message ID 99724f6a1e45b497e15037bbac1cb5f70a3bb236.1569486607.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series ci: include a Visual Studio build & test in our Azure Pipeline | expand

Commit Message

Linus Arver via GitGitGadget Sept. 26, 2019, 8:30 a.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

When the `--immediate` option is in effect, any test failure will
immediately exit the test script. Together with `--write-junit-xml`, we
will want the JUnit-style `.xml` file to be finalized (and not leave the
XML incomplete). Let's make it so.

This comes in particularly handy when trying to debug via Azure
Pipelines, where the JUnit-style XML is consumed to present the test
results in an informative and helpful way.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

Comments

Junio C Hamano Sept. 28, 2019, 10:22 p.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index d1ba33745a..f21c781e68 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -695,7 +695,7 @@ test_failure_ () {
>  	say_color error "not ok $test_count - $1"
>  	shift
>  	printf '%s\n' "$*" | sed -e 's/^/#	/'
> -	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
> +	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
>  }

There are three places that do GIT_EXIT_OK=t in the test framework,
and the above covers one of them.  The original in test_done is
another, and that place is made to call the "finalize" thing (it
used to have the same finalization code inlined).

The remaining one appears in

        error () {
                say_color error "error: $*"
                GIT_EXIT_OK=t
                exit 1
        }

I wonder if we should cover this case, too.  One caller of "error" I
know is BUG that says "bug in the test script", which means that
after successfully passing 30 tests, when the 31st test has 5 params
to test_expect_success by mistake, without finailzation we will lose
the result for the first 30.

And if we call "finalize" from the "error" helper, perhaps it makes
even more sense to update the above manual exit in test_failure_ to
do something like

	if test -n "$immediate"
	then
		error "immediate exit after the first error"
	fi

to delegate the finalization.

> @@ -1085,21 +1104,7 @@ test_done () {
>  	# removed, so the commands can access pidfiles and socket files.
>  	test_atexit_handler
>  
> -	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
> -	then
> -		test -n "$junit_have_testcase" || {
> -			junit_start=$(test-tool date getnanos)
> -			write_junit_xml_testcase "all tests skipped"
> -		}
> -
> -		# adjust the overall time
> -		junit_time=$(test-tool date getnanos $junit_suite_start)
> -		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
> -			<"$junit_xml_path" >"$junit_xml_path.new"
> -		mv "$junit_xml_path.new" "$junit_xml_path"
> -
> -		write_junit_xml "  </testsuite>" "</testsuites>"
> -	fi
> +	finalize_junit_xml
>  
>  	if test -z "$HARNESS_ACTIVE"
>  	then
Johannes Schindelin Sept. 30, 2019, 9:52 a.m. UTC | #2 
Hi Junio,

On Sun, 29 Sep 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index d1ba33745a..f21c781e68 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -695,7 +695,7 @@ test_failure_ () {
> >  	say_color error "not ok $test_count - $1"
> >  	shift
> >  	printf '%s\n' "$*" | sed -e 's/^/#	/'
> > -	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
> > +	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
> >  }
>
> There are three places that do GIT_EXIT_OK=t in the test framework,
> and the above covers one of them.  The original in test_done is
> another, and that place is made to call the "finalize" thing (it
> used to have the same finalization code inlined).
>
> The remaining one appears in
>
>         error () {
>                 say_color error "error: $*"
>                 GIT_EXIT_OK=t
>                 exit 1
>         }
>
> I wonder if we should cover this case, too.  One caller of "error" I
> know is BUG that says "bug in the test script", which means that
> after successfully passing 30 tests, when the 31st test has 5 params
> to test_expect_success by mistake, without finailzation we will lose
> the result for the first 30.

Good point.

> And if we call "finalize" from the "error" helper, perhaps it makes
> even more sense to update the above manual exit in test_failure_ to
> do something like
>
> 	if test -n "$immediate"
> 	then
> 		error "immediate exit after the first error"
> 	fi
>
> to delegate the finalization.

This adds an additional message. I am not sure how many scripts/CI
integrations are there that rely on the current behavior, so I would
like to exclude this change from this here patch series: it is about
including a Visual Studio build in our Azure Pipeline, nothing more,
nothing less...

Ciao,
Dscho

>
> > @@ -1085,21 +1104,7 @@ test_done () {
> >  	# removed, so the commands can access pidfiles and socket files.
> >  	test_atexit_handler
> >
> > -	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
> > -	then
> > -		test -n "$junit_have_testcase" || {
> > -			junit_start=$(test-tool date getnanos)
> > -			write_junit_xml_testcase "all tests skipped"
> > -		}
> > -
> > -		# adjust the overall time
> > -		junit_time=$(test-tool date getnanos $junit_suite_start)
> > -		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
> > -			<"$junit_xml_path" >"$junit_xml_path.new"
> > -		mv "$junit_xml_path.new" "$junit_xml_path"
> > -
> > -		write_junit_xml "  </testsuite>" "</testsuites>"
> > -	fi
> > +	finalize_junit_xml
> >
> >  	if test -z "$HARNESS_ACTIVE"
> >  	then
>
diff mbox series

Patch

diff --git a/t/test-lib.sh b/t/test-lib.sh
index d1ba33745a..f21c781e68 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -695,7 +695,7 @@  test_failure_ () {
 	say_color error "not ok $test_count - $1"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
-	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
 }
 
 test_known_broken_ok_ () {
@@ -1063,6 +1063,25 @@  write_junit_xml_testcase () {
 	junit_have_testcase=t
 }
 
+finalize_junit_xml () {
+	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
+	then
+		test -n "$junit_have_testcase" || {
+			junit_start=$(test-tool date getnanos)
+			write_junit_xml_testcase "all tests skipped"
+		}
+
+		# adjust the overall time
+		junit_time=$(test-tool date getnanos $junit_suite_start)
+		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
+			<"$junit_xml_path" >"$junit_xml_path.new"
+		mv "$junit_xml_path.new" "$junit_xml_path"
+
+		write_junit_xml "  </testsuite>" "</testsuites>"
+		write_junit_xml=
+	fi
+}
+
 test_atexit_cleanup=:
 test_atexit_handler () {
 	# In a succeeding test script 'test_atexit_handler' is invoked
@@ -1085,21 +1104,7 @@  test_done () {
 	# removed, so the commands can access pidfiles and socket files.
 	test_atexit_handler
 
-	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
-	then
-		test -n "$junit_have_testcase" || {
-			junit_start=$(test-tool date getnanos)
-			write_junit_xml_testcase "all tests skipped"
-		}
-
-		# adjust the overall time
-		junit_time=$(test-tool date getnanos $junit_suite_start)
-		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
-			<"$junit_xml_path" >"$junit_xml_path.new"
-		mv "$junit_xml_path.new" "$junit_xml_path"
-
-		write_junit_xml "  </testsuite>" "</testsuites>"
-	fi
+	finalize_junit_xml
 
 	if test -z "$HARNESS_ACTIVE"
 	then