Subject: Re: [PATCH 04/18] t1401-symbolic-ref: avoid direct filesystem access

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 23 Apr 2021 11:16:00 +0200

On Fri, Apr 23 2021, Han-Wen Nienhuys wrote:

> On Fri, Apr 23, 2021 at 9:47 AM Han-Wen Nienhuys <[email-masked]> wrote:
>>
>> On Fri, Apr 23, 2021 at 7:12 AM SZEDER Gábor <[email-masked]> wrote:
>> > > I see it as the test writer's job to clean up to the extent that
>> > > git-symbolic-ref can reset to a sane state.
>> >
>> > No, it's the job of whoever updates the cleanup routine to make sure
>> > that the updated cleanup routine still works just as well as it did in
>> > the past.
>>
>> Sorry, I don't mean to shirk my responsibility. What I mean is: is
>> there a way to signal "This test is fubar. Stop further execution of
>> test shell script".
>
> Or, more generally: if a cleanup routine fails, what is the point of
> continuing to run the test script? If that happens, the test directory
> is an indeterminate state, so the outcome of follow-on test-functions
> is indeterminate as well.

It's not a cleanup routine that's failing, but potentially the "setup
the next test" routine.

In this case it's probably best to invert the logic, i.e. to make the
tests do:

	test_when_finished "rm -rf copy" &&	
	git clone . copy &&
	<test-logic>

Rather than:

	reset_state &&
	<test-logic>

That also makes it more obvious what's setup and what/where tests rely
on things carried forward from previous tests.

This test is just written in a style that I think it's fair to say
wouldn't pass review today. E.g. if you skip the first N tests we do
that resetting N times redundantly without any need for it. The pattern
of moving state setup & teardown into individual tests is better.