Re: [PATCH v5 0/4] t: replace incorrect test_must_fail usage (part 5)

From: Jeff King <peff@peff.net>
Date: Jun 23, 2020, 18:40 UTC
Message-ID: <20200623184011.GC1444619@coredump.intra.peff.net>
In-Reply-To: <cover.1592907663.git.liu.denton@gmail.com>

On Tue, Jun 23, 2020 at 06:28:01AM -0400, Denton Liu wrote:

Show 22 quoted lines

> I've taken your suggestions and rerolled the last patch. Notably, I
> combined $before and $command into a single function and then passed in
> `test_must_fail` as the second argument if it's expected to fail. I also
> decided to use a helper function to reduce the duplication of the
> calling logic but it's still a little verbose because I opted to make
> use of argument passing over global variables where possible.
> 
> One thing that I'm leaning towards is completely removing $after. We can
> combine $after with $command like I did in the first few iterations of
> this series and just write it as:
> 
> 	git_test_thing () {
> 		# before...
> 		$2 git do-the-thing "$1" &&
> 		if test -n "$2"
> 		then
> 			return
> 		fi &&
> 		# after...
> 	}
> 
> but I'm also okay with leaving it as is.

Yeah, as soon as I saw "combined $before and $command", then I wondered why we needed to split anything (since clearly we're passing responsibility to handle must_fail to the callback function).

So I think we should either have the full before/command/after split, or have a single callback function which understands that it has to respect "$2" as a prefix).

I'm OK with either. I do like both of them better than the magic $OVERWRITING_FAIL thing, but this series seems to be taking a lot more energy than I think its outcome is worth. Let's resolve it somehow and work on something more exciting. ;)

Show 7 quoted lines

> Also, one problem that's present is that for tests that use
> test_submodule_switch() and test_submodule_forced_switch(), $command
> will be listed as git_test_func, which might make debugging more
> difficult. I think I'd prefer the version we currently have queued for
> this reason (and why I did the whole song and dance of $OVERWRITING_FAIL
> in the first place), but if you think that this is fine, I'm fine with
> this approach as well.

I agree it's not ideal, but I don't think it's too bad. There are lots of tests that have output you can't just cut-and-paste due to code generation, helper functions, etc.

-Peff