Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL

From: Denton Liu <liu.denton@gmail.com>
Date: May 21, 2020, 18:55 UTC
Message-ID: <20200521185514.GA615266@generichostname>
In-Reply-To: <20200521182928.GA1308647@coredump.intra.peff.net>

Hi Peff,

On Thu, May 21, 2020 at 02:29:28PM -0400, Jeff King wrote:

Show 19 quoted lines

> > This is useful because currently, when we run a test helper function, we
> > just mark the whole thing as `test_must_fail`. However, it's possible
> > that the helper function might fail earlier or later than expected due
> > to an introduced bug. If this happens, then the test case will still
> > report as passing but it should really be marked as failing since it
> > didn't actually display the intended behaviour.
> 
> Now this second concern I think is much more interesting, because it
> impacts the test results. And it's really not even about test_must_fail
> in particular, but is a general problem with checking failure in any
> compound operation. We may expect the early parts to succeed, but the
> later parts to fail, and we want to tell the difference. And that's true
> whether you're using "!", test_must_fail, etc.
> 
> You solve it here by passing OVERWRITING_FAIL down into the callback
> functions. And that does work. But I think it may be easier to
> understand if we invert the responsibility. Let the outer caller specify
> two callbacks: one for setup/prep that must succeed, and one for a
> single operation where we might expect success or failure.

I believe that we'll need a third optional argument. For example, in t3906, we have the following diff

	diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
	index 6a7e801ca0..6e8dac9669 100755
	--- a/t/t3906-stash-submodule.sh
	+++ b/t/t3906-stash-submodule.sh
	@@ -8,7 +8,11 @@ test_description='stash can handle submodules'
	 git_stash () {
		git status -su >expect &&
		ls -1pR * >>expect &&
	-	git read-tree -u -m "$1" &&
	+	$OVERWRITING_FAIL git read-tree -u -m "$1" &&
	+	if test -n "$OVERWRITING_FAIL"
	+	then
	+		return
	+	fi &&
		git stash &&
		git status -su >actual &&
		ls -1pR * >>actual &&

which means, when we're doing test_must_fail, we have to skip the remainder of the function otherwise, if the first command succeeds but it fails later in the test, then we report success even though it didn't fail at the point where we were expecting. I think that we'll have to have an optional third arg for $cleanup or something. The only thing that I'm worried about is that having three separate functions being passed in may be too messy.

Aside from that, I like this approach much more than mine.

Thanks,

Denton