From: Ævar Arnfjörð Bjarmason Date: Sat, 24 Jul 2010 23:17:18 GMT Subject: Re: [PATCH 6/6] t/README: A new section about test coverage Message-ID: In-Reply-To: <20100724212542.GA5610@burratino> On Sat, Jul 24, 2010 at 21:25, Jonathan Nieder wrote: > Ævar Arnfjörð Bjarmason wrote: > >> Document how test writers can generate coverage reports > > Very neat! Thanks for the review. >> --- 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. >> @@ -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 > > Thanks. >