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

[Jeff King <peff@peff.net>]

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

> > +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).

> - 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.

> - 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