Re: [PATCH 6/6] t/README: A new section about test coverage

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Jul 24, 2010 at 21:25, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> Document how test writers can generate coverage reports
>
> Very neat!

Thanks for the review.

>> --- a/t/README
>> +++ b/t/README
>> @@ -267,6 +267,9 @@ Do:
>>       git merge hla &&
>>       git push gh &&
>>       test ...
>> +
>> + - Check the test coverage for your tests. See the "Test coverage"
>> +   below.
>>
>>  Don't:
>
> I have a moment’s hesitation reading this, because I suspect test
> coverage checking would be most useful if test authors were _not_ to
> pay too much attention to it.
>
> Imagine that the git test suite is almost perfect, so it checks all
> the important behavior of git, including edge cases (yes, unlikely,
> but bear with me for a moment).  Then the test coverage data would be
> very useful indeed: it would point out code that is not actually
> needed for anything.
>
> However, if new authors make 99% coverage a goal while writing
> tests, the result will be lots of useless tests that check
> behavior no one cares about and less useful coverage information.

What I was going for here is that you should try to make sure that the
code you're adding is covered by tests by running the coverage tests.

I.e. if I add a new function "blah" to git-whatever which is
implemented by the "do_blah" function checking if every line of
"do_blah" is covered is an excellent indicator of whether that code is
being exhaustively tested, as opposed to just superficially tested.

In most cases a low test coverage counts is telling about the overall
quality of the tests.

But, the wording can probably be improved. Do you have a suggestion
for the above intent compressed into a sentence or two? I can't come
up with anything right now.

>> @@ -508,3 +511,40 @@ the purpose of t0000-basic.sh, which is to isolate that level of
>>  validation in one place.  Your test also ends up needing
>>  updating when such a change to the internal happens, so do _not_
>>  do it and leave the low level of validation to t0000-basic.sh.
>> +
>> +Test coverage
>> +-------------
>> +
>> +You can use the coverage tests to find out if your tests are really
>> +testing your code code. To do that, run the coverage target at the
>> +top-level (not in the t/ directory):
>
> In other words, I would rather the rationale here read:
>
>        You can use the coverage tests to find code paths that are not being
>        properly exercised yet. To do that...
>
> I think it is great if people write new tests that do not exercise
> their own code but instead explore related behavior.

That wording is better, thanks.

> That said, with or without any of the changes implied above,
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks.
>