On Sat, Jul 24, 2010 at 21:25, Jonathan Nieder <jrnieder@xxxxxxxxx> 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 <jrnieder@xxxxxxxxx> > > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html