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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 28, 2019, 22:22 UTC
Message-ID: <xmqq7e5sjc1p.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <99724f6a1e45b497e15037bbac1cb5f70a3bb236.1569486607.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 11 quoted lines

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

Show 23 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