patch, 6 partsDetailed test coverage reports for Git

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(-)

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(-)

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(-)

Ævar Arnfjörð Bjarmason wrote:

[...]

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

On Sat, Jul 24, 2010 at 22:37, Thomas Rast <trast@student.ethz.ch> wrote:

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.

Ævar Arnfjörð Bjarmason wrote:

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

Jonathan Nieder <jrnieder@gmail.com> writes:

Neat-o ;-).

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(-)

Ævar Arnfjörð Bjarmason wrote:

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

On Sat, Jul 24, 2010 at 23:02, Thomas Rast <trast@student.ethz.ch> wrote:

Will fix, thanks.

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(-)

Ævar Arnfjörð Bjarmason wrote:

I think this either needs a dependency or .PHONY.

On Sat, Jul 24, 2010 at 23:01, Thomas Rast <trast@student.ethz.ch> wrote:

Will fix, thanks.

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(-)

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(-)

Ævar Arnfjörð Bjarmason wrote:

Very neat!

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.

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.

Jonathan Nieder wrote:

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

Sorry for the noise. Jonathan

On Sat, Jul 24, 2010 at 21:25, Jonathan Nieder <jrnieder@gmail.com> wrote:

Thanks for the review.

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.

That wording is better, thanks.

Ævar Arnfjörð Bjarmason wrote:

[...]

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