Re: [PATCH 1/3] t6050-replace: make failing editor test more robust
- From
Jeff King <peff@peff.net>
- Date
- Jan 5, 2016, 15:39 UTC
- Message-ID
- <20160105153933.GA5586@sigill.intra.peff.net>
- In-Reply-To
- <1451990010-15458-1-git-send-email-szeder@ira.uka.de>
On Tue, Jan 05, 2016 at 11:33:30AM +0100, SZEDER Gábor wrote:
Show 5 quoted lines
> 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).
Show 7 quoted lines
> 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