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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Dec 08 2022, Johannes Schindelin wrote:

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