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

From: Eric Sunshine <sunshine@sunshineco.com>
To: gitgitgadget@gmail.com
Cc: Git List <git@vger.kernel.org>, Jeff King <peff@peff.net>,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v2 1/1] contrib: add coverage-diff script
Date: Thu, 13 Sep 2018 13:54:37 -0400	[thread overview]
Message-ID: <CAPig+cQeA9UhVEAz-4TgMD4K6FsFk5tL0AH_uSjXApt888q25g@mail.gmail.com> (raw)
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:
> 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.