mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
To: Johannes Schindelin <>
Cc: Eric Sunshine <>,
	Eric Sunshine via GitGitGadget <>,
Subject: "test_atexit" v.s. "test_when_finished" (was: [PATCH 3/3] t1509: facilitate repeated script invocations)
Date: Thu, 08 Dec 2022 14:14:39 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <n2586428-1r80-9s29-8345-7p2opnor5086@tzk.qr>

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


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

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

  reply	other threads:[~2022-12-08 13:23 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-21  3:00 [PATCH 0/3] fix t1509-root-work-tree failure Eric Sunshine via GitGitGadget
2022-11-21  3:00 ` [PATCH 1/3] t1509: fix failing "root work tree" test due to owner-check Eric Sunshine via GitGitGadget
2022-12-08 11:49   ` Johannes Schindelin
2022-11-21  3:00 ` [PATCH 2/3] t1509: make "setup" test more robust Eric Sunshine via GitGitGadget
2022-12-08 11:49   ` Johannes Schindelin
2022-11-21  3:00 ` [PATCH 3/3] t1509: facilitate repeated script invocations Eric Sunshine via GitGitGadget
2022-12-06  2:42   ` Ævar Arnfjörð Bjarmason
2022-12-06  3:23     ` Eric Sunshine
2022-12-08 12:04       ` Johannes Schindelin
2022-12-08 13:14         ` Ævar Arnfjörð Bjarmason [this message]
2022-12-09  4:46           ` "test_atexit" v.s. "test_when_finished" Junio C Hamano
2022-12-05 18:21 ` [PATCH 0/3] fix t1509-root-work-tree failure Eric Sunshine
2022-12-08 12:10   ` Johannes Schindelin
2022-12-09  4:59     ` Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).