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

Hi William,

This branch looks great. As we discussed on IRC please run it past a Soyuz team member and I'll approve it.

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

> +
> + >>> 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/

> +
> + >>> for b in builderset.getBuilders():
> + ... b.name

Please use 'print b.name' so the unicodiness is hidden.

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

Ditto

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

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