Code review comment for lp:~wgrant/launchpad/move-rescueiflost-tests

On Tue, 2010-04-06 at 15:51 +0000, Brad Crittenden wrote:
> Review: Needs Information
> Hi William,
>
> This branch looks great. As we discussed on IRC please run it past a Soyuz team member and I'll approve it.

Thanks for the review, Brad.

> > === modified file 'lib/lp/buildmaster/doc/builder.txt'
> > --- lib/lp/buildmaster/doc/builder.txt 2010-03-24 10:24:22 +0000
> > +++ lib/lp/buildmaster/doc/builder.txt 2010-04-03 04:39:18 +0000
> > @@ -2,36 +2,34 @@
> > Builder Class
> > =============
> >
> > -This test aims to meet the requirements of
> > -<https://launchpad.canonical.com/BasicTestCoverage> for the Builder class,
> > -which represents the Buildd Slave entity.
> > -
> > -Need auxiliar methods from zope toolchain:
> > +The Builder class represents a slave machine in the build farm. These
> > +slaves are used to execute untrusted code -- for example when building
> > +packages.
> > +
> > +There are several builders in the sample data. Let's examine the first.
> > +
> > + >>> from lp.buildmaster.model.builder import Builder
> > + >>> builder = Builder.get(1)
> > +
> > +As expected, it implements IBuilder.
>
> Nice changes.
>
> > +Builders can take on different behaviors depending on the type of build
> > +they are currently processing. Each builder provides an attribute
> > +(current_build_behavior) to which all the build-type specific behavior
> > +for the current build is delegated. In the sample data, bob's current
> > +behavior is the behavior for dealing with binary packages
>
> How about:
> "behavior is dealing with binary packages."
> (Note full-stop.)
>
> > >>> from zope.security.proxy import isinstance
> > >>> from lp.soyuz.model.binarypackagebuildbehavior import (
> > @@ -40,85 +38,77 @@
> > ... builder.current_build_behavior, BinaryPackageBuildBehavior)
> > True
> >
> > -Confirm we can get the slave xmlrpc interface
> > +A builder has an XML-RPC proxy in the 'slave' attribute, which allows
> > +us to easily call methods on the slave machines.
> >
> > >>> s = builder.slave
> >
> > -Confirm that the urlbase is correct in that slave. (If the protocol changes,
> > -this may change too)
> > +The base URL of the proxy matches the builder's URL.
> >
> > >>> s.urlbase == builder.url
> > True
> >
> > -Check if the instance corresponds to the declared interface:
> > -
> > - >>> verifyObject(IBuilder, builder)
> > - True
> > -
> >
> > BuilderSet
> > ==========
> >
> > -Now perform the tests for the Builder ContentSet class, BuilderSet.
> > -
> > -Check if it can be imported:
> > -
> > +Builders and groups thereof are managed through a utility, IBuilderSet.
> > +
> > + >>> from zope.component import getUtility
> > >>> from lp.buildmaster.interfaces.builder import IBuilderSet
> > -
> > -Check we can use the set as a utility:
> > -
> > >>> builderset = getUtility(IBuilderSet)
> > -
> > -Check if the instance returned as utility corresponds to its
> > -respective interface:
> > -
> > >>> verifyObject(IBuilderSet, builderset)
> > True
> >
> > -Check if the instance is iterable:
> > +Iterating over a BuilderSet yields all registered builders.
> >
> > >>> for b in builderset:
> > - ... b.id
> > - 1
> > + ... print b.name
> > + bob
> > + frog
> > +
> > +count() return the number of builder instance we have stored:
>
> typo: instances

Fixed. This was in the original text, so I've also reworded it slightly.

