tests: fix incorrect --write-junit-xml code

From: Johannes Schindelin <[email protected]>

In 78d5e4cfb4b (tests: refactor --write-junit-xml code, 2022-05-21), this developer refactored the --write-junit-xml code a bit, including the part where the current test case's title was used in a set invocation, but failed to account for the fact that some test cases' titles start with a long option, which the set misinterprets as being intended for parsing.

Let's fix this by using the set -- <...> form.

---
tests(JUnit): fix a regression introduced by refactoring

As if another data point was needed to corroborate my claim that refactorings are neither free of cost nor of risk, this patch fixes a regression I myself introduced while refactoring the JUnit XML output code. At least this refactoring was motivated by an ulterior goal to improve Git's contributor experience, not just refactoring for refactoring's sake.

Unfortunately, I noticed this regression no earlier than when I needed to validate Git for Windows v2.37.1. Since v2.37.1 was an embargoed release, I could not use GitHub Actions for the CI testing, so I had to reinstate Git's Azure Pipeline.

It will probably surprise only few, if at all, that this is far from the only regression in the CI code that I had to fix just so I could run the Azure Pipeline successfully. I plan on contributing all of these regression fixes, of course, packaged into neat little, logically-separate patch series that should be easy on reviewers.

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

 t/test-lib-junit.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index c959183c7e2..79c31c788b9 100644
@@ -46,7 +46,7 @@ finalize_test_case_output () {
 	shift
 	case "$test_case_result" in
 	ok)
-		set "$*"
+		set -- "$*"
 		;;
 	failure)
 		junit_insert="<failure message=\"not ok $test_count -"

@@ -65,17 +65,17 @@ finalize_test_case_output () {
 			junit_insert="$junit_insert<system-err>$(xml_attr_encode \
 				"$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")</system-err>"
 		fi
-		set "$1" "      $junit_insert"
+		set -- "$1" "      $junit_insert"
 		;;
 	fixed)
-		set "$* (breakage fixed)"
+		set -- "$* (breakage fixed)"
 		;;
 	broken)
-		set "$* (known breakage)"
+		set -- "$* (known breakage)"
 		;;
 	skip)
 		message="$(xml_attr_encode --no-lf "$skipped_reason")"
-		set "$1" "      <skipped message=\"$message\" />"
+		set -- "$1" "      <skipped message=\"$message\" />"
 		;;
 	esac
 

base-commit: 4e2a4d1dd44367d7783f33b169698f2930ff13c0

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

I wonder if it would make your life easier if the same GitHub Actions CI stuff were available for the Cabal repository we use for embargoed work, by allowing you to use the same validation for usual releases and the enbargoed ones?

"Johannes Schindelin via GitGitGadget" <[email protected]>

Isn't it. Looking at
e.g. https://github.com/git/cabal/actions/runs/2284056293 (private link) that's the CI run for the latest push to "master" (same as
https://github.com/git/git/commit/e8005e4871f130c4e402ddca2032c111252f070a).

