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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2021, 08:14 UTC
Message-ID: <xmqqlf9krmkx.fsf@gitster.g>
In-Reply-To: <87y2dl18mg.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 12 quoted lines

> 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.

Show 6 quoted lines

>> 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.