Re: [PATCH 8/8] t0012: test "-h" with builtins

From: Jeff King <peff@peff.net>
Date: Jun 14, 2017, 10:03 UTC
Message-ID: <20170614100352.ebkvlntpcpug3hax@sigill.intra.peff.net>
In-Reply-To: <20170613230803.GP133952@aiede.mtv.corp.google.com>

On Tue, Jun 13, 2017 at 04:08:03PM -0700, Jonathan Nieder wrote:

Show 20 quoted lines

> > +test_expect_success 'generate builtin list' '
> > +	git --list-builtins >builtins
> > +'
> > +
> > +while read builtin
> > +do
> > +	test_expect_success "$builtin can handle -h" '
> > +		test_expect_code 129 git $builtin -h >output 2>&1 &&
> > +		test_i18ngrep usage output
> > +	'
> > +done <builtins
> 
> I love this.  A few thoughts:
> 
> - I think the "generate builtin list" test should be marked as setup
>   so people know it can't be skipped.  I'll send a followup to do that.
> 
> - By the same token, if "generate builtin list" fails then these
>   commands afterward would fail in an unpleasant way.  Fortunately
>   that's straightforward to fix, too.

Be my guest if you want, but I don't think either of those is actually worth spending any time on. It's a nice convenience when tests are independent, but ultimately if any single test fails, the results of all tests after it must be suspect. There are too many hidden dependencies to treat it any other way.

So I have no problem with skipping tests while debugging for a quicker turnaround, with the knowledge that what you're seeing _might_ be slightly invalidated. But I don't think it's worth dumping developer time into trying to make each block independent. That's what individual scripts are for.

(I also think it's particularly worthless for this script; the whole thing runs in ~500ms on my machine. It takes longer to type GIT_TEST_SKIP).

Show 7 quoted lines

> - This checks both stdout and stderr to verify that the usage appears
>   somewhere.  I would have expected commands to be consistent ---
>   should they always send usage to stdout, since the user requested it,
>   or to stderr, since that's what we have done historically?
> 
>   So I understand why this test is agnostic about that but I suspect
>   it's pointing to some inconsistency that could be fixed independently.

The difference is between parse-option's usage_with_options() and usage(). The former sends "-h" to stdout and errors to stderr, which I think is sensible. The latter only knows about stderr.

I think it would be reasonable to have follow the parse-options strategy consistently. It will definitely take some tweaking, though. There are lots of commands that respect "-h" only by hitting some default unhandled case. So you'll not only have to have a stdout analog for usage(), but you'll have to teach each command when to use each.

Show 9 quoted lines

> - Do we plan to translate the "usage:" prefix some day?  I could go
>   both ways --- on one hand, having a predictable error:/usage:/fatal:
>   prefix may make life easier for e.g. GUI authors trying to show
>   different kinds of error messages in different ways, but on the other
>   hand, always using English for the prefix may be unfriendly to non
>   English speakers.
> 
>   In the past I had assumed it would never be translated but now I'm
>   not so sure.  What do you think?

I don't have an opinion either way. I know we don't translate it now, but it has been discussed. I figured it couldn't hurt to use i18ngrep to future-proof it.

> - Should we have something like this for non-builtins, too?

That would be nice, though you'll probably need some cooperation from the Makefile to get the list. I was specifically worried about catching the particular setup_git_directory() interaction with builtins here, so it seemed like a good stopping point.

But as a general rule, making sure that everything responds sensibly to "-h" seems like a good thing.

-Peff