Re: [PATCH 1/3] t6050-replace: make failing editor test more robust

[Jeff King <peff@peff.net>]

On Tue, Jan 05, 2016 at 11:33:30AM +0100, SZEDER Gábor wrote:

> However, this editor doesn't actually modify the edited object,
> because start_command() turns this editor into:
> 
>   /bin/sh -c './fakeeditor;false "$@"' './fakeeditor;false' \
>           '.../.git/REPLACE_EDITOBJ'

Thanks for thorough explanation. I think your patch makes sense.

> Should we be more thorough, perhaps, and check the error message to be
> extra sure that 'git replace --edit' errors out for the expected
> reason?  There are oh so many 'test_must_fail's in our test scripts
> and we don't check the error message in most of the cases...

We usually try to avoid hard-coding error messages, because they end up
brittle. I think if we've isolated the failure, it's a reasonable test
(in an ideal world, you check that "foo" doesn't fail, and "foo -wrong"
does fail; i.e., just changing one variable in your experiment).

>  test_expect_success '--edit with and without already replaced object' '
> @@ -372,7 +376,7 @@ test_expect_success '--edit with and without already replaced object' '
>  test_expect_success '--edit and change nothing or command failed' '
>  	git replace -d "$PARA3" &&
>  	test_must_fail env GIT_EDITOR=true git replace --edit "$PARA3" &&
> -	test_must_fail env GIT_EDITOR="./fakeeditor;false" git replace --edit "$PARA3" &&
> +	test_must_fail env GIT_EDITOR="./failingfakeeditor" git replace --edit "$PARA3" &&

We have the same problem when running aliases, or any git command that
you want to expand into more complex shell. The usual solution for
one-off is something like:

  test_must_fail env GIT_EDITOR="f() { ./fakeeditor; false; } f" git ...

That might be preferable to yours, because a reader can see immediately
in the test what is going on, without wondering what it is that
failingfakeeditor does. OTOH, it is perhaps somewhat non-obvious. It
came to mind to me because it is an idiom we use elsewhere; I remember
thinking it was very clever the first time somebody showed it to me. :)

I'd be OK with the patch using either method.

-Peff