On Wed, Apr 14 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> On Tue, Apr 13 2021, Matheus Tavares Bernardino wrote:
>> ...
>>>> >> actual="$pfx-diff.$test"
>>>> >>
>>>> >> test_expect_$status "git $cmd # magic is ${magic:-(not used)}" '
>>>> >> + test_when_finished "rm $actual" &&
>>>> >
>>>> > Nit: before these two patches, "$actual" was only removed when the
>>>> > test succeeded. So, in case of failure, the failed output files would
>>>> > still be there for debugging. It might be interesting to keep this
>>>> > behavior and only remove "$actual" at the end of the test.
>> ...
>> Ah, yes that's a problem we should solve, but I think we should not put
>> off migration to test_when_finished because of that.
>>
>> The whole reason we use it is to clean up the work area for the next
>> test.
>>
>> 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:
for file in t4013/*
Anyway, that's only half of the example I cited for why we should use
test_when_finished in general, the other half was noting that because
we've semantically annotated cleanup tasks we can smartly handle those
for debugging, e.g. stash the "removed" files away, and if the test
fails present them to the user.
