Re: [PATCH] t3206: test_when_finished before dirtying operations, not after
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 6, 2024, 16:52 UTC
- Message-ID
- <xmqq4j7xvbiv.fsf@gitster.g>
- In-Reply-To
- <CAPig+cRO=BqR1WFcni6CSxqyqvt1Ksmsyr0odmqTDKX4JdbDaA@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 28 quoted lines
> On Mon, Aug 5, 2024 at 8:55 PM Junio C Hamano <gitster@pobox.com> wrote: >> 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 <gitster@pobox.com> >> --- >> 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-<.
Show 8 quoted lines
>> @@ -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 || :" &&