launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/some-lint-that-i-could-not-resist-fixing into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/some-lint-that-i-could-not-resist-fixing/+merge/98640

This cleans up some lint, but also it changes the ZeroconfService tests to use the --terminate option to avahi-browse instead of the select() stuff that is currently there. The existing code also suffers from a potential problem where there's a lot of output which fills up the stdout and/or the stderr pipes before they are read from; this change eliminates that problem.

-- 
https://code.launchpad.net/~allenap/maas/some-lint-that-i-could-not-resist-fixing/+merge/98640
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/some-lint-that-i-could-not-resist-fixing into lp:maas.

=== modified file 'src/maasserver/migrations/0001_initial.py'
--- src/maasserver/migrations/0001_initial.py	2012-03-16 10:49:35 +0000
+++ src/maasserver/migrations/0001_initial.py	2012-03-21 14:17:18 +0000
@@ -18,9 +18,11 @@
 
 # encoding: utf-8
 import datetime
+
+from django.db import models
 from south.db import db
 from south.v2 import SchemaMigration
-from django.db import models
+
 
 class Migration(SchemaMigration):
 

=== modified file 'src/maasserver/tests/test_maasavahi.py'
--- src/maasserver/tests/test_maasavahi.py	2012-03-19 20:52:17 +0000
+++ src/maasserver/tests/test_maasavahi.py	2012-03-21 14:17:18 +0000
@@ -13,8 +13,6 @@
 
 from collections import defaultdict
 
-from maastesting.testcase import TestCase
-
 import maasserver.maasavahi
 from maasserver.maasavahi import (
     MAASAvahiService,
@@ -24,6 +22,7 @@
     Config,
     config_manager,
     )
+from maastesting.testcase import TestCase
 
 
 class MockZeroconfServiceFactory:

=== modified file 'src/maasserver/tests/test_zeroconfservice.py'
--- src/maasserver/tests/test_zeroconfservice.py	2012-03-21 05:36:13 +0000
+++ src/maasserver/tests/test_zeroconfservice.py	2012-03-21 14:17:18 +0000
@@ -1,7 +1,7 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Tests for the ZeroconfService class"""
+"""Tests for `zeroconfservice`."""
 
 from __future__ import (
     print_function,
@@ -11,22 +11,27 @@
 __metaclass__ = type
 __all__ = []
 
-import random
-import select
+import itertools
 import subprocess
-import time
 
 from maasserver.zeroconfservice import ZeroconfService
+from maastesting.factory import factory
 from maastesting.testcase import TestCase
-
-# These tests will actually inject data in the system Avahi system.
-# Would be nice to isolate it from the system Avahi service, but I didn't
-# feel like writing a private DBus session with a mock Avahi service on it.
+from testtools.content import text_content
+
+
 class TestZeroconfService(TestCase):
+    """Test :class:`ZeroconfService`.
+
+    These tests will actually inject data in the system Avahi service. It
+    would be nice to isolate it from the system Avahi service, but there's a
+    lot of work involved in writing a private DBus session with a mock Avahi
+    service on it, probably more than it's worth.
+    """
 
     STYPE = '_maas_zeroconftest._tcp'
 
-    count = 0
+    count = itertools.count(1)
 
     def avahi_browse(self, service_type, timeout=3):
         """Return the list of published Avahi service through avahi-browse."""
@@ -34,29 +39,24 @@
         # running a glib mainloop. And stopping one is hard. Much easier to
         # kill an external process. This slows test, and could be fragile,
         # but it's the best I've come with.
+        command = (
+            'avahi-browse', '--no-db-lookup', '--parsable',
+            '--terminate', service_type)
         browser = subprocess.Popen(
-            ['avahi-browse', '-k', '-p', service_type],
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        until = time.time() + timeout
-        while time.time() < until:
-            # Busy loop until there is some input on stdout,
-            # or we give up.
-            ready = select.select([browser.stdout], [], [], 0.10)
-            if ready[0] or browser.poll():
-                break
-        browser.terminate()
+            command, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        stdout, stderr = browser.communicate()
+        self.addDetail("stdout", text_content(stdout))
+        self.addDetail("stderr", text_content(stderr))
         names = []
-        for record in browser.stdout.readlines():
+        for record in stdout.splitlines():
             fields = record.split(';')
             names.append(fields[3])
         return names
 
-    @classmethod
     def getUniqueServiceNameAndPort(self):
         # getUniqueString() generates an invalid service name
-        name = 'My-Test-Service-%d' % self.count
-        self.count += 1
-        port = random.randint(30000, 40000)
+        name = 'My-Test-Service-%d' % next(self.count)
+        port = factory.getRandomPort()
         return name, port
 
     def test_publish(self):
@@ -66,7 +66,7 @@
         service = ZeroconfService(name, port, self.STYPE)
         service.publish()
         # This will unregister the published name from Avahi.
-        self.addCleanup(service.group.Reset)
+        self.addCleanup(service.unpublish)
         services = self.avahi_browse(self.STYPE)
         self.assertIn(name, services)
 

=== modified file 'src/maasserver/zeroconfservice.py'
--- src/maasserver/zeroconfservice.py	2012-03-20 21:16:06 +0000
+++ src/maasserver/zeroconfservice.py	2012-03-21 14:17:18 +0000
@@ -1,20 +1,27 @@
 # Copyright 2008 (c) Pierre Duquesne <stackp@xxxxxxxxx>
 # Copyright 2012 Canonical Ltd.
-# This software is licensed under the GNU Affero General Public
-# License version 3 (see the file LICENSE).
+# This software is licensed under the GNU Affero General Public License
+# version 3 (see the file LICENSE).
 # Example code taken from http://stackp.online.fr/?p=35
 
-import avahi
-import dbus
-
+"""Work with Zeroconf."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
 __all__ = [
     "ZeroconfService",
     ]
 
