Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test
- From
Andrei Rybak <rybak.a.v@gmail.com>
- Date
- Aug 19, 2018, 21:37 UTC
- Message-ID
- <aedb7f33-65cc-5d63-4e7e-dd5f37831e19@gmail.com>
- In-Reply-To
- <20180819203253.GA5789@sigill.intra.peff.net>
On 19/08/18 22:32, Jeff King wrote:
Show 22 quoted lines
> On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
>
>> 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.No. Only when both "$1" and "$2" are empty files will the function above report "bug in test script". From patch's commit message:
... both invocations produce empty 'pack{a,b}.objects' files, and the
subsequent 'test_cmp' happily finds those two empty files identical.That's what I meant by "will only catch cases exactly like the bogus test in t5310".
However ...
Show 11 quoted lines
> 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).
... this is indeed a better solution. I written out the cases for updated test_cmp to straighten out my thinking:
* both $1 and $2 are empty:
bogus test:
needs either fixing generation of both expect and actual
or switching to test_must_be_empty
OR
bogus helper function, as Gábor described above:
needs to switch to test_cmp_allow_empty
* $1 is non-empty && $2 is empty
proceeding with test
test failure from GIT_TEST_CMP
* $1 is empty && $2 is non-empty
bogus test - needs either switching to test_must_be_empty
(and after that test_must_be_empty will report failure)
or fixing generation of expect (and after that test result
depends on contents).
* both $1 and $2 are non-empty
proceeding with test
result depends on contents