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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 30, 2019, 09:52 UTC
Message-ID: <nycvar.QRO.7.76.6.1909301151050.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqq7e5sjc1p.fsf@gitster-ct.c.googlers.com>

Hi Junio,

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

Show 33 quoted lines

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

Show 10 quoted lines

> 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

Show 25 quoted lines

>
> > @@ -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
>