Re: [RFC][PATCH] index-pack: add testcases found using AFL

From: Jeff King <peff@peff.net>
Date: Mar 12, 2017, 12:32 UTC
Message-ID: <20170312123212.3rnqyx3dvi5yppk5@sigill.intra.peff.net>
In-Reply-To: <CACBZZX5fGU9C-z94KbMAs_AegOSGtq8nbrkRe-NxBCHYsDswkA@mail.gmail.com>

On Fri, Mar 10, 2017 at 11:58:13PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 24 quoted lines

> >> I observe the following coverage changes (for t5300 only):
> >>
> >>   path                  old%  new%    pp
> >>   ----------------------------------------
> >>   builtin/index-pack.c  74.3  76.6   2.3
> >>   pack-write.c          79.8  80.4    .6
> >>   patch-delta.c         67.4  81.4  14.0
> >>   usage.c               26.6  35.5   8.9
> >>   wrapper.c             42.0  46.1   4.1
> >>   zlib.c                58.7  64.1   5.4
> >
> > I'm not sure how I feel about this. More coverage is good, I guess, but
> > we don't have any idea what these packfiles are doing, or whether
> > index-pack is behaving sanely in the new lines. The most we can say is
> > that we tested more lines of code and that nothing segfaulted or
> > triggered something like ASAN.
> 
> Isn't the main value with these sorts of tests that they make up the
> difference in the current manually maintained coverage & some
> randomized coverage. So when you change the code in the future and the
> randomized coverage changes, we don't know if that's a good or a bad
> thing, but at least we're more likely to know that it changed, and at
> that point someone's likely to actually investigate the root cause,
> which'll turn some AFL blob testcase into an isolated testcase?

I think you may be more optimistic than me in people actually looking at our coverage numbers. There is a "make coverage" target, but I don't think its output is useful for anybody trying to make comparison metrics.

One further devil's advocate:

If people really _do_ care about coverage, arguably the AFL tests are a pollution of that concept. Because they are running the code, but doing a very perfunctory job of testing it. IOW, our coverage of "code that doesn't segfault or trigger ASAN" is improved, but our coverage of "code that has been tested to be correct" is not (and since the tests are lumped together, it's hard to get anything but one number).

So I dunno. I remain on the fence about the patch.

-Peff