buildbot

I just read Roy Osherove's The Art of Unit Testing with Examples in .NET, on the advice of a slashdot review. I was not terribly impressed with the book, but reading it did help me to solidify my thinking about testing and test-driven development, and put words to concepts I had come to on my own.

Rather late in the book, Osherove describes three properties of good tests.

• Trustworthiness - Do developers believe that passing tests mean things are working? Do developers believe that failing tests indicate a real bug?
• Maintainability - Do developers think that tests are easy to add and maintain, or are they likely to avoid writing tests when rushed?
• Readability - Do developers often consult the unit tests to see how the system under test is supposed to work?

What most struck me was that these properties were related to developers' perceptions of the tests, not the tests themselves. Tests are as much a social artifact of a project as a technical tool.

Buildbot's Tests

Around the time I was reading this, one of the more prolific Buildbot contributors commented, "I try not to change the tests - they scare me." Buildbot's tests were badly isolated, slow, and failed intermittently. As maintainer, I had grown accustomed to saying "oh, that test fails sometimes, don't worry about it" - a trustworithiness failure. Because of the terrible isolation, changing just about anything in Buildbot would cause dozens of tests to fail, requiring repetitive editing to fix - not maintainable. And the tests consisted of long sequences of operations and assertions, written in the Twisted style, which is already not readable. As a result, even I don't know what most of the tests are actually testing. This was a bad situation for any application, but particularly embarassing for a popular testing tool!

So I blew the tests away. Well, not really - I moved them to buildbot/broken_test/ in hopes they can be useful in writing new tests, and so that the braver souls among us can still run them. Now our metabuildbot is green, and I can legitimately ask for unit tests for new code.

There are costs associated with this move, too. A lot of people have worked very hard to write tests that have now been categorically labeled "broken," to whom all I can say is "I'm sorry". With far fewer tests and thus far worse coverage, it's also difficult to have confidence that Buildbot really works. The short-term workaround is to make a few beta releases and rely on real-world testing to suss out any problem.

So this is only the first step. We - I - still need to write real tests for the vast majority of the Buildbot code. That's particularly complicated because Buildbot's units are badly isolated, and interfaces are ill-defined. I will need to do a good bit of refactoring to bring it into compliance.

Buildbot's WebStatus display has, for a long time, had an allowForce option which controls what kind of mayhem can be wrought via the web interface. Historically, this has been a boolean option: either web users can do everything (force builds and shut down slaves) or nothing. Bug 701 asks that we change that to give more granular access control.

Buildbot has an interesting way of separating the status display from the control functionality. It has two parallel interface hierarchies, IStatus and IControl, implementing the necessary methods. The IStatus hierarchy is illustrated with the orange bubbles here:

The IControl hierarchy is similar, although it only goes down to the Build level right now.

When allowForce is true, the WebStatus object adapts the buildmaster to the IControl interface and adds a link to the result in its control attribute. Forcing a build or shutting down a slave then uses this object to navigate to the appropriate control object and calls a method from the corresponding interface. If the control attribute is None, no access is allowed.

This scheme has the advantage that it is difficult to accidentally expose functionality, since when allowForce is false, the control methods are inaccessible. However, it has the disadvantage of not allowing any more granular level of access control.

I just reworked the web status to have a more flexible authorization mechanism, and while I wasn't able to remove the IControl hierarchy entirely, I was able to marginalize it to only those code blocks that need to perform controlled actions, instead of passing control objects all over the place.