Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
