Re: [PATCH v3] t3200: fix antipatterns in existing branch tests

[Tao Klerks <tao@klerks.biz>]

On Wed, May 4, 2022 at 7:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>
> > @@ -510,63 +512,57 @@ test_expect_success 'git branch --copy dumps usage' '
> >  test_expect_success 'git branch -c d e should work' '
> >       git branch --create-reflog d &&
> >       git reflog exists refs/heads/d &&
> > -     git config branch.d.dummy Hello &&
> > +     test_config branch.d.dummy Hello &&
> >       git branch -c d e &&
> >       git reflog exists refs/heads/d &&
> >       git reflog exists refs/heads/e &&
> > -     echo Hello >expect &&
> > -     git config branch.e.dummy >actual &&
> > -     test_cmp expect actual &&
> > -     echo Hello >expect &&
> > -     git config branch.d.dummy >actual &&
> > -     test_cmp expect actual
> > +     test_cmp_config Hello branch.e.dummy &&
> > +     test_cmp_config Hello branch.d.dummy
> >  '
>
> This test used to leave both branch.d.dummy and branch.e.dummy behind
> for later tests.  Now with this patch, we clean branch.d.dummy
> because we use test_config, but branch.e.dummy that was copied by
> successful "git branch -c" will still be left.
>
>  - It is unforunate that it is impossible to verify that the change
>    in behaviour for branch.d.dummy is correct.  Without checking all
>    the remainder of the test (and no, grepping for branch.d.dummy is
>    not "checking all the remainder"---a later "branch -c d x" would
>    have created brnach.x.dummy in the original, but with this patch,
>    it would not), which is time consuming, that is.
>
>    I trust you made sure that branch.d.dummy is never used after
>    this test is done---it would have been good to explain it either
>    in the proposed log message or after three-dash that you did
>    check and how to save reviewer bandwidth.

I will add a comment, and will (first) more diligently check for
dependencies on things now-removed.

My strategy was, frankly, a little more haphazard: eyeball the intent,
look for references in the following couple tests, and otherwise
assume that if the tests still pass, it means they didn't unexpectedly
depend on this.

Fwiw, the not-trivial-but-still-straightforward approach I'm using is
to search case-sensitively for the word "d" in this example. a, b, c
and d are all bad because they are common english words or flags used
throughout this test, but other letters and combinations are easier to
check in this reasonably-simple way.

>
>  - Are you deliberatly leaving branch.e.dummy uncleaned, or is it a
>    mere oversight?
>

Somewhere in-between. My intent with these changes was to use the
right helpers/patterns, but I did not aspire to making sure nothing
leaks between tests at all.

Generally speaking, none of the tests I've seen (in this file or
others) clean up / delete the *branches* they create - presumably
because there's no significant need - the existence of branches rarely
or never has side-effects unless explicitly referenced. Config, on the
other hand, is a good thing to clean up by default, because it is much
more likely to interfere with later tests.

Something like configuring "branch.x.dummy" (or the more meaningful
"branch.x.merge") straddles those 2 worlds - I would use test_config
to be consistent if setting one manually, but it's still
branch-specific stuff so I wouldn't be diligent about removing it if
it was created as a side-effect of another command like "git branch
-c" - just like I'm not removing the new branch itself.

That said, now that you've called my attention to it, I'll look for
leaky branch configs :)

I will also add a note clarifying "intend to use the right
patterns/helpers, but not necessarily to eliminate state leaks between
tests" in the commit message.

> >  test_expect_success 'git branch --copy is a synonym for -c' '
> >       git branch --create-reflog copy &&
> >       git reflog exists refs/heads/copy &&
> > -     git config branch.copy.dummy Hello &&
> > +     test_config branch.copy.dummy Hello &&
> >       git branch --copy copy copy-to &&
> >       git reflog exists refs/heads/copy &&
> >       git reflog exists refs/heads/copy-to &&
> > -     echo Hello >expect &&
> > -     git config branch.copy.dummy >actual &&
> > -     test_cmp expect actual &&
> > -     echo Hello >expect &&
> > -     git config branch.copy-to.dummy >actual &&
> > -     test_cmp expect actual
> > +     test_cmp_config Hello branch.copy.dummy &&
> > +     test_cmp_config Hello branch.copy-to.dummy
> >  '
>
> The same comment for branch.copy.dummy and branch.copy-to.dummy
> applies.
>
> I'll stop here for now.  Thanks for starting this clean-up.

Thank you!