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

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

[Ævar Arnfjörð Bjarmason]

Here's v3 of the patch. It's almost entirely changes from Jonathan
Nieder mentioned in the review for v2 (minus the bugs). Thanks
Jonathan!

Here's the stat from v2:
   
    .gitignore |   12 ++++++------
    Makefile   |   15 ++++++++++-----
    t/README   |    8 ++++----
    3 files changed, 20 insertions(+), 15 deletions(-)
   
And the diff:
    
    diff --git a/.gitignore b/.gitignore
    index baed247..0a30a7e 100644
    --- a/.gitignore
    +++ b/.gitignore
    @@ -183,0 +184,6 @@
    +*.gcda
    +*.gcno
    +*.gcov
    +/coverage-untested-functions
    +/cover_db/
    +/cover_db_html/
    @@ -207,6 +212,0 @@
    -*.gcda
    -*.gcno
    -*.gcov
    -/coverage-untested-functions
    -/cover_db
    -/cover_db_html
    diff --git a/Makefile b/Makefile
    index b6975aa..fdfa4fe 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -1487,0 +1488 @@ ifndef V
    +	QUIET_GCOV     = @echo '   ' GCOV $@;
    @@ -2287,0 +2289,3 @@ coverage-clean:
    +	$(RM) coverage-untested-functions
    +	$(RM) -r cover_db/
    +	$(RM) -r cover_db_html/
    @@ -2290,0 +2295 @@ COVERAGE_LDFLAGS = $(CFLAGS)  -O0 -lgcov
    +GCOVFLAGS = --preserve-paths --branch-probabilities --all-blocks
    @@ -2298,2 +2303,2 @@ 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; \
    @@ -2305 +2310 @@ coverage-untested-functions: coverage-report
    -		| tee coverage-untested-functions
    +		> coverage-untested-functions
    @@ -2307 +2312 @@ coverage-untested-functions: coverage-report
    -coverage-report-cover-db: coverage-report
    +cover_db: coverage-report
    @@ -2310 +2315 @@ coverage-report-cover-db: coverage-report
    -coverage-report-cover-db-html: coverage-report-cover-db
    +cover_db_html: cover_db
    diff --git a/t/README b/t/README
    index 400e2da..4fe8d50 100644
    --- a/t/README
    +++ b/t/README
    @@ -270 +270 @@ Do:
    -    
    +
    @@ -534 +534 @@ directory):
    -    
    +
    @@ -554 +554 @@ Devel::Cover module. To install it do:
    -   
    +
    @@ -557 +557 @@ Then, at the top-level:
    -    make coverage-report-cover-db-html
    +    make cover_db_html
    
Æ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   |   24 +++++++++++++++++++++---
 t/README   |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 3 deletions(-)

[PATCH v3 1/7] 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 the *.gcda, *.gcno and *.gcov files
generated by GCC and our coverage invocations.

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

diff --git a/.gitignore b/.gitignore
index 14e2b6b..57f79ef 100644
--- a/.gitignore
+++ b/.gitignore
@@ -181,6 +181,10 @@
 *.[aos]
 *.py[co]
 .depend/
+*.gcda
+*.gcno
+*.gcov
+/coverage-untested-functions
 *+
 /config.mak
 /autom4te.cache
