git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] tests: fix --write-junit-xml with subshells
@ 2020-02-12 11:27 Johannes Schindelin via GitGitGadget
  2020-02-12 17:19 ` Junio C Hamano
  0 siblings, 1 reply; 2+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-02-12 11:27 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In t0000, more precisely in its `test_bool_env` test case, there are two
subshells that are supposed to fail. To be even _more_ precise, they
fail by calling the `error` function, and that is okay, because it is in
a subshell, and it is expected that those two subshell invocations fail.

However, the `error` function also tries to finalize the JUnit XML (if
that XML was asked for, via `--write-junit-xml`. As a consequence, the
XML is edited to add a `time` attribute for the `testsuite` tag. And
since there are two expected `error` calls in addition to the final
`test_done`, the `finalize_junit_xml` function is called three times and
naturally the `time` attribute is added _three times_.

Azure Pipelines is not happy with that, complaining thusly:

 ##[warning]Failed to read D:\a\1\s\t\out\TEST-t0000-basic.xml. Error : 'time' is a duplicate attribute name. Line 2, position 82..

One possible way to address this would be to unset `write_junit_xml` in
the `test_bool_env` test case.

But that would be fragile, as other `error` calls in subshells could be
introduced.

So let's just modify `finalize_junit_xml` to remove any `time` attribute
before adding the authoritative one.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Fix a warning in the Azure Pipelines runs
    
    Yet another thing I stumbled across...

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-552%2Fdscho%2Fwrite-junit-xml-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-552/dscho/write-junit-xml-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/552

 t/test-lib.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 44df51be8f..0ea1e5a05e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1083,7 +1083,8 @@ finalize_junit_xml () {
 
 		# adjust the overall time
 		junit_time=$(test-tool date getnanos $junit_suite_start)
-		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
+		sed -e "s/\(<testsuite.*\) time=\"[^\"]*\"/\1/" \
+			-e "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
 			<"$junit_xml_path" >"$junit_xml_path.new"
 		mv "$junit_xml_path.new" "$junit_xml_path"
 

base-commit: de93cc14ab7e8db7645d8dbe4fd2603f76d5851f
-- 
gitgitgadget

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

* Re: [PATCH] tests: fix --write-junit-xml with subshells
  2020-02-12 11:27 [PATCH] tests: fix --write-junit-xml with subshells Johannes Schindelin via GitGitGadget
@ 2020-02-12 17:19 ` Junio C Hamano
  0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2020-02-12 17:19 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In t0000, more precisely in its `test_bool_env` test case, there are two
> subshells that are supposed to fail. To be even _more_ precise, they
> fail by calling the `error` function, and that is okay, because it is in
> a subshell, and it is expected that those two subshell invocations fail.
>
> However, the `error` function also tries to finalize the JUnit XML (if
> that XML was asked for, via `--write-junit-xml`. As a consequence, the
> XML is edited to add a `time` attribute for the `testsuite` tag. And
> since there are two expected `error` calls in addition to the final
> `test_done`, the `finalize_junit_xml` function is called three times and
> naturally the `time` attribute is added _three times_.
>
> Azure Pipelines is not happy with that, complaining thusly:
>
>  ##[warning]Failed to read D:\a\1\s\t\out\TEST-t0000-basic.xml. Error : 'time' is a duplicate attribute name. Line 2, position 82..
>
> One possible way to address this would be to unset `write_junit_xml` in
> the `test_bool_env` test case.
>
> But that would be fragile, as other `error` calls in subshells could be
> introduced.

Interesting.  Naïvely, I would have expected that the right fix,
especially after reading "as other error calls can be introduced"
was to stop calling finalize from error() and instead call it from
the exit trap, but that would not work, as the exit trap would also
be called even from a subshell X-<.

> So let's just modify `finalize_junit_xml` to remove any `time` attribute
> before adding the authoritative one.

It feels wrong to repeatedly rewrite the $junit_xml_path file during
the test, and it feels doubly wrong to call finalize many number of
times, but given that we do not have a good way to revamp the code
to stop doing so, I think "strip existing time attribute, if any,
and then add what we have" we see here is the best solution
available.

Will queue.  Thanks.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 44df51be8f..0ea1e5a05e 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1083,7 +1083,8 @@ finalize_junit_xml () {
>  
>  		# adjust the overall time
>  		junit_time=$(test-tool date getnanos $junit_suite_start)
> -		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
> +		sed -e "s/\(<testsuite.*\) time=\"[^\"]*\"/\1/" \
> +			-e "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
>  			<"$junit_xml_path" >"$junit_xml_path.new"
>  		mv "$junit_xml_path.new" "$junit_xml_path"
>  
>
> base-commit: de93cc14ab7e8db7645d8dbe4fd2603f76d5851f

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

end of thread, other threads:[~2020-02-12 17:19 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 11:27 [PATCH] tests: fix --write-junit-xml with subshells Johannes Schindelin via GitGitGadget
2020-02-12 17:19 ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git