[PATCH 0/6] Detailed test coverage reports for Git

[PATCH 0/6] Detailed test coverage reports for Git

[Ævar Arnfjörð Bjarmason]

Thomas Rast added initial test coverage support in 901c369af5. Expand
on that so that coverage is extended to C files in builtin/, xdiff/
and compat/.

In addition I've added support for formatting the coverage reports
with gcov2perl and Devel::Cover. Here's an example report formatted
with these tools:

    http://v.nix.is/~avar/cover_db_html/coverage.html

With it we can see that Git currently has 77.1% test coverage for its
core C code. It's also possible to dive in on a per-file basis,
e.g. here you can see how sparse the tests for git-blame's -L option
are, as I noted in a previous thread (and send partial patches):

    http://v.nix.is/~avar/cover_db_html/builtin-blame-c.html

I didn't yet look at how I could run the test suite so that we also
get test coverage for our core Perl code. Devel::Cover obviously
supports that, but it's just a matter of running the tests with the
right environmental variables, and merging the gcov + Devel::Cover
reports.

But that's a project for another day.

Ævar Arnfjörð Bjarmason (6):
  gitignore: Ignore files generated by "make coverage"
  Makefile: Include subdirectories in "make cover" reports
  Makefile: Split out the untested functions target
  Makefile: Add coverage-report-cover-db target
  Makefile: Add coverage-report-cover-db-html target
  t/README: A new section about test coverage

 .gitignore |   15 +++++++++++++++
 Makefile   |   16 +++++++++++++++-
 t/README   |   40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 1 deletions(-)

[PATCH 1/6] gitignore: Ignore files generated by "make coverage"

[Ævar Arnfjörð Bjarmason]

The "make coverage" support added by Thomas Rast in 901c369af5 didn't
contain a corresponding patch to patch .gitignore.

Change gitignore to ignore those. I'm not simply ignoring all *.gcda
*.gcno *.gcov since I'd like to be surprised if they start showing up
in unexpected places.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 14e2b6b..f836a45 100644
--- a/.gitignore
+++ b/.gitignore
@@ -204,3 +204,16 @@
 *.pdb
 /Debug/
 /Release/
