On Jun 30, 2016, at 04:13, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:
On Thu, Jun 30, 2016 at 6:43 AM, Adi Roiban <adi@roiban.ro <mailto:adi@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-0fea8a8ca713deb7ea6a1... <https://github.com/twisted/twisted/pull/261/files#diff-0fea8a8ca713deb7ea6a1...>
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