Re: Git Test Coverage Report (Thursday, Aug 15)

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 20, 2019, 12:54 UTC
Message-ID: <a081ccdb-bb0d-00a1-136a-c8fa54296639@gmail.com>
In-Reply-To: <20190816181103.GA13894@sigill.intra.peff.net>

On 8/16/2019 2:11 PM, Jeff King wrote:

Show 5 quoted lines

> On Thu, Aug 15, 2019 at 10:01:04PM -0400, Derrick Stolee wrote:
> 
>> Here is today's test coverage report.
> 
> Are the scripts you use to generate these available somewhere?

The scripts I originally used are available in contrib/coverage-diff.sh.

However, to allow for some more advanced workflows, I created the following C# project: https://github.com/derrickstolee/git-test-coverage/tree/master/TestCoverageReport

The runs are done via the following Azure Pipelines build: https://dev.azure.com/git/git/_build/results?buildId=879

There, you can see how the code is built, tested, and the gcov output
is collected in the TestCoverageReport tool to generate the .txt and .htm
files.

Show 13 quoted lines

> I think it's useful to look at uncovered code, but I often struggle to
> figure out whether the parts attached to my name are relevant. In
> particular, I think two changes to the report format might help:
> 
>   1. Include names alongside commit ids when listing uncovered lines. I
>      know that will end up with some overly-long lines, but it makes it
>      easy to grep for one's name to find relevant sections of the file
>      (as opposed to finding your name at the bottom and
>      cross-referencing with actual content lines).
> 
>      Seeing that an uncovered line is a BUG(), for example, makes it
>      easy to know that it's not really an interesting uncovered case in
>      the code.

The HTML reports [1] have the following feature: click on a commit in the commit summary and it highlights the lines from that commit.

[1] https://derrickstolee.github.io/git-test-coverage/reports/2019-08-15.htm

However, the plain-text report _could_ add names. Maybe instead the report should just group the output by commit instead of branch.

Show 23 quoted lines

>   2. Include more context. Just taking a random example from this email:
> 
>> builtin/rebase.c
>> e191cc8b 129) strbuf_addstr(&buf, strategy_opts);
> 
> We know what the uncovered line was trying to do, but more interesting
> is likely the conditional that causes it to be uncovered. In this case
> the surrounding code is:
> 
>           if (opts->ignore_whitespace) {
>                   struct strbuf buf = STRBUF_INIT;
>   
>                   if (strategy_opts)
>                           strbuf_addstr(&buf, strategy_opts);
>   
>                   strbuf_addstr(&buf, " --ignore-space-change");
>                   free(strategy_opts);
>                   strategy_opts = strbuf_detach(&buf, NULL);
>           }
> 
> even the usual 3 lines of diff context would make it a lot quicker to
> understand what's going on (it only kicks in when multiple strategy
> options are used).

In this case, the additional context helps, but only if you expand by several lines. In other cases, the necessary context could be dozens of lines.

Perhaps a deeper report could include something like "git format-patch --function-context" to naturally extend to the appropriate context. It may be better to have a "block context" for these conditionals. In such a case, it would be important to mark the "new" lines explicitly so the context lines don't become too noisy.

Show 8 quoted lines

> (As an aside, this code leaks the newly allocated buffer and leaves a
> dangling pointer in opts->strategy_opts, but that's all orthogonal to
> the uncovered line; I'll send a separate message to the original
> author).
> 
> 
> Anyway, I wonder if we could adjust the output of the script to make
> reading it that way a bit easier.

I'll think about this, for sure. When I have time to go deep into the report (not just the code I wrote) I look for lines that seem to be non-trivial, then go find their context by looking at the commit. Here is another example of why the HTML report is helpful: it will link to the commit diff view [2] or the exact line of the file [3]. (Note: the exact line link only works if we point to the branch that was tested, not the commit that made the change.)

[2] https://github.com/git/git/commit/e191cc8b8080f63a0080031bf1276269c6bf42dd [3] https://github.com/git/git/blob/6cf2e4a04610efe882eb663cd36436b80b343fa0/builtin/rebase.c#L129

Thanks, -Stolee