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

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

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

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:

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.

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

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,

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

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

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

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,

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

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

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

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,

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

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

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

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/