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

[Nicolas Sylvain]

On Fri, Jul 17, 2009 at 10:13 AM, Jean-Paul Calderone <exarkun at divmod.com>wrote:

> 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.


That makes a lot of sense.

It's important to note that the scope of this patch does not include "add a
new testing
framework for buildbot", but I'll add at least some tests to get started
with what I can
find.

I also agree with you that just running the console page blindly and getting
the http error code
would not give much. We would see when someone regresses the console view,
but they
would not know why or how to fix it.

I'll see what I can do, and send you the patch when it's done.

thanks

Nicolas





> Jean-Paul
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://buildbot.net/pipermail/devel/attachments/20090717/a50a9034/attachment.html>