Re: [PATCH] t3206: test_when_finished before dirtying operations, not after

Eric Sunshine [off-list ref] writes:

On Mon, Aug 5, 2024 at 8:55 PM Junio C Hamano [off-list ref] wrote:

quoted

Many existing tests in this script perform operation(s) and then use
test_when_finished to define how to undo the effect of the
operation(s).

This is backwards.  When your operation(s) fail before you manage to
successfully call test_when_finished (remember, that these commands
must be all &&-chained, so a failure of an earlier operation mean
your test_when_finished may not be executed at all).  You must
establish how to clean up your mess with test_when_finished before
you create the mess to be cleaned up.

Signed-off-by: Junio C Hamano <redacted>
---
 t/t3206-range-diff.sh | 52 +++++++++++++++++++++----------------------

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh

@@ -533,9 +533,9 @@ test_expect_success 'dual-coloring' '
 for prev in topic main..topic
 do
        test_expect_success "format-patch --range-diff=$prev" '
+               test_when_finished "rm 000?-*" &&
                git format-patch --cover-letter --range-diff=$prev \
                        main..unmodified >actual &&
-               test_when_finished "rm 000?-*" &&

Do we care whether the action invoked by `test_when_finished` itself
succeeds or fails? In particular, should this be using `rm -f` rather
than `rm`?

Thanks for good eyes.  The original avoids that issue by making sure
it only installs the clean-up after the operation to create crufts
successfully completes ;-)  Of course, if it fails in the middle,
then the crufts are left behind X-<.

quoted

@@ -606,9 +606,9 @@ test_expect_success 'basic with modified format.pretty without "commit "' '
 test_expect_success 'range-diff compares notes by default' '
+       test_when_finished git notes remove topic unmodified &&
        git notes add -m "topic note" topic &&
        git notes add -m "unmodified note" unmodified &&
-       test_when_finished git notes remove topic unmodified &&

Similarly, should this be using `|| :`?

Ah, I forgot that "notes remove" would barf when there is no note to
remove, instead of being idempotent no-op.  Yes, you'd need ||: there.

    test_when_finished "git notes remove topic unmodified || :" &&