Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

From: Jeff King <peff@peff.net>
Date: Aug 19, 2018, 20:32 UTC
Message-ID: <20180819203253.GA5789@sigill.intra.peff.net>
In-Reply-To: <73346b91-6d19-651a-c361-1666a39681f0@gmail.com>

On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:

Show 28 quoted lines

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