Re: [PATCH v2 1/1] contrib: add coverage-diff script

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 13, 2018, 17:54 UTC
Message-ID: <CAPig+cQeA9UhVEAz-4TgMD4K6FsFk5tL0AH_uSjXApt888q25g@mail.gmail.com>
In-Reply-To: <7714b0659e3210e34d0904b3347473427546d15c.1536850601.git.gitgitgadget@gmail.com>

On Thu, Sep 13, 2018 at 10:56 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 5 quoted lines

> There have been a few bugs in recent patches what would have been caught
> if the test suite covered those blocks (including a few of mine). I want
> to work towards a "sensible" amount of coverage on new topics. In my opinion,
> this means that any logic should be covered, but the 'die()' blocks in error
> cases do not need to be covered.

The bit about die() blocks is perhaps a bit too general. While it's true that some die()'s signal very unlikely (or near-impossible) conditions, others are merely reporting invalid user or other input to the program. The latter category is often very much worth testing, as the number of test_must_fail() invocations in the test suite shows. 68a6b3a1bd (worktree: teach 'move' to override lock when --force given twice, 2018-08-28), which was highlighted in your cover letter, provides a good example of legitimately testing that a die() is covered. So, perhaps the above can be toned-down a bit by saying something like:

    ...but die() blocks covering very unlikely (or near-impossible)
    situations may not warrant coverage.

> It is important to not measure the coverage of the codebase by what old code
> is not covered.