On Fri, Jan 15, 2021 at 10:39:17AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > 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
> 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
