Re: [PATCH 3/6] test_lib: allow test_{must,might}_fail to accept non-git on "sigpipe"

From: Jeff King <peff@peff.net>
Date: Jan 15, 2021, 10:00 UTC
Message-ID: <YAFntgQE3NZ3yQx5@coredump.intra.peff.net>
In-Reply-To: <87o8hq4jzu.fsf@evledraar.gmail.com>

On Fri, Jan 15, 2021 at 10:39:17AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 20 quoted lines

> > Compare
> >
> > 	some | test_might_fail ok=sigpipe grep | commands | here
> >
> > to
> >
> > 	some | test_filter | commands | here
> 
> I saw your original series/patch. including Junio's suggestion that
> test_grep_return_success was a bit too verbose & the suggestion for
> "test_filter".
> 
> I think the "test_might_fail" in this case is more readable, readable !=
> short. I.e. imagine you haven't just been looking at this code & open
> that test file. If it's using "test_{might,must}_fail ok=*" you're more
> likely to immediately understand it since you've seen those functions in
> lots of places before.
> 
> If not, then "test might fail ok=sigpipe" is almost so self-documenting
> that you don't need to look at the function.

But I'm left confused...do we expect grep to get sigpipe here? I don't think so. The problem is that grep will return 0 for "I did not match anything". We would get sigpipe if "commands | here" exited without reading all of the input.

So the "ok=sigpipe" seems very misleading to me (and likewise, the whole "test_might_fail ok=sigpipe" exception seems weird; sigpipe is not the reason we want to use it with grep here; it is because grep might actually fail).

Whereas the test-filter approach is expressing what we actually are interested in (ignoring the exit code of grep, no matter what).

> Whereas a "test_filter" for me at least would prompt an immediate "hrm?
> what's that?", followed by grepping it and the side-quest of reading the
> source for that function we use in a grand total of <10 places.

I agree this is sometimes a problem. But if we want to inline it, it seems like the correct spelling here is:

  some | { grep ... || true; } | commands | here

Show 6 quoted lines

> Anyway, just my 0.02. I also think it makes conceptual sense to just
> have a limited whitelist in "test_{might,must}_fail", since in this case
> the reason we recommend against its use for non-git doesn't
> apply. I.e. we're normally not in the business of testing the OS, but in
> this case it's got the useful behavior of knowing how to handle the exit
> code & signal special-case, so we might as well use it.

I'm somewhat lukewarm on the whitelist already, as I got bit recently by a (not yet published) test doing:

  foo() {
	git --with --some --options "$@"
  }
  test_expect_success '...' '
	# this one is expected to succeed
	foo ok &&
	# this one is not
	test_must_fail foo bad
  '

-Peff