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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 23, 2021, 09:16 UTC
Message-ID: <87mttp1hv3.fsf@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_N9_eXtuGcXSj1bwP5sy+t1fn8Q=X1na0jK9OpzDz2UAA@mail.gmail.com>

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

Show 18 quoted lines

> On Fri, Apr 23, 2021 at 9:47 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
>>
>> On Fri, Apr 23, 2021 at 7:12 AM SZEDER Gábor <szeder.dev@gmail.com> 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.