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

From: Junio C Hamano <[email-masked]>
Date: Thu, 15 Apr 2021 01:14:38 -0700

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

> What I am saying is that it's incompatible to have:
>
>  1. Failing tests
>  2. Not removing scratch files that would otherwise be removed
>
> And:
>
>  3. Knowing that the rest of the tests pass in the case of #1 without
>     reading them all.
>
> Hence the suggestion that we should use test_when_finished without
> exception for such patterns.

I disagree with the above; t4013's "read magic cmd" part is designed
to be independent from each other; I do not think you need to read
all of the parts enclosed in << ... EOF to understand that.

In short,

 * "test_when_finished 'rm ...'" is a good tool to ensure something
   gets removed no matter what else happens in the same test.  Since
   it does not run the clean-up under "-i", it can even be used on
   files that would be useful in diagnosing failures.  But under
   "-d", it does clean-up to avoid affecting the following test.

 * $actual was made unique so that even under "-d", a failing test
   would not negatively affect the subsequent ones.  Removing it for
   failure cases is actively wrong, so use of test_when_finished,
   which may be an expedient way to remove artifact that would
   negatively affect later test pieces, does not apply --- existing
   code is doing better than test_when_finished can offer, and
   replacing it with test_when_finished is a regression.

 * And the most important part: the unnecessary removal of $actual
   was not even part of the "flakyness-causing" bug you started this
   topic to fix anyway.  We should just remove the regression caused
   by unnecessary use of test_when_finished and focus on the "don't
   place the actual output from a brand new test under the file used
   for the expectation the next time---instead place it under
   temporary file and call for attention" part, which was the real
   improvement.

>> 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.
>
> Is Matheus the newbie here? I think he's contributed enough to form his
> own opinion.

No, those "reading the discussion from sidelines" are not on To: or
Cc:, but are eager to learn by reading what is available to git@
subscribers (including the lore archive).  I do not want those of us
whose name appear often in the list archive to be sending a wrong
signal to them.

> In any case, I think it's best to just drop this series.

That is a sad and wrong conclusion.  We should just realize what we
really wanted to fix in the first place and keep the improvement;
otherwise all the review time was wasted.

And next time, I'd suggest you not to spend too many bytes on "while
at it".  Clear and obvious clean-up while the tree and the project
is otherwise quiescent is very much no-brainer welcome, but
otherwise...

Thanks.