Re: Git Test Coverage Report for v2.37.0-rc0

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 15, 2022, 13:21 UTC
Message-ID: <604c694e-7b8f-40bd-c8da-93b00dfe3541@github.com>
In-Reply-To: <220615.86mteexls9.gmgdl@evledraar.gmail.com>

On 6/15/22 6:16 AM, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> 
> On Tue, Jun 14 2022, Derrick Stolee wrote:
> 
> Neat, thanks! Re git-test-coverage: Maybe it's covered somewhere, but is
> there tooling to reproduce this somewhere? I.e. we have "make coverage"
> in tree, but not this "do the diff and blame on lines" step, unless I've
> missed it somewhere...

There is a script at contrib/coverage-diff.sh, but I then created a separate project [1] that I could iterate on more quickly for new formats (like the commit-grouped output you see here).

I was running builds on Azure Pipelines [2], but they started failing a couple years ago (I think because of timeouts, but maybe also because of failures in 'seen'). I tried resurrecting them recently, but failed. So, I just ran this locally on my own machine.

[1] https://github.com/derrickstolee/git-test-coverage [2] https://dev.azure.com/git/git/_build?definitionId=12

Show 14 quoted lines

>>> Ævar Arnfjörð Bjarmason	fd3aaf53 run-command: add an "ungroup" option to run_process_parallel()
>>> run-command.c
>>> fd3aaf53 1645) code = pp->start_failure(pp->ungroup ? NULL :
>>> fd3aaf53 1646)  &pp->children[i].err,
>>> fd3aaf53 1649) if (!pp->ungroup) {
>>> fd3aaf53 1650) strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
>>> fd3aaf53 1651) strbuf_reset(&pp->children[i].err);
>>
>> This change seems sufficiently complicated that it would be good to look
>> into the test coverage here. It's possible that it is covered by the
>> GIT_TEST_* variables that I didn't use when generating my test coverage.
> 
> We should definitely have test coverage for these, but FWIW it's all
> pre-existing-not-tested code.

Yes, I agree that we should not hold up new changes just because old code isn't tested properly. It is worth considering whether we can add tests easily _or_ if we should just give the new code another look.

Show 12 quoted lines

> I.e. this has presumably been untested as far back as c553c72eed6
> (run-command: add an asynchronous parallel child processor,
> 2015-12-15). It's just:
> 
> 	if (start_command(&pp->children[i].process))
>         	/* this whole thing is untested, as no "start command" fails */
> 
> There's probably no easy way to automate this, but for significant bonus
> points I think a script like this would be much more useful if once it
> identifies given commit:line pairs it would check out $that_commit^, run
> "make coverage" again, see how pre-image faired (presumably --word-diff
> would associate them), and post that diff on top.

I think it's worth pointing out which lines are changing untested code.

Two core principles in "Working Effectively with Legacy Code" by Michael Feathers [3] are:

1. Untested code is legacy code.
2. Add tests before changing legacy code.

[3] https://www.oreilly.com/library/view/working-effectively-with/0131177052/

The point of this report is to see how well we are fitting with those principles, but also to take a pragmatic approach to deciding whether new tests are worth the effort.

Show 10 quoted lines

> I see in this case it's as easy as tweaking t0061-run-command.sh to do:
> 
>     test-tool run-command run-command-parallel 5 this-command-does-not-exist
> 
> I.e. we just don't stress that failure path, but should.
> 
> I'm planning to submit a cleanup series for the "ungroup" feature,
> i.e. the API is weird because we were trying to come up with a minimal
> regression fix. I'll make sure to include tests for this scenario in
> that series. Thanks!

Thank you!

It might also be worth pointing out that the 'coverage-test' build disables threading due to colliding in the *.gcov output. That leads to untested code that is actually tested by the "real" test suite.

-Stolee