+/*.gcda
+/*.gcno
+/*.gcov
+/builtin/*.gcda
+/builtin/*.gcno
+/builtin/*.gcov
+/xdiff/*.gcda
+/xdiff/*.gcno
+/xdiff/*.gcov
+/compat/*.gcda
+/compat/*.gcno
+/compat/*.gcov
+/coverage-untested-functions
-- 
1.7.0.4

[PATCH 2/6] Makefile: Include subdirectories in "make cover" reports

[Ævar Arnfjörð Bjarmason]

The buildin/, xdiff/ and compat/ subdirectories weren't being included
in the gcov aggregation, nor were the files there being cleaned up.

Changed rm -f to the $(RM) variable while I was at it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index bc3c570..63f3f84 100644
--- a/Makefile
+++ b/Makefile
@@ -2281,7 +2281,10 @@ coverage:
 	$(MAKE) coverage-report
 
 coverage-clean:
-	rm -f *.gcda *.gcno
+	$(RM) *.gcov *.gcda *.gcno
+	$(RM) builtin/*.gcov
+	$(RM) builtin/*.gcda
+	$(RM) builtin/*.gcno
 
 COVERAGE_CFLAGS = $(CFLAGS) -O0 -ftest-coverage -fprofile-arcs
 COVERAGE_LDFLAGS = $(CFLAGS)  -O0 -lgcov
@@ -2293,6 +2296,9 @@ coverage-build: coverage-clean
 
 coverage-report:
 	gcov -b *.c
+	gcov -b -o builtin builtin/*.c
+	gcov -b -o xdiff xdiff/*.c
+	gcov -b -o compat compat/*.c
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		| tee coverage-untested-functions
-- 
1.7.0.4

[PATCH 3/6] Makefile: Split out the untested functions target

[Ævar Arnfjörð Bjarmason]

Change the coverage-report target so that it doesn't generate the
coverage-untested-functions file by default. I'm adding more targets
for doing various things with the gcov files, and they shouldn't all
run by default.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 63f3f84..5e9a6a2 100644
--- a/Makefile
+++ b/Makefile
@@ -2299,6 +2299,8 @@ coverage-report:
 	gcov -b -o builtin builtin/*.c
 	gcov -b -o xdiff xdiff/*.c
 	gcov -b -o compat compat/*.c
+
+coverage-report-untested-functions:
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		| tee coverage-untested-functions
-- 
1.7.0.4

[PATCH 4/6] Makefile: Add coverage-report-cover-db target

[Ævar Arnfjörð Bjarmason]

Add a target to convert the *.gcov files to a Devel::Cover
database. That database can subsequently be formatted by the cover(1)
tool which is included with Devel::Cover.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore |    1 +
 Makefile   |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index f836a45..5e24b0b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -217,3 +217,4 @@
 /compat/*.gcno
 /compat/*.gcov
 /coverage-untested-functions
+/cover_db
diff --git a/Makefile b/Makefile
index 5e9a6a2..b15c894 100644
--- a/Makefile
+++ b/Makefile
@@ -2304,3 +2304,6 @@ coverage-report-untested-functions:
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		| tee coverage-untested-functions
+
+coverage-report-cover-db:
+	gcov2perl -db cover_db *.gcov
-- 
1.7.0.4

[PATCH 5/6] Makefile: Add coverage-report-cover-db-html target

[Ævar Arnfjörð Bjarmason]

Add a target to generate a detailed HTML report for the entire Git
codebase using Devel::Cover's cover(1) tool. Output it in
cover_db_html instead of the default cover_db, so that it isn't mixed
up with our raw report files.

The target depends on the coverage-report-cover-db target, it may be
run redundantly if it was previously run. But the HTML output won't be
affected by running gcov2perl twice, so I didn't try to avoid that
small redundancy.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore |    1 +
 Makefile   |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 5e24b0b..e02f1f9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -218,3 +218,4 @@
 /compat/*.gcov
 /coverage-untested-functions
 /cover_db
+/cover_db_html
diff --git a/Makefile b/Makefile
index b15c894..c35c348 100644
--- a/Makefile
+++ b/Makefile
@@ -2307,3 +2307,6 @@ coverage-report-untested-functions:
 
 coverage-report-cover-db:
 	gcov2perl -db cover_db *.gcov
+
+coverage-report-cover-db-html: coverage-report-cover-db
+	cover -report html -outputdir cover_db_html cover_db
-- 
1.7.0.4

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

[Ævar Arnfjörð Bjarmason]

Document how test writers can generate coverage reports, to ensure
that their tests are really testing the code they think they're
testing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..718f35d 100644
--- 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:
 
@@ -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):
+
+    make coverage
+    
+That'll compile Git with GCC's coverage arguments, and generate a test
+report with gcov after the tests finish. Running the coverage tests
+can take a while, since running the tests in parallel is incompatible
+with GCC's coverage mode.
+
+After the tests have run you can generate a list of untested
+functions:
+
+    make coverage-report-untested-functions
+
+You can also generate a detailed per-file HTML report using the
+Devel::Cover module. To install it do:
+
+   # On Debian:
+   sudo aptitude install libdevel-cover-perl
+
+   # From the CPAN with cpanminus
+   curl -L http://cpanmin.us | perl - --sudo --self-upgrade
+   cpanm --sudo Devel::Cover
+   
+Then, at the top-level:
+
+    make coverage-report-cover-db-html
+
+That'll generate a detailed cover report in the "cover_db_html"
+directory, which you can then copy to a webserver, or inspect locally
+in a browser.
-- 
1.7.0.4

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

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Document how test writers can generate coverage reports

Very neat!

> --- 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.

> @@ -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 said, with or without any of the changes implied above,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

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

[Jonathan Nieder]

Jonathan Nieder wrote:

> 	You can use the coverage tests to find code paths that are not being
> 	properly exercised yet. To do that...

er, that should have read "used or properly exercised" rather than
"properly exercised yet".

Sorry for the noise.
Jonathan

Re: [PATCH 2/6] Makefile: Include subdirectories in "make cover" reports

[Thomas Rast]

Ævar Arnfjörð Bjarmason wrote:
> The buildin/, xdiff/ and compat/ subdirectories weren't being included
> in the gcov aggregation, nor were the files there being cleaned up.
[...]
>  coverage-clean:
> -	rm -f *.gcda *.gcno
> +	$(RM) *.gcov *.gcda *.gcno
> +	$(RM) builtin/*.gcov
> +	$(RM) builtin/*.gcda
> +	$(RM) builtin/*.gcno

By the same logic, the xdiff and compat directories should also be
included here.  Maybe also block-sha1?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 4/6] Makefile: Add coverage-report-cover-db target

[Thomas Rast]

Ævar Arnfjörð Bjarmason wrote:
> +
> +coverage-report-cover-db:
> +	gcov2perl -db cover_db *.gcov

I think this either needs a dependency or .PHONY.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 3/6] Makefile: Split out the untested functions target

[Thomas Rast]

Ævar Arnfjörð Bjarmason wrote:
> Change the coverage-report target so that it doesn't generate the
> coverage-untested-functions file by default. I'm adding more targets
> for doing various things with the gcov files, and they shouldn't all
> run by default.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Makefile |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 63f3f84..5e9a6a2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2299,6 +2299,8 @@ coverage-report:
>  	gcov -b -o builtin builtin/*.c
>  	gcov -b -o xdiff xdiff/*.c
>  	gcov -b -o compat compat/*.c
> +
> +coverage-report-untested-functions:
>  	grep '^function.*called 0 ' *.c.gcov \
>  		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
>  		| tee coverage-untested-functions

This should depend on coverage-report, and either have its name
changed to coverage-untested-functions or be .PHONY.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Ævar Arnfjörð Bjarmason]

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.
>

Re: [PATCH 2/6] Makefile: Include subdirectories in "make cover" reports

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 24, 2010 at 22:37, Thomas Rast <trast@student.ethz.ch> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> The buildin/, xdiff/ and compat/ subdirectories weren't being included
>> in the gcov aggregation, nor were the files there being cleaned up.
> [...]
>>  coverage-clean:
>> -     rm -f *.gcda *.gcno
>> +     $(RM) *.gcov *.gcda *.gcno
>> +     $(RM) builtin/*.gcov
>> +     $(RM) builtin/*.gcda
>> +     $(RM) builtin/*.gcno
>
> By the same logic, the xdiff and compat directories should also be
> included here.  Maybe also block-sha1?

Yeah, actually now that I think about it any C code we compile could
spew those *.gcda *.gcno files, which means:

    $ find . -type f -name '*.c'| ack '^(.*/.*)/[^/]+$' --output '$1'|sort|uniq
    ./block-sha1
    ./builtin
    ./compat
    ./compat/fnmatch
    ./compat/nedmalloc
    ./compat/regex
    ./compat/win32
    ./contrib/convert-objects
    ./contrib/examples
    ./contrib/svn-fe
    ./ppc
    ./xdiff

