[Twisted-Python] Some comments regarding #5190 - ``RFC 6125 ("Service Identity") implementation´´

[Glyph Lefkowitz]

On Apr 30, 2014, at 5:21 AM, exarkun at twistedmatrix.com wrote:

> I've just noticed that the changeset for #5190 included some untested code.  Specifically, there are no tests for the code which detects missing dependencies and emits warnings about them.

My bad.  Well, technically hawkowl's bad; hawkowl is a committer and did the review and therefore has all the criminal liability in this case, but as the author who wrote the code I bear some responsibility, at least in some abstract, hypothetical sense ;-).

Thanks for working on the fix; it looks like the relevant ticket is <https://twistedmatrix.com/trac/ticket/7097>.  I'll try to review that as soon as it's ready; let me know.

> I'd previously noticed that this code was broken but hadn't realized this was because it was untested.

Neither the author nor the reviewer realized this either, apparently.  Certainly it wasn't an intentional omission.

> I don't think there's any disagreement whatsoever over Twisted's testing requirements.  All code must have full line and branch coverage (as reported by the coverage.py tool).  Developers, please write tests for all of your code (and please learn to do test-driven development - it will make this task easier, I promise).  Reviewers, please don't accept proposed changes that include untested code.

The problem with code like this is that, in some configurations, it is in fact reported as covered by coverage.py.  It requires manual examination to get the intersection of a diff and a coverage report, and even when you do, we still have too many places where it's "okay" to skip coverage.

As the author I looked at coverage periodically and it looked sort of like what I expected.  Since I was testing multiple installed-library configurations I had used "coverage combine" which misleadingly told me that it was all covered (although this particular code should have been tested independently without requiring a combined run).  And I'm sure the reviewer thought about it a little bit, but even if they'd looked at a coverage report, it might have looked like it was OK to skip these particular lines.  And I was in fact doing test-driven development; I didn't add the warning code there until I was looking at a failing test because one of the buildbots didn't have one of my expected dependencies installed, and I made my tests pass locally by having an environment without those dependencies installed locally either.

Yes, I understand how this isn't really 100% TDD, and that a failure on a buildbot should have resulted in me writing a new test; mistakes were made etc.  But all TDD necessarily involves the occasional error/error/pass where there really ought to have been a pass/fail/pass - if we understood what was going on with all of our code all the time we probably wouldn't need tests in the first place :-).  It's a bit disingenuous to say that I need to "learn to do test-driven development" to avoid mistakes like this, though.

On the other side of the equation, I imagine that a reviewer looking at this, even carefully considering coverage, might see a missed line on some buildbot or in their local run and then thought "oh, of course, but that line will be run if I had/didn't have that library installed".  And there are some bits of code which are acceptable to cover in this manner (except they should have direct test coverage from actual tests, rather than just importing the test module, which coverage.py won't show you).  It's a quite subtle point to understand that this particular kind of code should actually be fully covered in all configurations.  Especially because these tests are smack in the middle of a file which will be validly missing coverage in some supported configurations (no pyOpenSSL installed) and surrounded with thickets of conditionals and test skips to optionally import more dependencies than just this one.

We should remain vigilant, but I think that if we want to really reduce errors like this in the future we need to make them easier to spot.  Failing that we need to have more specific suggestions.  In this case, I happen to know that I do TDD and that Hawkowl was is aware of the standard on coverage issues (and is at least aware of coverage.py, whether or not it was run as part of this review), so those two suggestions aren't going to help as we're already doing them.  Any time the solution to a problem is "everybody should just try harder" that seems like a bet against human fallibility.

So until someone has a month to spend on an all-singing all-dancing combined ratcheting coverage report for all the builders and a fantastic visualization for its output which highlights every possible coverage issue, here are some specific suggestions which might avoid some parts of this class of error:

For authors (what I could have done better):

I know I said they're inevitable, but whenever you get an error/pass, always consider where you could make it a clean fail/pass instead.  You (and by "you" I obviously mean "me") think you understand why an error happened but the only way to really demonstrate you understand it well enough to convert it into an assertion that fails with a useful error message.
Be intensely suspicious of any code that needs to run at import time.  I did stuff the warning into a function, which at least doesn't leak local variables, but I probably could have moved this warning somewhere easier to manage, and would have noticed warnings coming out of tests as opposed to just being printed at the beginning.  Declarative like deprecatedModuleAttribute automate some of the magic for making code-level artifacts emit warnings when bound to and used rather than accidents of their initial import, so make use of those. (I'm still thinking about how I could have applied that in this specific case; I probably could have.)
Configure your development environment to be more aggressive about warnings (at least for now, eventually trial should fix this for you, see <https://twistedmatrix.com/trac/ticket/6348>).  I don't think it would have helped in this particular case because the warning itself is emitted at import time (see point 2) but this sort of mistake crops up unfortunately frequently related to deprecation warnings, which are a bit more common, and could often be caught by a better setup.  I recently changed my PYTHONWARNINGS environment variable to 'all::DeprecationWarning,all::UserWarning', and that seems to catch most things.  (Unfortunately setting it to simply 'all' produces too much noise from the stdlib and dependencies so it's better to be slightly more restrictive.)

For reviewers (what hawkowl could have done better):

Run coverage.  Particularly, run coverage just on the relevant and changed test modules, and make sure the system under test gets run directly and just accidentally executed by running the code.
I know I've been reminding reviewers lately to give clear feedback about what elements of reviews are suggestions and which are required fixes for violations of policy, and that may produce the subjective impression that I've been asking for faster or less careful reviews.  If so, I should correct that impression: I would like there to be less bike shedding, but it's still pretty important that the £10 million reactor actually work.  Any lack of test coverage is at least a potential policy violation.  Even if you think you understand why it's missing, even if it looks like a platform variance that doesn't make sense to test on the machine you're running, always ask the author to explain or justify why coverage isn't there, if it could be added to a cross-platform test with a reasonable (or, in many cases, even an existing) fake; if there's no relevant fake and it would be too much work, maybe we need to file a ticket for implementing some test support.
Especially if you're dealing with a new feature or a significant behavior change, always try to actually run and interact with the code and look at its output.  In this case, noticing the whitespace / formatting errors in the warning messages might have lead us to spot the coverage error earlier.  (Jean-Paul made some comments to me when he noticed it, but it was an off-the-cuff thing after the branch had already been landed and not part of a code review; context is important here, as evidenced by the fact that it took him some time to realize that it was indicative of a test coverage issue!

If anyone else has any ideas.

-glyph

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://twistedmatrix.com/pipermail/twisted-python/attachments/20140501/fe5aa030/attachment.html>