<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jun 30, 2016, at 17:36, Jean-Paul Calderone <<a href="mailto:exarkun@twistedmatrix.com" class="">exarkun@twistedmatrix.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">On Thu, Jun 30, 2016 at 6:25 PM, Glyph Lefkowitz <span dir="ltr" class=""><<a href="mailto:glyph@twistedmatrix.com" target="_blank" class="">glyph@twistedmatrix.com</a>></span> wrote:<br class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jun 30, 2016, at 04:13, Jean-Paul Calderone <<a href="mailto:exarkun@twistedmatrix.com" target="_blank" class="">exarkun@twistedmatrix.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">On Thu, Jun 30, 2016 at 6:43 AM, Adi Roiban <span dir="ltr" class=""><<a href="mailto:adi@roiban.ro" target="_blank" class="">adi@roiban.ro</a>></span> wrote:<br class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="">Hi,<div class=""><br class=""></div><div class="">Recently we have introduced a hard check of 100% coverage for all changes.</div><div class="">This is done via coverage + codecov + github protected branches.</div><div class=""><br class=""></div><div class="">Now, if your patch is not 100% covered github will not let you merge it.<br clear="all" class=""><div class=""><br class=""></div><div class="">See for example this change: <a href="https://github.com/twisted/twisted/pull/261/files#diff-0fea8a8ca713deb7ea6a10053273319aR2360" target="_blank" class="">https://github.com/twisted/twisted/pull/261/files#diff-0fea8a8ca713deb7ea6a10053273319aR2360</a></div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">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.<br class=""><br class=""></div><div class="">I'd suggest removing the coverage enforcement for test suite code.</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">I am inclined to disagree, albeit mildly.</div><div class=""><br class=""></div><div class="">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.</div></div></div></blockquote><div class=""><br class=""></div><div class="">I mostly agree with this. However, I was thinking of a slightly different pattern when I wrote my earlier email. Here's are a couple (fictional) examples of that pattern one might find in unit tests for application code (and there's nothing Twisted-specific here):<br class=""><br class=""><div style="margin-left:40px" class=""><font face="monospace,monospace" class="">if foo:<br class=""></font></div><div style="margin-left:40px" class=""><font face="monospace,monospace" class=""> self.fail("Foo!")<br class=""><br class=""></font></div><div style="margin-left:40px" class=""><font face="monospace,monospace" class="">try:<br class=""></font></div><div style="margin-left:40px" class=""><font face="monospace,monospace" class=""> foo()<br class=""></font></div><div style="margin-left:40px" class=""><font face="monospace,monospace" class="">except:<br class=""></font></div><div style="margin-left:40px" class=""><font face="monospace,monospace" class=""> bar<br class=""></font></div><div style="margin-left:40px" class=""><font face="monospace,monospace" class="">else:<br class=""></font></div><div style="margin-left:40px" class=""><font face="monospace,monospace" class=""> self.fail("Foo :(")</font></div></div></div></div></div></div></blockquote><div><br class=""></div><div>Hm. This pattern is exactly what I was thinking of though - as you point out, these examples did get generalized :-).</div><div><br class=""></div><div>In principle, I agree that a one-off example like this does not benefit from extensive refactoring to facilitate general use. But... in <i class="">practice</i>, every example of this that I've seen in a long-lived codebase eventually metastasizes into a repeated copy/paste pattern, or a big gross mixin that all tests practically need. Forcing everyone to deal with the problem sooner rather than later seems to have been a win on the few projects where I've gotten to practice it.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">It's not exactly that this <i class="">can't</i> be code that's executed in a passing run of the test suite. It's more a question of what the right balance point is. If someone wants to generalize logic like this (and, fortunately, someone did generalize these <i class="">particular</i> examples - they're assertFalse and assertRaises, respectively) then that's great and the result is a higher level of confidence resulting from a successful run of the test suite. I'd suggest that if tests like these exercise all of the implementation code (on a successful run), though, then you've still achieved a pretty high level of test coverage and maybe further efforts are more productively directed elsewhere (increasing the coverage level of other implementation code in Twisted, for example :).<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>Speaking only from my direct experience here, adding good test helpers is a net reduction in effort <i class="">very</i> quickly; they pay for themselves on only the third test you write with them :). So I don't feel like this is much of a burden, but I'd be interested in hearing others' feedback here.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">If folks want a higher bar than this, I'm not going to argue (at least not much, at least not now). The bar <i class="">hasn't</i> been this high in the past though (and there are many many such cases to be found in Twisted's test suite right now and I don't have the impression this has ever been <i class="">much</i> of a source of problems).<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>I wish it were easier to search my own reviews, because I think I've prodded people to factor this kind of stuff out, but I can't find any handy examples.</div><div><br class=""></div><div>But, I can see that right after this, I'm going to have to answer Craig's email, so perhaps we'll get to see a specific example :).</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class="">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.</div><div class=""><br class=""></div><div class="">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.</div></div></div></blockquote><div class=""><br class=""></div><div class="">I completely agree that fakes should be verified. So much so that I'm not even sure I believe in fakes in <i class="">general</i> anymore. Instead, you should just have easy to use interfaces and ship inexpensive implementations alongside whatever other implementations you also need.</div></div></div></div></div></blockquote><div><br class=""></div><div>💯</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">And all those implementations should have great test coverage. I also completely agree that when tests fail, they should do so in a meaningful way. I suspect slightly the implication that automated test coverage for the failure case demonstrates the failure is reported meaningfully, though. :) I think we're still stuck with relying on humans (authors, reviewers) to verify that property.<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>Sure, rich and meaningful semantic error reporting is a human's job. But "doesn't just raise an exception and lose all the information about what happened due to an attribute spelling error on the rare race-y test that fails 1/10000 times" is a good first approximation, and tests are good at that sort of thing ;-).</div><div><br class=""></div><div>-glyph</div></div></body></html>