> > +
> > + >>> builderset.count()
> > 2
> >
> > -Check if the __getitem__ method:
> > -
> > - >>> builderset['bob'].name
> > - u'bob'
> > -
> > -Check now the specific method in the utility as new():
> > +Builders can be retrieved by name.
> > +
> > + >>> print builderset['bob'].name
> > + bob
> > + >>> print builderset['bad']
> > + None
> > +
> > +And also by ID.
> > +
> > + >>> print builderset.get(2).name
> > + frog
> > + >>> print builderset.get(100).name
> > + Traceback (most recent call last):
> > + ...
> > + SQLObjectNotFound: Object not found
> > +
> > +The 'new' method will create a new builder in the database.
> >
> > >>> bnew = builderset.new(1, 'http://dummy.com:8221/', 'dummy',
> > ... 'Dummy Title', 'eh ?', 1)
> > >>> bnew.name
> > u'dummy'
> >
> > -Check get() which returns a correspondent Builder instance to a given id:
> > -
> > - >>> builderset.get(bnew.id).name
> > - u'dummy'
> > -
> > -Or raises an SQLObjectNotFound exception:
> > -
> > - >>> builderset.get(100)
> > - Traceback (most recent call last):
> > - ...
> > - SQLObjectNotFound: Object not found
> > -
> > -count() return the number of builder instance we have stored:
> > -
> > - >>> builderset.count()
> > - 3
> > -
> > -getBuilder() method returns all the builders available. It seems the
> > -same than the own instance but we have plans to turn it aware of some
> > -attributes of builder instance as: builderok and trust.
> > -
> > - >>> for b in builderset.getBuilders():
> > - ... b.name
> > - u'bob'
> > - u'dummy'
> > +'getBuilders' returns builders with the 'active' flag set, ordered by
> > +virtualisation status, architecture, then name.
>
> We've agreed on en_US (don't hate me) so
> /virtualisation/virtualization/

Also (begrudgingly) fixed.

> > +
> > + >>> for b in builderset.getBuilders():
> > + ... b.name
>
> Please use 'print b.name' so the unicodiness is hidden.

Fixed.

> > + u'bob'
> > + u'dummy'
> > + u'frog'
> > + >>> login('<email address hidden>')
> > + >>> bnew.active = False
> > + >>> login(ANONYMOUS)
> > + >>> for b in builderset.getBuilders():
> > + ... b.name
>
> Ditto

Fixed.

> > + u'bob'
> > u'frog'
> >
> > 'getBuildQueueSizeForProcessor' returns the number of pending builds
> > === modified file 'lib/lp/code/tests/test_recipebuilder.py'
> > --- lib/lp/code/tests/test_recipebuilder.py 2010-03-25 18:34:07 +0000
> > +++ lib/lp/code/tests/test_recipebuilder.py 2010-04-03 04:39:18 +0000
> > @@ -21,7 +21,7 @@
> > from lp.soyuz.adapters.archivedependencies import get_sources_list_for_building
> > from lp.soyuz.model.processor import ProcessorFamilySet
> > from lp.soyuz.tests.soyuzbuilddhelpers import (MockBuilder,
> > - SaneBuildingSlave,)
> > + OkSlave,)
>
> If the import is more than one line, the first line is to end with the
> open paren.

And fixed.

> > from lp.soyuz.tests.test_binarypackagebuildbehavior import (
> > BaseTestVerifySlaveBuildID)
> > from lp.soyuz.tests.test_publishing import (
>
> > === modified file 'lib/lp/soyuz/tests/soyuzbuilddhelpers.py'
> > --- lib/lp/soyuz/tests/soyuzbuilddhelpers.py 2010-03-26 01:52:07 +0000
> > +++ lib/lp/soyuz/tests/soyuzbuilddhelpers.py 2010-04-03 04:39:18 +0000
> > @@ -7,11 +7,6 @@
> >
> > __all__ = [
> > 'MockBuilder',
> > - 'SaneBuildingSlave',
> > - 'SaneWaitingSlave',
> > - 'InsaneWaitingSlave',
> > - 'LostBuildingSlave',
> > - 'LostWaitingSlave',
> > 'LostBuildingBrokenSlave',
> > 'BrokenSlave',
> > 'OkSlave',
> > @@ -36,9 +31,12 @@
> > class MockBuilder:
> > """Emulates a IBuilder class."""
> >
> > - current_build_behavior = BinaryPackageBuildBehavior(None)
> > + def __init__(self, name, slave, behavior=None):
> > + if behavior is None:
> > + self.current_build_behavior = BinaryPackageBuildBehavior(None)
> > + else:
> > + self.current_build_behavior = behavior
> >
> > - def __init__(self, name, slave):
> > self.slave = slave
> > self.builderok = True
> > self.manual = False
>
> Nice cleanup William.

Thanks for the review.