{"thread":{"id":"24499","subject":"[PATCH 0/6] Detailed test coverage reports for Git","startedAt":"2010-07-24T20:50:57Z","lastAt":"2010-07-26T05:44:39Z","messageCount":19,"participants":["Ævar Arnfjörð Bjarmason","Jonathan Nieder","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"146206","messageId":"1280004663-4887-1-git-send-email-avarab@gmail.com","threadId":"24499","inReplyTo":null,"subject":"[PATCH 0/6] Detailed test coverage reports for Git","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T20:50:57Z","receivedAt":"2010-07-24T20:50:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Thomas Rast added initial test coverage support in 901c369af5. Expand\non that so that coverage is extended to C files in builtin/, xdiff/\nand compat/.\n\nIn addition I've added support for formatting the coverage reports\nwith gcov2perl and Devel::Cover. Here's an example report formatted\nwith these tools:\n\n    http://v.nix.is/~avar/cover_db_html/coverage.html\n\nWith it we can see that Git currently has 77.1% test coverage for its\ncore C code. It's also possible to dive in on a per-file basis,\ne.g. here you can see how sparse the tests for git-blame's -L option\nare, as I noted in a previous thread (and send partial patches):\n\n    http://v.nix.is/~avar/cover_db_html/builtin-blame-c.html\n\nI didn't yet look at how I could run the test suite so that we also\nget test coverage for our core Perl code. Devel::Cover obviously\nsupports that, but it's just a matter of running the tests with the\nright environmental variables, and merging the gcov + Devel::Cover\nreports.\n\nBut that's a project for another day.\n\nÆvar Arnfjörð Bjarmason (6):\n  gitignore: Ignore files generated by \"make coverage\"\n  Makefile: Include subdirectories in \"make cover\" reports\n  Makefile: Split out the untested functions target\n  Makefile: Add coverage-report-cover-db target\n  Makefile: Add coverage-report-cover-db-html target\n  t/README: A new section about test coverage\n\n .gitignore |   15 +++++++++++++++\n Makefile   |   16 +++++++++++++++-\n t/README   |   40 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 70 insertions(+), 1 deletions(-)\n"},{"id":"146208","messageId":"1280004663-4887-2-git-send-email-avarab@gmail.com","threadId":"24499","inReplyTo":"1280004663-4887-1-git-send-email-avarab@gmail.com","subject":"[PATCH 1/6] gitignore: Ignore files generated by \"make coverage\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T20:50:58Z","receivedAt":"2010-07-24T20:50:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"make coverage\" support added by Thomas Rast in 901c369af5 didn't\ncontain a corresponding patch to patch .gitignore.\n\nChange gitignore to ignore those. I'm not simply ignoring all *.gcda\n*.gcno *.gcov since I'd like to be surprised if they start showing up\nin unexpected places.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n .gitignore |   13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex 14e2b6b..f836a45 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -204,3 +204,16 @@\n *.pdb\n /Debug/\n /Release/\n+/*.gcda\n+/*.gcno\n+/*.gcov\n+/builtin/*.gcda\n+/builtin/*.gcno\n+/builtin/*.gcov\n+/xdiff/*.gcda\n+/xdiff/*.gcno\n+/xdiff/*.gcov\n+/compat/*.gcda\n+/compat/*.gcno\n+/compat/*.gcov\n+/coverage-untested-functions\n-- \n1.7.0.4\n"},{"id":"146207","messageId":"1280004663-4887-3-git-send-email-avarab@gmail.com","threadId":"24499","inReplyTo":"1280004663-4887-1-git-send-email-avarab@gmail.com","subject":"[PATCH 2/6] Makefile: Include subdirectories in \"make cover\" reports","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T20:50:59Z","receivedAt":"2010-07-24T20:50:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The buildin/, xdiff/ and compat/ subdirectories weren't being included\nin the gcov aggregation, nor were the files there being cleaned up.\n\nChanged rm -f to the $(RM) variable while I was at it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile |    8 +++++++-\n 1 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex bc3c570..63f3f84 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2281,7 +2281,10 @@ coverage:\n \t$(MAKE) coverage-report\n \n coverage-clean:\n-\trm -f *.gcda *.gcno\n+\t$(RM) *.gcov *.gcda *.gcno\n+\t$(RM) builtin/*.gcov\n+\t$(RM) builtin/*.gcda\n+\t$(RM) builtin/*.gcno\n \n COVERAGE_CFLAGS = $(CFLAGS) -O0 -ftest-coverage -fprofile-arcs\n COVERAGE_LDFLAGS = $(CFLAGS)  -O0 -lgcov\n@@ -2293,6 +2296,9 @@ coverage-build: coverage-clean\n \n coverage-report:\n \tgcov -b *.c\n+\tgcov -b -o builtin builtin/*.c\n+\tgcov -b -o xdiff xdiff/*.c\n+\tgcov -b -o compat compat/*.c\n \tgrep '^function.*called 0 ' *.c.gcov \\\n \t\t| sed -e 's/\\([^:]*\\)\\.gcov: *function \\([^ ]*\\) called.*/\\1: \\2/' \\\n \t\t| tee coverage-untested-functions\n-- \n1.7.0.4\n"},{"id":"146209","messageId":"1280004663-4887-4-git-send-email-avarab@gmail.com","threadId":"24499","inReplyTo":"1280004663-4887-1-git-send-email-avarab@gmail.com","subject":"[PATCH 3/6] Makefile: Split out the untested functions target","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T20:51:00Z","receivedAt":"2010-07-24T20:51:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the coverage-report target so that it doesn't generate the\ncoverage-untested-functions file by default. I'm adding more targets\nfor doing various things with the gcov files, and they shouldn't all\nrun by default.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 63f3f84..5e9a6a2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2299,6 +2299,8 @@ coverage-report:\n \tgcov -b -o builtin builtin/*.c\n \tgcov -b -o xdiff xdiff/*.c\n \tgcov -b -o compat compat/*.c\n+\n+coverage-report-untested-functions:\n \tgrep '^function.*called 0 ' *.c.gcov \\\n \t\t| sed -e 's/\\([^:]*\\)\\.gcov: *function \\([^ ]*\\) called.*/\\1: \\2/' \\\n \t\t| tee coverage-untested-functions\n-- \n1.7.0.4\n"},{"id":"146210","messageId":"1280004663-4887-5-git-send-email-avarab@gmail.com","threadId":"24499","inReplyTo":"1280004663-4887-1-git-send-email-avarab@gmail.com","subject":"[PATCH 4/6] Makefile: Add coverage-report-cover-db target","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T20:51:01Z","receivedAt":"2010-07-24T20:51:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a target to convert the *.gcov files to a Devel::Cover\ndatabase. That database can subsequently be formatted by the cover(1)\ntool which is included with Devel::Cover.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n .gitignore |    1 +\n Makefile   |    3 +++\n 2 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex f836a45..5e24b0b 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -217,3 +217,4 @@\n /compat/*.gcno\n /compat/*.gcov\n /coverage-untested-functions\n+/cover_db\ndiff --git a/Makefile b/Makefile\nindex 5e9a6a2..b15c894 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2304,3 +2304,6 @@ coverage-report-untested-functions:\n \tgrep '^function.*called 0 ' *.c.gcov \\\n \t\t| sed -e 's/\\([^:]*\\)\\.gcov: *function \\([^ ]*\\) called.*/\\1: \\2/' \\\n \t\t| tee coverage-untested-functions\n+\n+coverage-report-cover-db:\n+\tgcov2perl -db cover_db *.gcov\n-- \n1.7.0.4\n"},{"id":"146211","messageId":"1280004663-4887-6-git-send-email-avarab@gmail.com","threadId":"24499","inReplyTo":"1280004663-4887-1-git-send-email-avarab@gmail.com","subject":"[PATCH 5/6] Makefile: Add coverage-report-cover-db-html target","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T20:51:02Z","receivedAt":"2010-07-24T20:51:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a target to generate a detailed HTML report for the entire Git\ncodebase using Devel::Cover's cover(1) tool. Output it in\ncover_db_html instead of the default cover_db, so that it isn't mixed\nup with our raw report files.\n\nThe target depends on the coverage-report-cover-db target, it may be\nrun redundantly if it was previously run. But the HTML output won't be\naffected by running gcov2perl twice, so I didn't try to avoid that\nsmall redundancy.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n .gitignore |    1 +\n Makefile   |    3 +++\n 2 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex 5e24b0b..e02f1f9 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -218,3 +218,4 @@\n /compat/*.gcov\n /coverage-untested-functions\n /cover_db\n+/cover_db_html\ndiff --git a/Makefile b/Makefile\nindex b15c894..c35c348 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2307,3 +2307,6 @@ coverage-report-untested-functions:\n \n coverage-report-cover-db:\n \tgcov2perl -db cover_db *.gcov\n+\n+coverage-report-cover-db-html: coverage-report-cover-db\n+\tcover -report html -outputdir cover_db_html cover_db\n-- \n1.7.0.4\n"},{"id":"146212","messageId":"1280004663-4887-7-git-send-email-avarab@gmail.com","threadId":"24499","inReplyTo":"1280004663-4887-1-git-send-email-avarab@gmail.com","subject":"[PATCH 6/6] t/README: A new section about test coverage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T20:51:03Z","receivedAt":"2010-07-24T20:51:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Document how test writers can generate coverage reports, to ensure\nthat their tests are really testing the code they think they're\ntesting.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/README |   40 ++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 40 insertions(+), 0 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 0d1183c..718f35d 100644\n--- a/t/README\n+++ b/t/README\n@@ -267,6 +267,9 @@ Do:\n \tgit merge hla &&\n \tgit push gh &&\n \ttest ...\n+    \n+ - Check the test coverage for your tests. See the \"Test coverage\"\n+   below.\n \n Don't:\n \n@@ -508,3 +511,40 @@ the purpose of t0000-basic.sh, which is to isolate that level of\n validation in one place.  Your test also ends up needing\n updating when such a change to the internal happens, so do _not_\n do it and leave the low level of validation to t0000-basic.sh.\n+\n+Test coverage\n+-------------\n+\n+You can use the coverage tests to find out if your tests are really\n+testing your code code. To do that, run the coverage target at the\n+top-level (not in the t/ directory):\n+\n+    make coverage\n+    \n+That'll compile Git with GCC's coverage arguments, and generate a test\n+report with gcov after the tests finish. Running the coverage tests\n+can take a while, since running the tests in parallel is incompatible\n+with GCC's coverage mode.\n+\n+After the tests have run you can generate a list of untested\n+functions:\n+\n+    make coverage-report-untested-functions\n+\n+You can also generate a detailed per-file HTML report using the\n+Devel::Cover module. To install it do:\n+\n+   # On Debian:\n+   sudo aptitude install libdevel-cover-perl\n+\n+   # From the CPAN with cpanminus\n+   curl -L http://cpanmin.us | perl - --sudo --self-upgrade\n+   cpanm --sudo Devel::Cover\n+   \n+Then, at the top-level:\n+\n+    make coverage-report-cover-db-html\n+\n+That'll generate a detailed cover report in the \"cover_db_html\"\n+directory, which you can then copy to a webserver, or inspect locally\n+in a browser.\n-- \n1.7.0.4\n"},{"id":"146213","messageId":"20100724212542.GA5610@burratino","threadId":"24499","inReplyTo":"1280004663-4887-7-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 6/6] t/README: A new section about test coverage","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-24T21:25:43Z","receivedAt":"2010-07-24T21:25:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> Document how test writers can generate coverage reports\n\nVery neat!\n\n> --- a/t/README\n> +++ b/t/README\n> @@ -267,6 +267,9 @@ Do:\n>  \tgit merge hla &&\n>  \tgit push gh &&\n>  \ttest ...\n> +    \n> + - Check the test coverage for your tests. See the \"Test coverage\"\n> +   below.\n>  \n>  Don't:\n\nI have a moment’s hesitation reading this, because I suspect test\ncoverage checking would be most useful if test authors were _not_ to\npay too much attention to it.\n\nImagine that the git test suite is almost perfect, so it checks all\nthe important behavior of git, including edge cases (yes, unlikely,\nbut bear with me for a moment).  Then the test coverage data would be\nvery useful indeed: it would point out code that is not actually\nneeded for anything.\n\nHowever, if new authors make 99% coverage a goal while writing\ntests, the result will be lots of useless tests that check\nbehavior no one cares about and less useful coverage information.\n\n> @@ -508,3 +511,40 @@ the purpose of t0000-basic.sh, which is to isolate that level of\n>  validation in one place.  Your test also ends up needing\n>  updating when such a change to the internal happens, so do _not_\n>  do it and leave the low level of validation to t0000-basic.sh.\n> +\n> +Test coverage\n> +-------------\n> +\n> +You can use the coverage tests to find out if your tests are really\n> +testing your code code. To do that, run the coverage target at the\n> +top-level (not in the t/ directory):\n\nIn other words, I would rather the rationale here read:\n\n\tYou can use the coverage tests to find code paths that are not being\n\tproperly exercised yet. To do that...\n\nI think it is great if people write new tests that do not exercise\ntheir own code but instead explore related behavior.\n\nThat said, with or without any of the changes implied above,\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"146214","messageId":"20100724212907.GA5664@burratino","threadId":"24499","inReplyTo":"20100724212542.GA5610@burratino","subject":"Re: [PATCH 6/6] t/README: A new section about test coverage","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-24T21:29:07Z","receivedAt":"2010-07-24T21:29:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> \tYou can use the coverage tests to find code paths that are not being\n> \tproperly exercised yet. To do that...\n\ner, that should have read \"used or properly exercised\" rather than\n\"properly exercised yet\".\n\nSorry for the noise.\nJonathan\n"},{"id":"146219","messageId":"201007250037.57086.trast@student.ethz.ch","threadId":"24499","inReplyTo":"1280004663-4887-3-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 2/6] Makefile: Include subdirectories in \"make cover\" reports","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-24T22:37:56Z","receivedAt":"2010-07-24T22:37:56Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> The buildin/, xdiff/ and compat/ subdirectories weren't being included\n> in the gcov aggregation, nor were the files there being cleaned up.\n[...]\n>  coverage-clean:\n> -\trm -f *.gcda *.gcno\n> +\t$(RM) *.gcov *.gcda *.gcno\n> +\t$(RM) builtin/*.gcov\n> +\t$(RM) builtin/*.gcda\n> +\t$(RM) builtin/*.gcno\n\nBy the same logic, the xdiff and compat directories should also be\nincluded here.  Maybe also block-sha1?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"146220","messageId":"201007250101.26577.trast@student.ethz.ch","threadId":"24499","inReplyTo":"1280004663-4887-5-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 4/6] Makefile: Add coverage-report-cover-db target","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-24T23:01:26Z","receivedAt":"2010-07-24T23:01:26Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> +\n> +coverage-report-cover-db:\n> +\tgcov2perl -db cover_db *.gcov\n\nI think this either needs a dependency or .PHONY.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"146221","messageId":"201007250102.20099.trast@student.ethz.ch","threadId":"24499","inReplyTo":"1280004663-4887-4-git-send-email-avarab@gmail.com","subject":"Re: [PATCH 3/6] Makefile: Split out the untested functions target","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-07-24T23:02:19Z","receivedAt":"2010-07-24T23:02:19Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> Change the coverage-report target so that it doesn't generate the\n> coverage-untested-functions file by default. I'm adding more targets\n> for doing various things with the gcov files, and they shouldn't all\n> run by default.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  Makefile |    2 ++\n>  1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> diff --git a/Makefile b/Makefile\n> index 63f3f84..5e9a6a2 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2299,6 +2299,8 @@ coverage-report:\n>  \tgcov -b -o builtin builtin/*.c\n>  \tgcov -b -o xdiff xdiff/*.c\n>  \tgcov -b -o compat compat/*.c\n> +\n> +coverage-report-untested-functions:\n>  \tgrep '^function.*called 0 ' *.c.gcov \\\n>  \t\t| sed -e 's/\\([^:]*\\)\\.gcov: *function \\([^ ]*\\) called.*/\\1: \\2/' \\\n>  \t\t| tee coverage-untested-functions\n\nThis should depend on coverage-report, and either have its name\nchanged to coverage-untested-functions or be .PHONY.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"146222","messageId":"AANLkTim7v199lMKNEhOALx_38ChIIgvD4oERQJU5SNdv@mail.gmail.com","threadId":"24499","inReplyTo":"20100724212542.GA5610@burratino","subject":"Re: [PATCH 6/6] t/README: A new section about test coverage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T23:17:18Z","receivedAt":"2010-07-24T23:17:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Jul 24, 2010 at 21:25, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ævar Arnfjörð Bjarmason wrote:\n>\n>> Document how test writers can generate coverage reports\n>\n> Very neat!\n\nThanks for the review.\n\n>> --- a/t/README\n>> +++ b/t/README\n>> @@ -267,6 +267,9 @@ Do:\n>>       git merge hla &&\n>>       git push gh &&\n>>       test ...\n>> +\n>> + - Check the test coverage for your tests. See the \"Test coverage\"\n>> +   below.\n>>\n>>  Don't:\n>\n> I have a moment’s hesitation reading this, because I suspect test\n> coverage checking would be most useful if test authors were _not_ to\n> pay too much attention to it.\n>\n> Imagine that the git test suite is almost perfect, so it checks all\n> the important behavior of git, including edge cases (yes, unlikely,\n> but bear with me for a moment).  Then the test coverage data would be\n> very useful indeed: it would point out code that is not actually\n> needed for anything.\n>\n> However, if new authors make 99% coverage a goal while writing\n> tests, the result will be lots of useless tests that check\n> behavior no one cares about and less useful coverage information.\n\nWhat I was going for here is that you should try to make sure that the\ncode you're adding is covered by tests by running the coverage tests.\n\nI.e. if I add a new function \"blah\" to git-whatever which is\nimplemented by the \"do_blah\" function checking if every line of\n\"do_blah\" is covered is an excellent indicator of whether that code is\nbeing exhaustively tested, as opposed to just superficially tested.\n\nIn most cases a low test coverage counts is telling about the overall\nquality of the tests.\n\nBut, the wording can probably be improved. Do you have a suggestion\nfor the above intent compressed into a sentence or two? I can't come\nup with anything right now.\n\n>> @@ -508,3 +511,40 @@ the purpose of t0000-basic.sh, which is to isolate that level of\n>>  validation in one place.  Your test also ends up needing\n>>  updating when such a change to the internal happens, so do _not_\n>>  do it and leave the low level of validation to t0000-basic.sh.\n>> +\n>> +Test coverage\n>> +-------------\n>> +\n>> +You can use the coverage tests to find out if your tests are really\n>> +testing your code code. To do that, run the coverage target at the\n>> +top-level (not in the t/ directory):\n>\n> In other words, I would rather the rationale here read:\n>\n>        You can use the coverage tests to find code paths that are not being\n>        properly exercised yet. To do that...\n>\n> I think it is great if people write new tests that do not exercise\n> their own code but instead explore related behavior.\n\nThat wording is better, thanks.\n\n> That said, with or without any of the changes implied above,\n>\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n> Thanks.\n>\n"},{"id":"146223","messageId":"AANLkTim8UIiplPxUdcEWnDpevOUUZARgObhIFCkfO3zl@mail.gmail.com","threadId":"24499","inReplyTo":"201007250037.57086.trast@student.ethz.ch","subject":"Re: [PATCH 2/6] Makefile: Include subdirectories in \"make cover\" reports","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T23:28:02Z","receivedAt":"2010-07-24T23:28:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Jul 24, 2010 at 22:37, Thomas Rast <trast@student.ethz.ch> wrote:\n> Ævar Arnfjörð Bjarmason wrote:\n>> The buildin/, xdiff/ and compat/ subdirectories weren't being included\n>> in the gcov aggregation, nor were the files there being cleaned up.\n> [...]\n>>  coverage-clean:\n>> -     rm -f *.gcda *.gcno\n>> +     $(RM) *.gcov *.gcda *.gcno\n>> +     $(RM) builtin/*.gcov\n>> +     $(RM) builtin/*.gcda\n>> +     $(RM) builtin/*.gcno\n>\n> By the same logic, the xdiff and compat directories should also be\n> included here.  Maybe also block-sha1?\n\nYeah, actually now that I think about it any C code we compile could\nspew those *.gcda *.gcno files, which means:\n\n    $ find . -type f -name '*.c'| ack '^(.*/.*)/[^/]+$' --output '$1'|sort|uniq\n    ./block-sha1\n    ./builtin\n    ./compat\n    ./compat/fnmatch\n    ./compat/nedmalloc\n    ./compat/regex\n    ./compat/win32\n    ./contrib/convert-objects\n    ./contrib/examples\n    ./contrib/svn-fe\n    ./ppc\n    ./xdiff\n\nMaybe it would be better to just put:\n\n    *.gcda\n    *.gcno\n\nInto .gitignore, and leave it to the user to clean these with git\nclean -dxf or something.\n"},{"id":"146224","messageId":"AANLkTincK7b3gLVUfD5cfvU6OOeTwyLcd0o2U5LSYp-L@mail.gmail.com","threadId":"24499","inReplyTo":"201007250101.26577.trast@student.ethz.ch","subject":"Re: [PATCH 4/6] Makefile: Add coverage-report-cover-db target","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T23:28:57Z","receivedAt":"2010-07-24T23:28:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Jul 24, 2010 at 23:01, Thomas Rast <trast@student.ethz.ch> wrote:\n> Ævar Arnfjörð Bjarmason wrote:\n>> +\n>> +coverage-report-cover-db:\n>> +     gcov2perl -db cover_db *.gcov\n>\n> I think this either needs a dependency or .PHONY.\n\nWill fix, thanks.\n"},{"id":"146225","messageId":"AANLkTikB_QV6NoaNkubfM267tZvjYN7LzMNTFkwW-eEO@mail.gmail.com","threadId":"24499","inReplyTo":"201007250102.20099.trast@student.ethz.ch","subject":"Re: [PATCH 3/6] Makefile: Split out the untested functions target","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-07-24T23:29:20Z","receivedAt":"2010-07-24T23:29:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Jul 24, 2010 at 23:02, Thomas Rast <trast@student.ethz.ch> wrote:\n> Ævar Arnfjörð Bjarmason wrote:\n>> Change the coverage-report target so that it doesn't generate the\n>> coverage-untested-functions file by default. I'm adding more targets\n>> for doing various things with the gcov files, and they shouldn't all\n>> run by default.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>  Makefile |    2 ++\n>>  1 files changed, 2 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/Makefile b/Makefile\n>> index 63f3f84..5e9a6a2 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -2299,6 +2299,8 @@ coverage-report:\n>>       gcov -b -o builtin builtin/*.c\n>>       gcov -b -o xdiff xdiff/*.c\n>>       gcov -b -o compat compat/*.c\n>> +\n>> +coverage-report-untested-functions:\n>>       grep '^function.*called 0 ' *.c.gcov \\\n>>               | sed -e 's/\\([^:]*\\)\\.gcov: *function \\([^ ]*\\) called.*/\\1: \\2/' \\\n>>               | tee coverage-untested-functions\n>\n> This should depend on coverage-report, and either have its name\n> changed to coverage-untested-functions or be .PHONY.\n\nWill fix, thanks.\n"},{"id":"146226","messageId":"20100724233232.GB6374@burratino","threadId":"24499","inReplyTo":"AANLkTim7v199lMKNEhOALx_38ChIIgvD4oERQJU5SNdv@mail.gmail.com","subject":"Re: [PATCH 6/6] t/README: A new section about test coverage","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-24T23:32:32Z","receivedAt":"2010-07-24T23:32:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n>> Ævar Arnfjörð Bjarmason wrote:\n\n>>> + - Check the test coverage for your tests. See the \"Test coverage\"\n>>> +   below.\n[...]\n> What I was going for here is that you should try to make sure that the\n> code you're adding is covered by tests by running the coverage tests.\n> \n> I.e. if I add a new function \"blah\" to git-whatever which is\n> implemented by the \"do_blah\" function checking if every line of\n> \"do_blah\" is covered is an excellent indicator of whether that code is\n> being exhaustively tested, as opposed to just superficially tested.\n> \n> In most cases a low test coverage counts is telling about the overall\n> quality of the tests.\n> \n> But, the wording can probably be improved. Do you have a suggestion\n> for the above intent compressed into a sentence or two? I can't come\n> up with anything right now.\n\nWhat I meant is that when developing a new feature, I think paying\ntoo much attention to coverage numbers is a very dangerous thing.\n\nIt produces two hazards: too many tests and too few tests.\n\n - too many tests because when I write my \"do_blah\" function\n   that is about a case no one cares about in practice, to write\n   artificial tests to exercise would actually be to do harm.\n\n - too few tests because if I focus on testing all the code I\n   just wrote, then I am very unlikely to include tests for the\n   cases I did /not/ write code for.  Some important cases are\n   just easy; we should still test them because that will help\n   if the code is ever refactored later.  Some important cases\n   may be just not implemented yet; test cases for them are\n   very helpful indeed to readers and future implementers.\n\nIn other words, I would rather that when writing tests, authors would\nforget about the implementation for a moment and just think about what\na user wants to do.\n\nThat said:\n\nThe rest of the time, checking test coverage does provide a very good\nindication about what features might not be well tested yet.  So it is\nstill a good way to decide where to /start/ writing tests.\n\nPlus it’s great fun to look at. :)\n\nThanks,\nJonathan\n"},{"id":"146227","messageId":"20100724234105.GA14277@burratino","threadId":"24499","inReplyTo":"AANLkTim8UIiplPxUdcEWnDpevOUUZARgObhIFCkfO3zl@mail.gmail.com","subject":"Re: [PATCH 2/6] Makefile: Include subdirectories in \"make cover\" reports","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-24T23:41:05Z","receivedAt":"2010-07-24T23:41:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Sat, Jul 24, 2010 at 22:37, Thomas Rast <trast@student.ethz.ch> wrote:\n>> Ævar Arnfjörð Bjarmason wrote:\n\n>>>  coverage-clean:\n>>> -     rm -f *.gcda *.gcno\n>>> +     $(RM) *.gcov *.gcda *.gcno\n>>> +     $(RM) builtin/*.gcov\n>>> +     $(RM) builtin/*.gcda\n>>> +     $(RM) builtin/*.gcno\n>>\n>> By the same logic, the xdiff and compat directories should also be\n>> included here.  Maybe also block-sha1?\n>\n> Yeah, actually now that I think about it any C code we compile could\n> spew those *.gcda *.gcno files, which means:\n\nYou can find a list of directories where the Makefile was thinking about\nbuilding things with \"dirs := $(sort $(dir $(OBJECTS)))\".  See\ndep_dirs for an example.\n\nHope that helps,\nJonathan\n"},{"id":"146345","messageId":"7vk4oiajaw.fsf@alter.siamese.dyndns.org","threadId":"24499","inReplyTo":"20100724234105.GA14277@burratino","subject":"Re: [PATCH 2/6] Makefile: Include subdirectories in \"make cover\" reports","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-26T05:44:39Z","receivedAt":"2010-07-26T05:44:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> You can find a list of directories where the Makefile was thinking about\n> building things with \"dirs := $(sort $(dir $(OBJECTS)))\".  See\n> dep_dirs for an example.\n\nNeat-o ;-).\n"}]}