"test_atexit" v.s. "test_when_finished" (was: [PATCH 3/3] t1509: facilitate repeated script invocations)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 8, 2022, 13:14 UTC
Message-ID: <221208.86fsdq6nci.gmgdl@evledraar.gmail.com>
In-Reply-To: <n2586428-1r80-9s29-8345-7p2opnor5086@tzk.qr>

On Thu, Dec 08 2022, Johannes Schindelin wrote:

Show 11 quoted lines

> On Mon, 5 Dec 2022, Eric Sunshine wrote:
>
>> On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> > On Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:
>>> [...]
>> > This is an existing wart, but I also wondered why the "expected",
>> > "result" etc. was needed. Either we could make the tests creating those
>> > do a "test_when_finished" removal of it, or better yet just create those
>> > in the trash directory.
>
> An even better suggestion would be to use `test_atexit`, of course.

Why?

For assets that are only needed within a given test we prefer cleaning them up with "test_when_finished", there's legitimate uses for "test_atexit", but those are for global state.

In this case (and again, we're discussing the #leftoverbits if someone wants to poke at this again) the tests in question could relatively easily be changed to do the creation and cleanup of the files that are "test_cmp"'d (or similar) within the lifetime of individual tests ("test_when_finished"), rather than the lifetime of the script ("test_atexit").

A good reason for why we do it way is that it has a nice interaction with "--immediate --debug".

On failure we'll skip the cleanup for the current test that just failed, but we're not distracted by scratch files from earlier tests, those would have already been cleaned up if they used the same "test_when_finished" pattern.

If you use "test_atexit" to do that all subsequent tests need to deal with the sum of your scratch files, until they're cleaned up in one big operation at the end.

It not only makes that debugging case harder, but also to write tests, as you'll need to contend with more unwanted global state in your test playground the further down the test file you are.

So I think what you're recommending here is an anti-pattern for the common case.

There *are* cases where we really do need the "global cleanup", e.g. tests that spawn the apache httpd use "test_atexit" rather than "test_when_finished", we don't want to have to start/stop the httpd for each test.

We should leave "test_atexit" for those sorts of cases, not routine per-test scratch file creation.

I semi-regularly run into cases where a stale "httpd" is left running in the background from such tests (and not after I kill -9'd a test), so I suspect we also have tricky races in that are, that probably aren't improved by "test_atexit".