[PATCH] tests: fix incorrect --write-junit-xml code

[PATCH] tests: fix incorrect --write-junit-xml code

[Johannes Schindelin via GitGitGadget]

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

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    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(-)

diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh
index c959183c7e2..79c31c788b9 100644
--- a/t/test-lib-junit.sh
+++ b/t/test-lib-junit.sh
@@ -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

Re: [PATCH] tests: fix incorrect --write-junit-xml code

[Junio C Hamano]

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

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

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" <gitgitgadget@gmail.com>
writes:

>  t/test-lib-junit.sh | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh
> index c959183c7e2..79c31c788b9 100644
> --- a/t/test-lib-junit.sh
> +++ b/t/test-lib-junit.sh
> @@ -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

OK.  Ancient shells did not understand "--" and it was idiomatic to
say "set x ...; shift", but we already do assume "set --" is usable
everywhere we care about in many of our scripts and tests.

Looks good to me.

Thanks.  Will queue.

Re: [PATCH] tests: fix incorrect --write-junit-xml code

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 14 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>>     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.
>
> 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?

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)?

never refactor v.s. testing (was: [PATCH] tests: fix incorrect --write-junit-xml code)

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 14 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [...]
>     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.

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/220714.86fsj4356l.gmgdl@evledraar.gmail.com/

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 17a268ccd1b..f3c12314787 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -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 \
diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh
index 79c31c788b9..2aac1c6c548 100644
--- a/t/test-lib-junit.sh
+++ b/t/test-lib-junit.sh
@@ -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"

Re: [PATCH] tests: fix incorrect --write-junit-xml code

[Johannes Schindelin]

Hi Junio,

On Thu, 14 Jul 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> >     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.
>
> I wonder if it would make your life easier

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

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

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.

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> >  t/test-lib-junit.sh | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh
> > index c959183c7e2..79c31c788b9 100644
> > --- a/t/test-lib-junit.sh
> > +++ b/t/test-lib-junit.sh
> > @@ -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
>
> OK.  Ancient shells did not understand "--" and it was idiomatic to
> say "set x ...; shift", but we already do assume "set --" is usable
> everywhere we care about in many of our scripts and tests.
>
> Looks good to me.
>
> Thanks.  Will queue.

Thank you,
Dscho

Re: [PATCH] tests: fix incorrect --write-junit-xml code

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Absolutely.

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

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

Re: [PATCH] tests: fix incorrect --write-junit-xml code

[Johannes Schindelin]

Hi Junio,

On Mon, 8 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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.
>
> 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-<.

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

Re: [PATCH] tests: fix incorrect --write-junit-xml code

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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 ;-)

Re: [PATCH] tests: fix incorrect --write-junit-xml code

[Johannes Schindelin]

Hi Junio,

On Wed, 10 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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.
>
> 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.

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

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

Amen!

Ciao,
Dscho