Maybe it would be better to just put:

    *.gcda
    *.gcno

Into .gitignore, and leave it to the user to clean these with git
clean -dxf or something.

Re: [PATCH 4/6] Makefile: Add coverage-report-cover-db target

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 24, 2010 at 23:01, Thomas Rast <trast@student.ethz.ch> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> +
>> +coverage-report-cover-db:
>> +     gcov2perl -db cover_db *.gcov
>
> I think this either needs a dependency or .PHONY.

Will fix, thanks.

Re: [PATCH 3/6] Makefile: Split out the untested functions target

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 24, 2010 at 23:02, Thomas Rast <trast@student.ethz.ch> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> Change the coverage-report target so that it doesn't generate the
>> coverage-untested-functions file by default. I'm adding more targets
>> for doing various things with the gcov files, and they shouldn't all
>> run by default.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  Makefile |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 63f3f84..5e9a6a2 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2299,6 +2299,8 @@ coverage-report:
>>       gcov -b -o builtin builtin/*.c
>>       gcov -b -o xdiff xdiff/*.c
>>       gcov -b -o compat compat/*.c
>> +
>> +coverage-report-untested-functions:
>>       grep '^function.*called 0 ' *.c.gcov \
>>               | sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
>>               | tee coverage-untested-functions
>
> This should depend on coverage-report, and either have its name
> changed to coverage-untested-functions or be .PHONY.

Will fix, thanks.

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

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
>> Ævar Arnfjörð Bjarmason wrote:

>>> + - Check the test coverage for your tests. See the "Test coverage"
>>> +   below.
[...]
> 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.

What I meant is that when developing a new feature, I think paying
too much attention to coverage numbers is a very dangerous thing.

It produces two hazards: too many tests and too few tests.

 - too many tests because when I write my "do_blah" function
   that is about a case no one cares about in practice, to write
   artificial tests to exercise would actually be to do harm.

 - too few tests because if I focus on testing all the code I
   just wrote, then I am very unlikely to include tests for the
   cases I did /not/ write code for.  Some important cases are
   just easy; we should still test them because that will help
   if the code is ever refactored later.  Some important cases
   may be just not implemented yet; test cases for them are
   very helpful indeed to readers and future implementers.

In other words, I would rather that when writing tests, authors would
forget about the implementation for a moment and just think about what
a user wants to do.

That said:

The rest of the time, checking test coverage does provide a very good
indication about what features might not be well tested yet.  So it is
still a good way to decide where to /start/ writing tests.

Plus it’s great fun to look at. :)

Thanks,
Jonathan

Re: [PATCH 2/6] Makefile: Include subdirectories in "make cover" reports

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Sat, Jul 24, 2010 at 22:37, Thomas Rast <trast@student.ethz.ch> wrote:
>> Ævar Arnfjörð Bjarmason wrote:

>>>  coverage-clean:
>>> -     rm -f *.gcda *.gcno
>>> +     $(RM) *.gcov *.gcda *.gcno
>>> +     $(RM) builtin/*.gcov
>>> +     $(RM) builtin/*.gcda
>>> +     $(RM) builtin/*.gcno
>>
>> By the same logic, the xdiff and compat directories should also be
>> included here.  Maybe also block-sha1?
>
> Yeah, actually now that I think about it any C code we compile could
> spew those *.gcda *.gcno files, which means:

You can find a list of directories where the Makefile was thinking about
building things with "dirs := $(sort $(dir $(OBJECTS)))".  See
dep_dirs for an example.

Hope that helps,
Jonathan

Re: [PATCH 2/6] Makefile: Include subdirectories in "make cover" reports

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> You can find a list of directories where the Makefile was thinking about
> building things with "dirs := $(sort $(dir $(OBJECTS)))".  See
> dep_dirs for an example.

Neat-o ;-).