+import avahi
+import dbus
+
 
 class ZeroconfService:
-    """A simple class to publish a network service with zeroconf using avahi.
-    """
+    """Publish a network service with Zeroconf using Avahi."""
 
     def __init__(self, name, port, stype="_http._tcp",
                  domain="", host="", text=""):
@@ -37,16 +44,13 @@
         server = dbus.Interface(
              bus.get_object(avahi.DBUS_NAME, avahi.DBUS_PATH_SERVER),
              avahi.DBUS_INTERFACE_SERVER)
-
         group = dbus.Interface(
             bus.get_object(avahi.DBUS_NAME, server.EntryGroupNew()),
             avahi.DBUS_INTERFACE_ENTRY_GROUP)
-
         group.AddService(
             avahi.IF_UNSPEC, avahi.PROTO_UNSPEC, dbus.UInt32(0),
             self.name, self.stype, self.domain, self.host,
             dbus.UInt16(self.port), self.text)
-
         group.Commit()
         self.group = group
 

=== modified file 'src/metadataserver/migrations/0001_initial.py'
--- src/metadataserver/migrations/0001_initial.py	2012-03-16 10:49:35 +0000
+++ src/metadataserver/migrations/0001_initial.py	2012-03-21 14:17:18 +0000
@@ -18,9 +18,11 @@
 
 # encoding: utf-8
 import datetime
+
+from django.db import models
 from south.db import db
 from south.v2 import SchemaMigration
-from django.db import models
+
 
 class Migration(SchemaMigration):
 

=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py	2012-03-15 22:43:55 +0000
+++ src/provisioningserver/tests/test_plugin.py	2012-03-21 14:17:18 +0000
@@ -11,8 +11,8 @@
 __metaclass__ = type
 __all__ = []
 
+from functools import partial
 from getpass import getuser
-from functools import partial
 import os
 
 from fixtures import TempDir