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, 21:43 UTC
- Message-ID
- <20180819214334.GA2626@sigill.intra.peff.net>
- In-Reply-To
- <aedb7f33-65cc-5d63-4e7e-dd5f37831e19@gmail.com>
On Sun, Aug 19, 2018 at 11:37:59PM +0200, Andrei Rybak wrote:
Show 26 quoted lines
> On 19/08/18 22:32, Jeff King wrote:
> > 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:Oh, you're right. Somewhere between the screen and my brain the "&&" became an "||".
I agree that works, and has the advantage that the arguments are treated symmetrically. We _might_ say "test failure" instead of "bug in test" when the expectation is empty and the generated output is not (because we do not know which is which), but I think that would be uncommon (and the most important thing is that we do not silently consider it a pass).
Show 14 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:
I'd be OK pursuing either this line, or what you showed originally.
-Peff