[PATCH v2 0/7] Detailed test coverage reports for Git

This is v2 of the test coverage series. It addresses all the points
that were raised for v1. Here's the diffstat against v1:
    
     .gitignore |   15 +++------------
     Makefile   |   19 +++++++++----------
     t/README   |   21 ++++++++++++++++-----
     3 files changed, 28 insertions(+), 27 deletions(-)

And the diff since v2:
    
    diff --git a/.gitignore b/.gitignore
    index e02f1f9..baed247 100644
    --- a/.gitignore
    +++ b/.gitignore
    @@ -207,12 +207,3 @@
    -/*.gcda
    -/*.gcno
    -/*.gcov
    -/builtin/*.gcda
    -/builtin/*.gcno
    -/builtin/*.gcov
    -/xdiff/*.gcda
    -/xdiff/*.gcno
    -/xdiff/*.gcov
    -/compat/*.gcda
    -/compat/*.gcno
    -/compat/*.gcov
    +*.gcda
    +*.gcno
    +*.gcov
    diff --git a/Makefile b/Makefile
    index c35c348..b6975aa 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -2282,0 +2283 @@ coverage:
    +object_dirs := $(sort $(dir $(OBJECTS)))
    @@ -2284,4 +2285,3 @@ coverage-clean:
    -	$(RM) *.gcov *.gcda *.gcno
    -	$(RM) builtin/*.gcov
    -	$(RM) builtin/*.gcda
    -	$(RM) builtin/*.gcno
    +	$(RM) $(addsuffix *.gcov,$(object_dirs))
    +	$(RM) $(addsuffix *.gcda,$(object_dirs))
    +	$(RM) $(addsuffix *.gcno,$(object_dirs))
    @@ -2298,4 +2298,3 @@ coverage-report:
    -	gcov -b *.c
    -	gcov -b -o builtin builtin/*.c
    -	gcov -b -o xdiff xdiff/*.c
    -	gcov -b -o compat compat/*.c
    +	for dir in $(object_dirs); do \
    +		gcov --preserve-paths --branch-probabilities --all-blocks --object-directory=$$dir $$dir*.c; \
    +	done
    @@ -2303 +2302 @@ coverage-report:
    -coverage-report-untested-functions:
    +coverage-untested-functions: coverage-report
    @@ -2308 +2307 @@ coverage-report-untested-functions:
    -coverage-report-cover-db:
    +coverage-report-cover-db: coverage-report
    diff --git a/t/README b/t/README
    index 718f35d..400e2da 100644
    --- a/t/README
    +++ b/t/README
    @@ -273,0 +274,9 @@ Do:
    +   Don't blindly follow test coverage metrics, they're a good way to
    +   spot if you've missed something. If a new function you added
    +   doesn't have any coverage you're probably doing something wrong,
    +   but having 100% coverage doesn't necessarily mean that you tested
    +   everything.
    +
    +   Tests that are likely to smoke out future regressions are better
    +   than tests that just inflate the coverage metrics.
    +
    @@ -518,3 +527,5 @@ 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):
    +You can use the coverage tests to find code paths that are not being
    +used or properly exercised yet.
    +
    +To do that, run the coverage target at the top-level (not in the t/
    +directory):
    @@ -532 +543 @@ functions:
    -    make coverage-report-untested-functions
    +    make coverage-untested-functions
    @@ -537 +548 @@ Devel::Cover module. To install it do:
    -   # On Debian:
    +   # On Debian or Ubuntu:
    
I also rewrote some of the commit messages.
    
Ævar Arnfjörð Bjarmason (7):
  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
  t/README: Add a note about the dangers of coverage chasing

 .gitignore |    6 ++++++
 Makefile   |   17 +++++++++++++++--
 t/README   |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 2 deletions(-)

We generate profiling files in all the $(OBJECTS) dirs. Aggregate
results from there, and add them to the corresponding clean target.

Also expand the gcov arguments. Generate reports for things like "x()
|| y()" using --all-blocks, and add --preserve-paths since we're
profiling in subdirectories now.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 Makefile |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index bc3c570..a95d260 100644
--- a/Makefile
+++ b/Makefile

@@ -2280,8 +2280,11 @@ coverage:
 	$(MAKE) coverage-build
 	$(MAKE) coverage-report
 
+object_dirs := $(sort $(dir $(OBJECTS)))
 coverage-clean:
-	rm -f *.gcda *.gcno
+	$(RM) $(addsuffix *.gcov,$(object_dirs))
+	$(RM) $(addsuffix *.gcda,$(object_dirs))
+	$(RM) $(addsuffix *.gcno,$(object_dirs))
 
 COVERAGE_CFLAGS = $(CFLAGS) -O0 -ftest-coverage -fprofile-arcs
 COVERAGE_LDFLAGS = $(CFLAGS)  -O0 -lgcov

@@ -2292,7 +2295,9 @@ coverage-build: coverage-clean
 		-j1 test
 
 coverage-report:
-	gcov -b *.c
+	for dir in $(object_dirs); do \
+		gcov --preserve-paths --branch-probabilities --all-blocks --object-directory=$$dir $$dir*.c; \
+	done
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		| tee coverage-untested-functions

-- 
1.7.0.4

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

Change gitignore to ignore the *.gcda, *.gcno and *.gcov files
generated by GCC and our coverage invocations.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 .gitignore |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 14e2b6b..0e61845 100644
--- a/.gitignore
+++ b/.gitignore

@@ -204,3 +204,7 @@
 *.pdb
 /Debug/
 /Release/
+*.gcda
+*.gcno
+*.gcov
+/coverage-untested-functions

-- 
1.7.0.4

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 <redacted>
---
 .gitignore |    1 +
 Makefile   |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 0e61845..bd53c02 100644
--- a/.gitignore
+++ b/.gitignore

@@ -208,3 +208,4 @@
 *.gcno
 *.gcov
 /coverage-untested-functions
+/cover_db

diff --git a/Makefile b/Makefile
index e9f8ef8..f2c680d 100644
--- a/Makefile
+++ b/Makefile

@@ -2303,3 +2303,7 @@ coverage-untested-functions: coverage-report
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		| tee coverage-untested-functions
+
+coverage-report-cover-db: coverage-report
+	gcov2perl -db cover_db *.gcov
+

-- 
1.7.0.4

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 <redacted>
---
 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index a95d260..e9f8ef8 100644
--- a/Makefile
+++ b/Makefile

@@ -2298,6 +2298,8 @@ coverage-report:
 	for dir in $(object_dirs); do \
 		gcov --preserve-paths --branch-probabilities --all-blocks --object-directory=$$dir $$dir*.c; \
 	done
+
+coverage-untested-functions: coverage-report
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		| tee coverage-untested-functions

-- 
1.7.0.4

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.

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

diff --git a/.gitignore b/.gitignore
index bd53c02..baed247 100644
--- a/.gitignore
+++ b/.gitignore

@@ -209,3 +209,4 @@
 *.gcov
 /coverage-untested-functions
 /cover_db
+/cover_db_html

diff --git a/Makefile b/Makefile
index f2c680d..b6975aa 100644
--- a/Makefile
+++ b/Makefile

@@ -2307,3 +2307,5 @@ coverage-untested-functions: coverage-report
 coverage-report-cover-db: coverage-report
 	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

Document how test writers can generate coverage reports, to ensure
that their codepaths are being properly exercised.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/README |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..2fa6744 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,42 @@ 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 code paths that are not being
+used or properly exercised yet.
+
+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-untested-functions
+
+You can also generate a detailed per-file HTML report using the
+Devel::Cover module. To install it do:
+
+   # On Debian or Ubuntu:
+   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

Having no coverage at all is almost always a bad sign, but trying to
attain 100% coverage everywhere is usually a waste of time. Add a
paragraph to explain this to future test writers.

Inspired-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 t/README |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 2fa6744..400e2da 100644
--- a/t/README
+++ b/t/README

@@ -271,6 +271,15 @@ Do:
  - Check the test coverage for your tests. See the "Test coverage"
    below.
 
+   Don't blindly follow test coverage metrics, they're a good way to
+   spot if you've missed something. If a new function you added
+   doesn't have any coverage you're probably doing something wrong,
+   but having 100% coverage doesn't necessarily mean that you tested
+   everything.
+
+   Tests that are likely to smoke out future regressions are better
+   than tests that just inflate the coverage metrics.
+
 Don't:
 
  - exit() within a <script> part.

-- 
1.7.0.4

Ævar Arnfjörð Bjarmason wrote:

+   Don't blindly follow test coverage metrics

Hmph, that is just common sense, while “you should really not be
paying any attention to your code while writing tests” is not.  I even
prefer the text without this patch applied.  So forget I said anything;
I can find a way to hint at that in t/README later. :)

Jonathan

Hi,

Ævar Arnfjörð Bjarmason wrote:

Change gitignore to ignore the *.gcda, *.gcno and *.gcov files
generated by GCC and our coverage invocations.

Good idea.  The following is nitpicking of the worst kind; sorry.

It is easier to maintain frequently-changing lists like .gitignore
and the #include lines in source files if new additions go in some
logical place in the middle instead of the end.  There is less
lock contention that way. :)

So maybe:

diff --git i/.gitignore w/.gitignore
index 0e61845..57f79ef 100644
--- i/.gitignore
+++ w/.gitignore

@@ -181,6 +181,10 @@
 *.[aos]
 *.py[co]
 .depend/
+*.gcda
+*.gcno
+*.gcov
+/coverage-untested-functions
 *+
 /config.mak
 /autom4te.cache

@@ -204,7 +208,3 @@
 *.pdb
 /Debug/
 /Release/
-*.gcda
-*.gcno
-*.gcov
-/coverage-untested-functions

-- 

Ævar Arnfjörð Bjarmason wrote:

We generate profiling files in all the $(OBJECTS) dirs. Aggregate
results from there, and add them to the corresponding clean target.

Also expand the gcov arguments. Generate reports for things like "x()
|| y()" using --all-blocks, and add --preserve-paths since we're
profiling in subdirectories now.

All good things.  It might be good to add a GCOVFLAGS that can be
added to the "make" command line, then.

What about coverage-untested-functions?

[...]

This will not error out if "for" fails; maybe it would make sense to use
set -e or exit.

More importantly, it spews quite a lot of output:

| for dir in ./ block-sha1/ builtin/ compat/ xdiff/; do \
|                 gcov --preserve-paths --branch-probabilities --all-blocks --object-directory=$dir $dir*.c || exit; \
|         done
| ./abspath.gcno:version '405p', prefer '404*'
| ./abspath.gcda:version '405p', prefer version '404*'
| ./advice.gcno:version '405p', prefer '404*'
[...]
| ./ctype.gcno:cannot open graph file
[...]
| File 'wt-status.c'
| Lines executed:36.26% of 535
| Branches executed:39.78% of 279
| Taken at least once:20.43% of 279
| No calls
| wt-status.c:creating 'wt-status.c.gcov'
|
| File 'write_or_die.c'
[...]
| block-sha1/sha1.c:creating 'block-sha1#sha1.c.gcov'
|
| builtin/add.gcno:version '405p', prefer '404*'
[...]
| compat/basename.gcno:cannot open graph file
| compat/cygwin.gcno:cannot open graph file
[...]

Can gcov be convinced to be a little quieter (i.e., only useful warnings)?
(I don’t know; just asking.)

Here are the changes I squashed in for testing; please feel free to
take or leave what you like.

diff --git i/Makefile w/Makefile
index a95d260..b791ad5 100644
--- i/Makefile
+++ w/Makefile

@@ -1485,6 +1485,7 @@ ifndef V
 	QUIET_BUILT_IN = @echo '   ' BUILTIN $@;
 	QUIET_GEN      = @echo '   ' GEN $@;
 	QUIET_LNCP     = @echo '   ' LN/CP $@;
+	QUIET_GCOV     = @echo '   ' GCOV $@;
 	QUIET_SUBDIR0  = +@subdir=
 	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
 			 $(MAKE) $(PRINT_DIR) -C $$subdir

@@ -2285,9 +2286,11 @@ coverage-clean:
 	$(RM) $(addsuffix *.gcov,$(object_dirs))
 	$(RM) $(addsuffix *.gcda,$(object_dirs))
 	$(RM) $(addsuffix *.gcno,$(object_dirs))
+	$(RM) coverage-untested-functions
 
 COVERAGE_CFLAGS = $(CFLAGS) -O0 -ftest-coverage -fprofile-arcs
 COVERAGE_LDFLAGS = $(CFLAGS)  -O0 -lgcov
+GCOVFLAGS = --preserve-paths --branch-probabilities --all-blocks
 
 coverage-build: coverage-clean
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" all

@@ -2295,9 +2298,9 @@ coverage-build: coverage-clean
 		-j1 test
 
 coverage-report:
-	for dir in $(object_dirs); do \
-		gcov --preserve-paths --branch-probabilities --all-blocks --object-directory=$$dir $$dir*.c; \
+	$(QUIET_GCOV)for dir in $(object_dirs); do \
+		gcov $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
 	done
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
-		| tee coverage-untested-functions
+		> coverage-untested-functions

-- 

Ævar Arnfjörð Bjarmason wrote:

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.

Oh!  Apparently these are directories. ;-)

diff --git i/.gitignore w/.gitignore
index baed247..54280af 100644
--- i/.gitignore
+++ w/.gitignore

@@ -209,4 +209,4 @@
 *.gcov
 /coverage-untested-functions
 /cover_db
-/cover_db_html
+/cover_db_html/

diff --git i/Makefile w/Makefile
index b6975aa..0fdb434 100644
--- i/Makefile
+++ w/Makefile

@@ -2282,6 +2282,7 @@ coverage:
 
 object_dirs := $(sort $(dir $(OBJECTS)))
 coverage-clean:
+	$(RM) -r cover_db_html
 	$(RM) $(addsuffix *.gcov,$(object_dirs))
 	$(RM) $(addsuffix *.gcda,$(object_dirs))
 	$(RM) $(addsuffix *.gcno,$(object_dirs))

@@ -2307,5 +2308,5 @@ coverage-untested-functions: coverage-report
 coverage-report-cover-db: coverage-report
 	gcov2perl -db cover_db *.gcov
 
-coverage-report-cover-db-html: coverage-report-cover-db
+cover_db_html: coverage-report-cover-db
 	cover -report html -outputdir cover_db_html cover_db

-- 

Ævar Arnfjörð Bjarmason wrote:

Ævar Arnfjörð Bjarmason (7):
  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
  t/README: Add a note about the dangers of coverage chasing

With whatever subset of the changes I have hinted at seems
suitable,

Reviewed-by: Jonathan Nieder <redacted>

Thanks.

Ævar Arnfjörð Bjarmason wrote:

nitpick: trailing newline.

This target is not included in .PHONY and it does not seem to be
phony after all, anyway.  Why not use something like the following?

 cover_db: coverage-report
	gcov2perl...

It seems more intuitive to me, and once the makefile learns to track
how long a coverage-report remains valid it would allow avoiding a
rebuild of the cover_db.

diff --git i/Makefile w/Makefile
index f2c680d..c59740c 100644
--- i/Makefile
+++ w/Makefile

@@ -2282,6 +2282,7 @@ coverage:
 
 object_dirs := $(sort $(dir $(OBJECTS)))
 coverage-clean:
+	$(RM) cover_db
 	$(RM) $(addsuffix *.gcov,$(object_dirs))
 	$(RM) $(addsuffix *.gcda,$(object_dirs))
 	$(RM) $(addsuffix *.gcno,$(object_dirs))

@@ -2304,6 +2305,5 @@ coverage-untested-functions: coverage-report
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		| tee coverage-untested-functions
 
-coverage-report-cover-db: coverage-report
+cover_db: coverage-report
 	gcov2perl -db cover_db *.gcov
-

-- 

On Sun, Jul 25, 2010 at 17:20, Jonathan Nieder [off-list ref] wrote:

Ævar Arnfjörð Bjarmason wrote:

quoted

Ævar Arnfjörð Bjarmason (7):
  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
  t/README: Add a note about the dangers of coverage chasing

With whatever subset of the changes I have hinted at seems
suitable,

Reviewed-by: Jonathan Nieder <redacted>

All the changes you made look good, I approve of having them squashed
when this is applied. Thanks.

Ævar Arnfjörð Bjarmason wrote:

On Sun, Jul 25, 2010 at 17:20, Jonathan Nieder [off-list ref] wrote:

quoted

Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

Ævar Arnfjörð Bjarmason (7):
  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
  t/README: Add a note about the dangers of coverage chasing

[...]

All the changes you made look good, I approve of having them squashed
when this is applied. Thanks.

I hope not. :)  The change to coverage-clean suggested in my reply to
patch 4 needs at least a "-r" after the $(RM), since I had not noticed
that cover_db is a directory.

On Sun, Jul 25, 2010 at 16:05, Jonathan Nieder [off-list ref] wrote:

Ævar Arnfjörð Bjarmason wrote:

quoted

+   Don't blindly follow test coverage metrics

Hmph, that is just common sense,

You'd be surprised at how uncommon it is when people have 98% coverage
and try to painfully squeeze out that last 2% :)

while “you should really not be paying any attention to your code
while writing tests” is not.  I even prefer the text without this
patch applied.  So forget I said anything; I can find a way to hint
at that in t/README later. :)

I don't know whether it should be applied. I just wrote a short
summary in response to the previous commentary.