[Twisted-Python] Coverage exceptions

[Glyph Lefkowitz]

> On Jun 30, 2016, at 04:13, Jean-Paul Calderone <exarkun at twistedmatrix.com> wrote:
> 
> On Thu, Jun 30, 2016 at 6:43 AM, Adi Roiban <adi at roiban.ro <mailto:adi at roiban.ro>> wrote:
> Hi,
> 
> Recently we have introduced a hard check of 100% coverage for all changes.
> This is done via coverage + codecov + github protected branches.
> 
> Now, if your patch is not 100% covered github will not let you merge it.
> 
> See for example this change: https://github.com/twisted/twisted/pull/261/files#diff-0fea8a8ca713deb7ea6a10053273319aR2360 <https://github.com/twisted/twisted/pull/261/files#diff-0fea8a8ca713deb7ea6a10053273319aR2360>
> 
> The errback is there to help with test failures ... but the test should never fail, so that errback is never called... and that line is not covered.
> 
> 
> It doesn't always make sense to require 100% execution of all test code.  It's not at all uncommon to only have code in a test suite that runs when a test fails.  Historically, Twisted has never had a requirement of 100% execution of test code.  The only test suite coverage requirements that have commonly been requested or enforced is for coverage of implementation code.
> 
> I'd suggest removing the coverage enforcement for test suite code.

I am inclined to disagree, albeit mildly.

When one is writing a deliberately un-covered path in test code, presumably, one is writing either a test helper - a mock, fake, or utility for setting up a real implementation - or an assertion method.  Historically, I believe that when we've invested more heavily in making these utilities "real" themselves, and not just throwaway stuff inline in a test method or module, the benefits have far outweighed the costs.  In fact the availability of proto_helpers is one of the selling points of Twisted as opposed to other competing engines.

Therefore, I think that asking folks to add independent test coverage to verify their fakes and ensure that the failure-reporting of their assertion messages are helpful in the event a test fails is a generally good idea, and we should keep the requirement for 100% coverage on both test and implementation coverage.

However, if there is contention around this, I'd much rather get a ratchet in place for implementation code that's reliable and everyone is happy with, so I'm OK with disabling coverage reporting for our *.test.* packages as a step towards that.

> How should we proceed with these changes?
> 
> Maybe this is not the best example and that code could be refactored... but I think that the topic of ignoring missing coverage is still valid.
> 
> I suggest to introduce `  # pragma: no cover`
> 
> and update the coverage config with
> 
> [report]
> exclude_lines =
>     pragma: no cover
> 
> 
> This seems like the wrong solution to me.  It forces contributors to do extra work to mark their test code as an exception and provides a mechanism for incorrectly bypassing the check by using a no-cover pragma in implementation code.

In any case I totally agree with this.  If we have a categorical difference in types of code (test vs. non-test) then let's make that distinction, but we should not be adding one-off exceptions as an exercise of non-uniform reviewer judgement on every review.

-glyph

-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20160630/4e1ad970/attachment-0002.html>