Subject: Re: [PATCH 2/2] diff tests: rewrite flakyness-causing test "aid"

From: Junio C Hamano <[email-masked]>
Date: Wed, 14 Apr 2021 00:53:30 -0700

Ævar Arnfjörð Bjarmason <[email-masked]> writes:

>>> Thus if we do:
>>>
>>>     git something >expected &&
>>>     test_cmp expected actual &&
>>>     rm expected actual
>>
>> Isn't it a poor example to use to argue for your particular change,
>> where $actual in the original is designed to be unique among tests,
>> in order to ensure that $actual files left after test pieces fail
>> would not interfere with the tests that come later?  IOW, there is
>> not a reason to remove $actual until the end of the test sequence,
>> is there?
>
> Not really, because you needed to read the rest of the test file to come
> to that conclusion.
>
> The point of using a helper that guarantees cleanup such as
> test_when_finished or test_config over manual "git config" or "git rm"
> isn't that we can prove that we need it because a later test needs the
> cleanup, but that anyone can add new tests or functionality without
> having to worry about cleaning up the existing trash directory.
>
> So yes, it's not needed here, but that's only because we know the rest
> of the tests don't have e.g. a test that does a:

In this particular case, $actual files are designed to be left
behind for failed test pieces, so that the tester can come back and
inspect them.  I probably should have said it a bit more strongly
than "there is not a reason to remove".  You SHOULD NOT remove and
that is why we had "check and then remove only upon success" there,
instead of test_when_finished.  We want them left for and only for
failing test pieces.

Please do not advocate for and encourage newbies who would be
reading the discussion from sidelines to use test_when_finished out
of dogmatic principle without thinking.  Even though there are valid
cases where test_when_finished is the perfect fit, in this
particular case, use of it is a clear regression.

Thanks.