[Buildbot-devel] Request For Comments/Review : Console view

[Jean-Paul Calderone]

On Fri, 17 Jul 2009 12:27:34 -0400, "Dustin J. Mitchell" <dustin at zmanda.com> wrote:
>On Fri, Jul 17, 2009 at 11:16 AM, Nicolas Sylvain<nsylvain at chromium.org> wrote:
>> I was thinking about that too. Do you have an idea about the best way of
>> testing such things? I was thinking about having a small test master with
>> some data in it, and having a way to generate a json file or csv file with
>> the color of each box, and compare it with what I expect. this is not great
>> though. (and a lot of work) I'm not really familiar with the current
>> buildbot unittests, but I heard it's pretty limited.
>
>I don't have a good answer to this question, which is why I didn't
>suggest unit tests.
>
>I think that, at worst, a test that fires up a master, requests a
>page, and asserts that it returns 200 OK would be worthwhile for the
>effort it would require, since it would exercise the console code at
>least a little bit.
>
>I'll see if I can whip that up, and also stare at the code for a while.

I'm not sure about this.  It would certainly exercise the code in question,
but I think it would be exactly the kind of test which everyone (and in
particular, I'm thinking of you here, Dustin ;) would eventually complain
about and be tempted to disable or delete.

The problem with such shotgun testing is that it is more trouble to maintain
than the value it provides justifies.  Eventually someone will want to change
something about this code.  Their change might break the test, but since the
test exercises such a large chunk of code, it's not going to be clear why it
broke, or it's not going to be clear how to fix it, or in the best case, it
will be clear why it broken and how to fix it, but they'll be annoyed that
they have to fix it (and after four or five iterations of that, they'll start
thinking about deleting it).

Instead, I'd suggest some real unit tests, the kind that provide real value
and are easy to maintain. :)

A lot of Buildbot is not factored to make these easy, which is unfortunate.
However, factoring can be fixed.  In fact, factoring *must* be fixed. ;)  If
everyone only laments how hard testing is and moves on without doing anything
about it, then clearly testing will never get easier, and the project will
gradually spiral down the untestable pit of doom.

All that's pretty general, of course, and isn't meant as a criticism of this
patch set in particular, or even of buildbot more generally.  Things are how
they are, and I'm just trying to raise some interest in improving them. :)

As for specific suggestions, I'll have a go at a few:

  * The new free function getResultsClass is quite independent and seems as
    if it could easily have half a dozen tests (there seem to be about seven
    code paths through the function, in fact) written just for it.

  * ConsoleStatusResource.head is relatively self-contained.  Unfortunately
    it returns a blob of html as a string (and not even valid html, since it
    is only part of a page).  Nevertheless, this could probably be parsed
    into a DOM somehow and various assertions made about its value (plus, the
    fact that it can be parsed at all will say something useful).

  * ConsoleStatusResource.fetchChangesFromHistory goes a bit further into
    Buildbot-y untestableness, but not extremely far.  If one could lay
    hands on a fake build status implementation which gave out fake builder
    implementations which gave out fake build implementations, the logic of
    this method could be unit tested.  This involves creating three fakes (or
    maybe buildbot has fakes for these already, I haven't really looked).
    They're pretty simple fakes, though.  A status is mostly a dictionary
    mapping names to builders.  A builder is mostly a list of builds.  A build
    is mostly a few attributes describing a build.  Oh, I guess a sourcestamp
    is required too.  So, a little bit of work required here, but tractable.

  * Many of the rest of the ConsoleStatusResource methods are concerned either
    with html generation, buildbot model inspection, or some combination of
    the two.  So tests along the lines of those described in the previous two
    points should be possible.  Where possible, it may be useful to factor
    the html generation into completely separate methods from the model
    inspection.  This eases the job of the tests for html generation, since
    specific cases can be more easily directly tested.  It also lets you
    remove any html-related logic from tests for the model code.

This is all tractable.  It's more work than just committing the current patch
as is, of course, but a good unit test suite pay for itself in time saved
later.

Jean-Paul