include cross-package coverage in codecov #210
Conversation
In a recent go-car PR, we noticed that codecov was complaining about an uncovered line, when it was definitely being covered by a test. The problem was that the test was in another package. We could be strict and only count coverage within each package. However, what's most useful is to count the aggregate coverage across all packages in a module. If a module only achieves 70% coverage within each of its packages, but achieves 90% coverage when including cross-package coverage, the latter number is what is actually most interesting to us. Achieving full coverage within each package is perhaps ideal, but certainly not a requirement to have good coverage. Plus, this change will reduce "uncovered line" noise that can be reasonably considered a false positive.
|
LGTM, but the merge target should be next, not master. |
|
Ah, I did commit this on top of next, but my "post PR" script just assumes I'm always targetting the default branch :) |
|
PTAL |
|
That's what I meant with the "ideal" part of the commit message: ideally, each package would have enough tests to cover itself well, say by at least 90%. In practice however, in go-car/v2 we have the blockstore package that uses the index package. The blockstore package has very extensive tests, which already exercise >70% of the code in the index package. So, in my opinion, it's just wrong for codecov to say that the index package has near-zero test coverage. It's also confusing to the user if they are told "this line in the index package is not covered", when if you add a panic, it makes I think either the current or new way of accumulating coverage are reasonable, depending on your expectations and how the information is used. If you prefer higher test quality, you probably want to not include cross-package coverage - but I think many PL repositories like go-car or storetheindex will take the practical approach of prioritizing integration tests to cover more code at once. And, with the way codecov says "line is not covered", I think including cross-package coverage is the better choice to avoid confusing the user :) |
(see commit message)