Code review comment for lp:~jml/launchpad/better-slave-mock

Thanks Jonathan - a few notes below, but nothing that needs attention.

It was a good chance for me to learn a little more about twisted :)

> === modified file 'lib/lp/buildmaster/model/builder.py'
> --- lib/lp/buildmaster/model/builder.py 2010-09-21 18:43:27 +0000
> +++ lib/lp/buildmaster/model/builder.py 2010-09-23 08:14:12 +0000
> @@ -137,43 +138,55 @@
> # should make a test double that doesn't do any XML-RPC and can be used to
> # make testing easier & tests faster.
>
> - def __init__(self, urlbase, vm_host):
> - """Initialise a Server with specific parameter to our buildfarm."""
> - self.vm_host = vm_host
> - self.urlbase = urlbase
> - rpc_url = urlappend(urlbase, "rpc")
> - self._server = xmlrpclib.Server(
> + def __init__(self, proxy, file_cache_url, vm_host):
> + """Initialise a Server with specific parameter to our buildfarm.

Nothing of yours, but s/parameter/parameters/

> +
> + :param proxy: An XML-RPC proxy, implementing 'callRemote'. It must
> + support passing and returning None objects.
> + :param file_cache_url: The URL of the file cache.
> + :param vm_host: The VM host to use when resuming.
> + """
> + self._vm_host = vm_host
> + self._file_cache_url = file_cache_url
> + self._server = proxy
> +
> + @classmethod
> + def makeBlockingSlave(cls, url_base, vm_host):
> + rpc_url = urlappend(url_base, 'rpc')
> + server_proxy = xmlrpclib.ServerProxy(
> rpc_url, transport=TimeoutTransport(), allow_none=True)
> + file_cache_url = urlappend(url_base, 'filecache')
> + return cls(BlockingProxy(server_proxy), file_cache_url, vm_host)

I don't have a preference, but have noticed the code guys using static
methods unless you need to use the class for something other than
constructing (eg. lp.code.model.sourcepackagerecipe.SourcePackageRecipe.new())

> @@ -203,9 +216,10 @@
> :param libraryfilealias: An `ILibraryFileAlias`.
> """
> url = libraryfilealias.http_url
> - logger.debug("Asking builder on %s to ensure it has file %s "
> - "(%s, %s)" % (self.urlbase, libraryfilealias.filename,
> - url, libraryfilealias.content.sha1))
> + logger.debug(
> + "Asking builder on %s to ensure it has file %s (%s, %s)" % (

Ignore for now, but I wonder if we should start removing interpolation
operators from logger calls (to avoid interpolation errors). Ie.
logger.debug("...", arg1, arg2, arg3) (Although if our tests exercise all of
our logging calls, then we don't need to worry ;))

> === modified file 'lib/lp/services/twistedsupport/tests/test_xmlrpc.py'
> --- lib/lp/services/twistedsupport/tests/test_xmlrpc.py 2010-08-20 20:31:18 +0000
> +++ lib/lp/services/twistedsupport/tests/test_xmlrpc.py 2010-09-23 08:14:12 +0000
> @@ -86,5 +91,47 @@
> self.assertEqual(TestFaultOne.msg_template, fault.faultString)
>
>
> +class TestBlockingProxy(TestCase):
> +
> + def test_callRemote_calls_attribute(self):
> + class ExampleProxy:
> + def ok(self, a, b, c=None):
> + return (c, b, a)
> + proxy = BlockingProxy(ExampleProxy())
> + result = proxy.callRemote('ok', 2, 3, c=8)
> + self.assertEqual((8, 3, 2), result)
> +
> + def test_callRemote_reraises_attribute(self):
> + class ExampleProxy:
> + def not_ok(self, a, b, c=None):
> + raise RuntimeError((c, b, a))
> + proxy = BlockingProxy(ExampleProxy())
> + error = self.assertRaises(
> + RuntimeError, proxy.callRemote, 'not_ok', 2, 3, c=8)
> + self.assertEqual(str(error), str((8, 3, 2)))

Ah, I'd not noticed assertRaises returns the error - (/me makes mental note to
go back and remove unnecessary try/except for test that checked the exception
str).