Re: Git Test Coverage Report (v2.20.0-rc0)

From: Jeff King <peff@peff.net>
Date: Nov 20, 2018, 12:40 UTC
Message-ID: <20181120124033.GA7596@sigill.intra.peff.net>
In-Reply-To: <80690e4b-4fc4-7846-009f-59dee9fbdd86@gmail.com>

On Tue, Nov 20, 2018 at 07:17:46AM -0500, Derrick Stolee wrote:

Show 14 quoted lines

> > And I think that's a pattern with the delta-island code. What we really
> > care about most is that if we throw a real fork-network repository at
> > it, it produces faster clones with fewer un-reusable deltas. So I think
> > a much more interesting approach here would be perf tests. But:
> > 
> >    - we'd want to count those as coverage, and that likely makes your
> >      coverage tests prohibitively expensive
> > 
> >    - it requires a specialized repo to demonstrate, which most people
> >      aren't going to have handy
> 
> Do you have regularly-running tests that check this in your infrastructure?
> As long as someone would notice if this code starts failing, that would be
> enough.

We do integration tests, and this feature gets exercised quite a bit in production at GitHub. But none of that caught the breakage I fixed this morning for the simple fact that we don't keep up with upstream master in real-time. We're running the delta-island code I wrote years ago, and the bug was introduced by the upstreaming. Probably in a month or three I'll merge v2.20 into our fork, and we would have noticed then.

I've had dreams of following upstream more closely, but there are two blockers:

  - we have enough custom bits that the merges are time-consuming (and
    introduce the possibility of funny interactions or regressions). I'd
    like to reduce our overall diff from upstream, but it's slow going
    and time is finite. (And ironically, it's made harder in some ways
    by the lag, because many of our topics are backports of things I
    send upstream).

  - deploying the ".0" release from master can be scary. For instance, I
    was really happy to have a fix for 9ac3f0e5b3 (pack-objects: fix
    performance issues on packing large deltas, 2018-07-22) before
    putting the bug into production.

Show 9 quoted lines

> > Yeah, this triggers if your regex has more than one capture group (and
> > likewise, we almost certainly don't run the "you have too many groups"
> > warning).
> 
> Did you know that regexes are notoriously under-tested [1]? When looking at
> this code, I didn't even know regexes were involved (but I didn't look
> enough at the context).
> 
> [1] https://dl.acm.org/citation.cfm?id=3236072

Heh. Well, fortunately we are compiling regexes from the user's config, so it's not Git's fault if the regexes are bad. ;)

(Of course on our production servers I'm on the hook for both sides!)

-Peff