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 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 <redacted>
---
Makefile | 8 +++++++-
1 files changed, 7 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 <redacted>
---
.gitignore | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
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(-)
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 | 3 +++
2 files changed, 4 insertions(+), 0 deletions(-)
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 <redacted>
---
.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 <redacted>
---
t/README | 40 ++++++++++++++++++++++++++++++++++++++++
1 files changed, 40 insertions(+), 0 deletions(-)
@@ -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.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:10
Ævar Arnfjörð Bjarmason wrote:
Document how test writers can generate coverage reports
Very neat!
quoted hunk
--- 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.
quoted hunk
@@ -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 <redacted>
Thanks.
From: Thomas Rast <hidden> Date: 2016-06-15 22:49:10
Ævar Arnfjörð Bjarmason wrote:
quoted hunk
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(-)
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
On Sat, Jul 24, 2010 at 21:25, Jonathan Nieder [off-list ref] wrote:
Ævar Arnfjörð Bjarmason wrote:
quoted
Document how test writers can generate coverage reports
Very neat!
Thanks for the review.
quoted
--- 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.
quoted
@@ -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 <redacted>
Thanks.
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.
On Sat, Jul 24, 2010 at 23:02, Thomas Rast [off-list ref] wrote:
Ævar Arnfjörð Bjarmason wrote:
quoted
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(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:10
Ævar Arnfjörð Bjarmason wrote:
quoted
Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
+ - 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
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
From: Junio C Hamano <hidden> Date: 2016-06-15 22:49:11
Jonathan Nieder [off-list ref] 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.