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

On Thu, Sep 23, 2010 at 10:06 AM, Michael Nelson
<email address hidden> wrote:
> Review: Approve code
> 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/
>

Fixed.

>> +
>> +        :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())
>

I'm not sure why that is. My preference is for the classmethod
approach, which is often more suitable for subclassing.

>> @@ -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 ;))
>

Interesting thought.

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

Yeah, I flag this every time I see it done in a review. Note that it's
only testtools & Trial that do this, Python rejected the change on the
grounds of not being Pythonic.

jml