On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
> > I actually think the above gives way too confusing output, when the
> > actual output is empty and we are expecting some output.
> >
> > The tester wants to hear from test_cmp "your 'git cmd' produced some
> > output when we are expecting none" as the primary message. We are
> > trying to find bugs in "git" under development, and diagnosing iffy
> > tests is secondary. But with your change, the first thing that is
> > checked is if 'expect' is an empty file and that is what we get
> > complaints about, without even looking at what is in 'actual'.
>
> I came up with two solutions for this issue:
>
> 1. Check both files at the same time (combination with Gábor's
> function):
>
> test_cmp () {
> if test "$1" != - &&
> test "$2" != - &&
> ! test -s "$1" &&
> ! test -s "$2"
> then
> error "bug in test script: using test_cmp to check empty file; use test_must_be_empty instead"
> fi
> test_cmp_allow_empty "$@"
> }
>
> This will still be reporting to the developer clearly, but
> will only catch cases exactly like the bogus test in t5310.
Doesn't that have the opposite issue? If we expect non-empty output but
the command produces empty output, we'd say "bug in the test script".
But that is not true at all; it's a failed test.
If we assume that "expect" is first (which is our convention but not
necessarily guaranteed), then I think the best logic is something like:
if $1 is empty; then
bug in the test script
elif test_cmp_allow_empty "$@"
test failure
We do not need to check $2 at all. An empty one is either irrelevant (if
the expectation is empty), or a test failure (because it would not match
the non-empty $1).
If we go that route, we should make sure that test_cmp's documentation
is updated to mention that the two sides are not symmetric (and possibly
update some callers, though I'd be OK leaving ones that aren't broken by
this, and just clean them up over time).
-Peff