-- 
1.7.0.4

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

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index bc3c570..b791ad5 100644
--- a/Makefile
+++ b/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
@@ -2280,11 +2281,16 @@ 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))
+	$(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
@@ -2292,7 +2298,9 @@ coverage-build: coverage-clean
 		-j1 test
 
 coverage-report:
-	gcov -b *.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
-- 
1.7.0.4

[PATCH v3 3/7] 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 b791ad5..40453f1 100644
--- a/Makefile
+++ b/Makefile
@@ -2301,6 +2301,8 @@ coverage-report:
 	$(QUIET_GCOV)for dir in $(object_dirs); do \
 		gcov $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
 	done
+
+coverage-untested-functions: coverage-report
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		> coverage-untested-functions
-- 
1.7.0.4

[PATCH v3 4/7] 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   |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 57f79ef..d8bd555 100644
--- a/.gitignore
+++ b/.gitignore
@@ -185,6 +185,7 @@
 *.gcno
 *.gcov
 /coverage-untested-functions
+/cover_db/
 *+
 /config.mak
 /autom4te.cache
diff --git a/Makefile b/Makefile
index 40453f1..1d721ef 100644
--- a/Makefile
+++ b/Makefile
@@ -2287,6 +2287,7 @@ coverage-clean:
 	$(RM) $(addsuffix *.gcda,$(object_dirs))
 	$(RM) $(addsuffix *.gcno,$(object_dirs))
 	$(RM) coverage-untested-functions
+	$(RM) -r cover_db/
 
 COVERAGE_CFLAGS = $(CFLAGS) -O0 -ftest-coverage -fprofile-arcs
 COVERAGE_LDFLAGS = $(CFLAGS)  -O0 -lgcov
@@ -2306,3 +2307,6 @@ coverage-untested-functions: coverage-report
 	grep '^function.*called 0 ' *.c.gcov \
 		| sed -e 's/\([^:]*\)\.gcov: *function \([^ ]*\) called.*/\1: \2/' \
 		> coverage-untested-functions
+
+cover_db: coverage-report
+	gcov2perl -db cover_db *.gcov
-- 
1.7.0.4

[PATCH v3 5/7] 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   |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index d8bd555..0a30a7e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -186,6 +186,7 @@
 *.gcov
 /coverage-untested-functions
 /cover_db/
+/cover_db_html/
 *+
 /config.mak
 /autom4te.cache
diff --git a/Makefile b/Makefile
index 1d721ef..fdfa4fe 100644
--- a/Makefile
+++ b/Makefile
@@ -2288,6 +2288,7 @@ coverage-clean:
 	$(RM) $(addsuffix *.gcno,$(object_dirs))
 	$(RM) coverage-untested-functions
 	$(RM) -r cover_db/
+	$(RM) -r cover_db_html/
 
 COVERAGE_CFLAGS = $(CFLAGS) -O0 -ftest-coverage -fprofile-arcs
 COVERAGE_LDFLAGS = $(CFLAGS)  -O0 -lgcov
@@ -2310,3 +2311,6 @@ coverage-untested-functions: coverage-report
 
 cover_db: coverage-report
 	gcov2perl -db cover_db *.gcov
+
+cover_db_html: cover_db
+	cover -report html -outputdir cover_db_html cover_db
-- 
1.7.0.4

[PATCH v3 6/7] 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 |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..15d4b52 100644
--- a/t/README
+++ b/t/README
@@ -268,6 +268,9 @@ Do:
 	git push gh &&
 	test ...
 
+ - Check the test coverage for your tests. See the "Test coverage"
+   below.
+
 Don't:
 
  - exit() within a <script> part.
@@ -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 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

[PATCH v3 7/7] t/README: Add a note about the dangers of coverage chasing

[Ævar Arnfjörð Bjarmason]

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 <jrnieder@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Jonathan doesn't particularly like this one. I have no particular
preference, just trying to get across the point that you shouldn't
cargo-cult-coverage.

 t/README |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index 15d4b52..4fe8d50 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

Re: [PATCH v3 0/7] Detailed test coverage reports for Git

[Thomas Rast]

Æ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

Thanks a lot for shaping this up!  I need the little patch below to
accommodate my addiction for a newer compiler than the one standard
for my distribution (but that was an issue before, too).  It works
very well, and the HTML output is quite nice.

Acked-by: Thomas Rast <trast@{inf,student}.ethz.ch>

-- 8< --
Subject: [PATCH] Makefile: make gcov invocation configurable

If you customize CC to use a different version of gcc, most likely you
also need to use a different version of gcov.  Make it configurable.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Makefile |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index fdfa4fe..81cb5ac 100644
--- a/Makefile
+++ b/Makefile
@@ -308,6 +308,7 @@ TCL_PATH = tclsh
 TCLTK_PATH = wish
 PTHREAD_LIBS = -lpthread
 PTHREAD_CFLAGS =
+GCOV = gcov
 
 export TCL_PATH TCLTK_PATH
 
@@ -2301,7 +2302,7 @@ coverage-build: coverage-clean
 
 coverage-report:
 	$(QUIET_GCOV)for dir in $(object_dirs); do \
-		gcov $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
+		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
 	done
 
 coverage-untested-functions: coverage-report
-- 
1.7.2.289.g93c76

Re: [PATCH v3 0/7] Detailed test coverage reports for Git

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 26, 2010 at 07:43, Thomas Rast <trast@student.ethz.ch> wrote:
> Æ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
>
> Thanks a lot for shaping this up!  I need the little patch below to
> accommodate my addiction for a newer compiler than the one standard
> for my distribution (but that was an issue before, too).  It works
> very well, and the HTML output is quite nice.

Thanks for the fixup. I ack that right back at you. Looks good.