So I didn't get this idea that we must have Azure to resurrect for "private CI", don't we have that already, and anyone can use it by re-pushing git.git to a private repo (the limitation being that on GitHub you can't make a private fork of a public repo)?

Hi Junio,

It would make my life easier if Chesterton's Fences were left alone ;-)

GitHub Actions are available in the Cabal repository. The problem is that due to the private nature of said repository, the number of included build minutes is limited.

I did manage to get GitHub to sponsor the Git project specifically to increase said build minutes. But that only scratches the problem at the surface.

I still think that we need to slow the heck down with refactoring for refactoring's sake because it's not only the CI builds that are affected. I pay a lot of time to accommodate for those refactorings, and so do others, and the benefit of most of those refactorings escapes me. My best guess is that they adapt Git's source code to individual tastes, which is of course a losing game because too many individuals with too many different tastes are involved in the Git project. Unless we start valuing particular individuals' tastes over others', which I believe we should not do.

You asked me in private to provide more reviews for those refactorings so that they see some push-back, but I lack the bandwidth for that.

Thank you,
Dscho

Absolutely.

I do remember telling you to push back what you do not want to see in 'seen' and advance to 'next'. If everybody lacks the bandwidth for shooting down bad ideas and only has time to promote their own ideas, which are not guaranteed to be good ones, it does not lead to a good place X-<.

Hi Junio,

The funny thing is that you're usually simply not picking up patches that do not get any reviews, but for these refactorings it is somehow different, and I do not understand why it needs to be different.

I do not _want_ to spend time reviewing patch series of dubious benefit. And that would hold true even if I could justify spending that time, which I can't.

There are so many more contributions that promise a much higher return of investment for reviewing them.

Ciao,
Dscho

Well, I am not sure if encouraging the maintainer to drop patches that are not reviewed is a good overall direction you would want to go in.

But let's try that. Less work for me, less disruption to our tree, and the world may be quieter and more pleasant ;-)

Hi Junio,

I am not encouraging you to drop high-quality patches that have a clear benefit to the project and/or Git's users. You probably know me better than to assume that I would ever do such a thing. You probably meant this comment tongue-in-cheek, but I cannot really tell.

It's about these long patch series that touch everything and their dog and the end result brings no clear benefit to users nor the Git project itself. I can easily imagine how wonderful it would be not to see such patch series ever again. ;-)

Amen!

Ciao,
Dscho

I take your point that any code change carries risk, but I really think walking away with the lesson that "refactorings" are bad is the wrong thing to draw from this.

This is a textbook example of the value of testing. Code can break because it's refactored directly, or because a library or tool it depended on changed, etc.

The problem here isn't that we changed some code, but that it was changed blindly.

Hindsight is 20/20 on the specifics of any bug, and I don't mean to pick on you for having made a change without adding tests here in particular. I've also done it, and will probably do it in the future where I'm overly sure of myself.

But the idea that we should first consider basic lack of test coverage before making (further) changes really isn't 20/20 hindsight. I think it should be a basic part of our workflow, particularly in cases like this (and the bisect case I pointed out in another thread[1]) where we can see that we could remove the entire feature (or sub-feature for [1]) and still pass 100% of our test.

As test coverage approaches 100% (which is often impossible in practice) the risk of refactorings approaches (but never reaches) 0%. Plot them on a mental graph and they're roughly inversely proportional to one another. We'll almost never reach 100% and 0% in practice.

In this case we have no test coverage for --write-junit-xml before or after this change. Before it were dying right away with just running test-lib.sh with --write-junit-xml, So even the below up to "test_must_be_empty" would catch it.

So we don't need the hypothetical 100%, just getting to 0.1% is enough, and the below hopefully gets us past that to 1% (a lot of --write-junit-xml remains completely untested):

1. https://lore.kernel.org/git/[email protected]/

index 17a268ccd1b..f3c12314787 100755
@@ -238,6 +238,34 @@ test_expect_success 'subtest: --verbose option' '
 	EOF
 '
 
+test_expect_success 'subtest: --write-junit-xml option' '
+	# both --write-junit-xml and lib-subtest.sh expect to use "out"
+	GIT_TEST_JUNIT_DIRECTORY=.junit-xml &&
+	export GIT_TEST_JUNIT_DIRECTORY &&
+
+	write_and_run_sub_test_lib_test_err write-junit-xml \
+		--write-junit-xml <<-\EOF &&
+	test_expect_success "--true <>" "true"
+	test_expect_success "--false <>" "false"
+	test_done
+	EOF
+	test_must_be_empty write-junit-xml/err &&
+
+	sed -e "s/^> //" -e "s/time[^>]*/.../g" >expect <<-\EOF &&
+	> <testsuites>
+	>   <testsuite name="write-junit-xml" ...>
+	>     <testcase name="write.1 --true &lt;&gt;" classname="write" ...>
+	>     </testcase>
+	>     <testcase name="write.2 --false &lt;&gt;" classname="write" ...>
+	>       <failure message="not ok 2 - --false &lt;&gt;"> false&#x0a;</failure>
+	>     </testcase>
+	>   </testsuite>
+	> </testsuites>
+	EOF
+	sed -e "s/time[^>]*/.../g" <write-junit-xml/.junit-xml/TEST-write-junit-xml.xml >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'subtest: --verbose-only option' '
 	run_sub_test_lib_test_err \
 		t1234-verbose \

index 79c31c788b9..2aac1c6c548 100644
@@ -22,7 +22,7 @@
 # that are are called at the appropriate times during the test runs.
 
 start_test_output () {
-	junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
+	junit_xml_dir="$TEST_OUTPUT_DIRECTORY/${GIT_TEST_JUNIT_DIRECTORY:-out}"
 	mkdir -p "$junit_xml_dir"
 	junit_xml_base=${1##*/}
 	junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"