← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~allenap/txlongpoll/config-file into lp:txlongpoll

 

Gavin Panella has proposed merging lp:~allenap/txlongpoll/config-file into lp:txlongpoll.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/txlongpoll/config-file/+merge/97079

This changes txlongpoll to use a config file (yaml, though the validation is format agnostic) in the same way that maas is using a config file (and where it's working well). It's also changes the semi-broken getRotatableLogFileObserver() and setUpOOPSHandler() to run as services. Again, this approach is copied from maas.
-- 
https://code.launchpad.net/~allenap/txlongpoll/config-file/+merge/97079
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/txlongpoll/config-file into lp:txlongpoll.
=== modified file 'buildout.cfg'
--- buildout.cfg	2011-11-07 21:10:44 +0000
+++ buildout.cfg	2012-03-12 18:17:41 +0000
@@ -6,6 +6,7 @@
 use-dependency-links = false
 unzip = true
 include-site-packages = false
+allowed-eggs-from-site-packages = pyyaml
 exec-sitecustomize = true
 develop = .
 download-cache = download-cache

=== added directory 'etc'
=== added file 'etc/txlongpoll.yaml'
--- etc/txlongpoll.yaml	1970-01-01 00:00:00 +0000
+++ etc/txlongpoll.yaml	2012-03-12 18:17:41 +0000
@@ -0,0 +1,33 @@
+##
+## txlongpoll configuration.
+##
+
+## The front-end service.
+#
+frontend:
+  # The port on which to serve.
+  port: 56100
+  # If specified, the queue name requested must have the given prefix.
+  # prefix:
+
+## OOPS configuration.
+#
+oops:
+  # Directory in which to place OOPS reports.
+  # directory: ""
+  # The reporter used when generating OOPS reports.
+  # reporter: "LONGPOLL"
+
+## Message broker configuration.
+#
+broker:
+  # host: "localhost"
+  # port: 5672
+  # username: <current user>
+  # password: "test"
+  # vhost: "/"
+
+## Where to log. This log can be rotated by sending SIGUSR1 to the
+## running server.
+#
+# logfile: "txlongpoll.log"

=== modified file 'setup.py'
--- setup.py	2011-11-28 10:53:25 +0000
+++ setup.py	2012-03-12 18:17:41 +0000
@@ -18,10 +18,11 @@
     zip_safe=False,
     description='Long polling HTTP frontend for AMQP',
     install_requires=[
+        'formencode',
         'oops_amqp',
         'oops_datedir_repo >= 0.0.13',
         'oops_twisted >= 0.0.3',
-        'setproctitle',
+        'pyyaml',
         'Twisted',
         'txAMQP >= 0.5',
         'zope.interface',

=== modified file 'txlongpoll/plugin.py'
--- txlongpoll/plugin.py	2011-11-07 16:50:53 +0000
+++ txlongpoll/plugin.py	2012-03-12 18:17:41 +0000
@@ -1,24 +1,23 @@
 # Copyright 2005-2011 Canonical Ltd.  This software is licensed under
 # the GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
 __all__ = [
     "AMQServiceMaker",
     ]
 
-from functools import partial
-import signal
-import sys
+from getpass import getuser
 
-from amqplib import client_0_8 as amqp
-import oops
-from oops_amqp import Publisher
-from oops_datedir_repo import DateDirRepo
-from oops_twisted import (
-    Config as oops_config,
-    defer_publisher,
-    OOPSObserver,
+from formencode import Schema
+from formencode.validators import (
+    Int,
+    RequireIfPresent,
+    String,
     )
-import setproctitle
 from twisted.application.internet import (
     TCPClient,
     TCPServer,
@@ -27,106 +26,91 @@
     IServiceMaker,
     MultiService,
     )
-from twisted.internet import reactor
 from twisted.plugin import IPlugin
 from twisted.python import (
     log,
     usage,
     )
-from twisted.python.log import (
-    addObserver,
-    FileLogObserver,
-    )
-from twisted.python.logfile import LogFile
 from twisted.web.server import Site
 from txlongpoll.client import AMQFactory
 from txlongpoll.frontend import (
     FrontEndAjax,
     QueueManager,
     )
+from txlongpoll.services import (
+    LogService,
+    OOPSService,
+    )
+import yaml
 from zope.interface import implements
 
 
-def getRotatableLogFileObserver(filename):
-    """Setup a L{LogFile} for the given application."""
-    if filename != '-':
-        logfile = LogFile.fromFullPath(
-            filename, rotateLength=None, defaultMode=0644)
-        def signal_handler(sig, frame):
-            reactor.callFromThread(logfile.reopen)
-        signal.signal(signal.SIGUSR1, signal_handler)
-    else:
-        logfile = sys.stdout
-    return FileLogObserver(logfile)
-
-
-def setUpOOPSHandler(options, logfile):
-    """Add OOPS handling based on the passed command line options."""
-    config = oops_config()
-
-    # Add the oops publisher that writes files in the configured place
-    # if the command line option was set.
-
-    if options["oops-exchange"]:
-        oops_exchange = options["oops-exchange"]
-        oops_key = options["oops-routingkey"] or ""
-        host = options["brokerhost"]
-        if options["brokerport"]:
-            host = "%s:%s" % (host, options["brokerport"])
-        rabbit_connect = partial(
-            amqp.Connection, host=host,
-            userid=options["brokeruser"],
-            password=options["brokerpassword"],
-            virtual_host=options["brokervhost"])
-        amqp_publisher = Publisher(
-            rabbit_connect, oops_exchange, oops_key)
-        config.publishers.append(defer_publisher(amqp_publisher))
-
-    if options["oops-dir"]:
-        repo = DateDirRepo(options["oops-dir"])
-        config.publishers.append(
-            defer_publisher(oops.publish_new_only(repo.publish)))
-
-    if options["oops-reporter"]:
-        config.template['reporter'] = options["oops-reporter"]
-
-    observer = OOPSObserver(config, logfile.emit)
-    addObserver(observer.emit)
-    return observer
+class ConfigOops(Schema):
+    """Configuration validator for OOPS options."""
+
+    if_key_missing = None
+
+    directory = String(if_missing=b"")
+    reporter = String(if_missing=b"LONGPOLL")
+
+    chained_validators = (
+        RequireIfPresent("reporter", present="directory"),
+        )
+
+
+class ConfigBroker(Schema):
+    """Configuration validator for message broker options."""
+
+    if_key_missing = None
+
+    host = String(if_missing=b"localhost")
+    port = Int(min=1, max=65535, if_missing=5672)
+    username = String(if_missing=getuser())
+    password = String(if_missing=b"test")
+    vhost = String(if_missing="/")
+
+
+class ConfigFrontend(Schema):
+    """Configuration validator for the front-end service."""
+
+    if_key_missing = None
+
+    port = Int(min=1, max=65535, if_missing=8001)
+    prefix = String(if_missing=None)
+
+
+class Config(Schema):
+    """Configuration validator."""
+
+    if_key_missing = None
+
+    oops = ConfigOops
+    broker = ConfigBroker
+    frontend = ConfigFrontend
+
+    logfile = String(
+        if_empty=b"txlongpoll.log",
+        if_missing=b"txlongpoll.log")
+
+    @classmethod
+    def parse(cls, stream):
+        """Load a YAML configuration from `stream` and validate."""
+        return cls.to_python(yaml.load(stream))
+
+    @classmethod
+    def load(cls, filename):
+        """Load a YAML configuration from `filename` and validate."""
+        with open(filename, "rb") as stream:
+            return cls.parse(stream)
 
 
 class Options(usage.Options):
 
     optParameters = [
-        ["logfile", "l", "txlongpoll.log", "Logfile name."],
-        ["brokerport", "p", 5672, "Broker port"],
-        ["brokerhost", "h", '127.0.0.1', "Broker host"],
-        ["brokeruser", "u", None, "Broker user"],
-        ["brokerpassword", "a", None, "Broker password"],
-        ["brokervhost", "v", '/', "Broker vhost"],
-        ["frontendport", "f", None, "Frontend port"],
-        ["prefix", "x", None, "Queue prefix"],
-        ["oops-dir", "r", None, "Where to write OOPS reports"],
-        ["oops-reporter", "o", "LONGPOLL", "String identifying this service."],
-        ["oops-exchange", None, None, "AMQP Exchange to send OOPS reports to."],
-        ["oops-routingkey", None, None, "Routing key for AMQP OOPSes."],
+        ["config-file", "c", "etc/txlongpoll.yaml",
+         "Configuration file to load."],
         ]
 
-    def postOptions(self):
-        for man_arg in ('frontendport', 'brokeruser', 'brokerpassword'):
-            if not self[man_arg]:
-                raise usage.UsageError("--%s must be specified." % man_arg)
-        for int_arg in ('brokerport', 'frontendport'):
-            try:
-                self[int_arg] = int(self[int_arg])
-            except (TypeError, ValueError):
-                raise usage.UsageError("--%s must be an integer." % int_arg)
-        if not self["oops-reporter"] and (
-            self["oops-exchange"] or self["oops-dir"]):
-            raise usage.UsageError(
-                "A reporter must be supplied to identify reports "
-                "from this service from other OOPS reports.")
-
 
 class AMQServiceMaker(object):
     """Create an asynchronous frontend server for AMQP."""
@@ -140,33 +124,49 @@
         self.description = description
 
     def makeService(self, options):
-        """Construct a TCPServer and TCPClient."""
-        setproctitle.setproctitle(
-            "txlongpoll: accepting connections on %s" %
-                options["frontendport"])
-
-        logfile = getRotatableLogFileObserver(options["logfile"])
-        setUpOOPSHandler(options, logfile)
-
-        broker_port = options["brokerport"]
-        broker_host = options["brokerhost"]
-        broker_user = options["brokeruser"]
-        broker_password = options["brokerpassword"]
-        broker_vhost = options["brokervhost"]
-        frontend_port = options["frontendport"]
-        prefix = options["prefix"]
-
-        manager = QueueManager(prefix)
-        factory = AMQFactory(
-            broker_user, broker_password, broker_vhost, manager.connected,
-            manager.disconnected,
-            lambda (connector, reason): log.err(reason, "Connection failed"))
-        resource = FrontEndAjax(manager)
-
-        client_service = TCPClient(broker_host, broker_port, factory)
-        server_service = TCPServer(frontend_port, Site(resource))
+        """Construct a service."""
         services = MultiService()
-        services.addService(client_service)
-        services.addService(server_service)
+
+        config_file = options["config-file"]
+        config = Config.load(config_file)
+
+        log_service = LogService(config["logfile"])
+        log_service.setServiceParent(services)
+
+        oops_config = config["oops"]
+        oops_dir = oops_config["directory"]
+        oops_reporter = oops_config["reporter"]
+        oops_service = OOPSService(log_service, oops_dir, oops_reporter)
+        oops_service.setServiceParent(services)
+
+        frontend_config = config["frontend"]
+        frontend_port = frontend_config["port"]
+        frontend_prefix = frontend_config["prefix"]
+        frontend_manager = QueueManager(frontend_prefix)
+
+        broker_config = config["broker"]
+        broker_port = broker_config["port"]
+        broker_host = broker_config["host"]
+        broker_username = broker_config["username"]
+        broker_password = broker_config["password"]
+        broker_vhost = broker_config["vhost"]
+
+        cb_connected = frontend_manager.connected
+        cb_disconnected = frontend_manager.disconnected
+        cb_failed = lambda connector_and_reason: (
+            log.err(connector_and_reason[1], "Connection failed"))
+
+        client_factory = AMQFactory(
+            broker_username, broker_password, broker_vhost,
+            cb_connected, cb_disconnected, cb_failed)
+        client_service = TCPClient(
+            broker_host, broker_port, client_factory)
+        client_service.setName("amqp")
+        client_service.setServiceParent(services)
+
+        frontend_resource = FrontEndAjax(frontend_manager)
+        frontend_service = TCPServer(frontend_port, Site(frontend_resource))
+        frontend_service.setName("frontend")
+        frontend_service.setServiceParent(services)
 
         return services

=== added file 'txlongpoll/services.py'
--- txlongpoll/services.py	1970-01-01 00:00:00 +0000
+++ txlongpoll/services.py	2012-03-12 18:17:41 +0000
@@ -0,0 +1,106 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Additional services that compose txlongpoll."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    "LogService",
+    "OOPSService",
+    ]
+
+import signal
+import sys
+
+import oops
+from oops_datedir_repo import DateDirRepo
+from oops_twisted import (
+    Config as oops_config,
+    defer_publisher,
+    OOPSObserver,
+    )
+from twisted.application.service import Service
+from twisted.internet import reactor
+from twisted.python.log import (
+    addObserver,
+    FileLogObserver,
+    removeObserver,
+    )
+from twisted.python.logfile import LogFile
+
+
+class LogService(Service):
+
+    name = "log"
+
+    def __init__(self, filename):
+        self.filename = filename
+        self.logfile = None
+        self.observer = None
+
+    def _signal_handler(self, sig, frame):
+        reactor.callFromThread(self.logfile.reopen)
+
+    def startService(self):
+        Service.startService(self)
+        if self.filename != '-':
+            self.logfile = LogFile.fromFullPath(
+                self.filename, rotateLength=None, defaultMode=0o644)
+            self.__previous_signal_handler = signal.signal(
+                signal.SIGUSR1, self._signal_handler)
+        else:
+            self.logfile = sys.stdout
+        self.observer = FileLogObserver(self.logfile)
+        self.observer.start()
+
+    def stopService(self):
+        Service.stopService(self)
+        if self.filename != '-':
+            signal.signal(signal.SIGUSR1, self.__previous_signal_handler)
+            del self.__previous_signal_handler
+            self.observer.stop()
+            self.observer = None
+            self.logfile.close()
+            self.logfile = None
+        else:
+            self.observer.stop()
+            self.observer = None
+            # Don't close stdout.
+            self.logfile = None
+
+
+class OOPSService(Service):
+
+    name = "oops"
+
+    def __init__(self, log_service, oops_dir, oops_reporter):
+        self.config = None
+        self.log_service = log_service
+        self.oops_dir = oops_dir
+        self.oops_reporter = oops_reporter
+
+    def startService(self):
+        Service.startService(self)
+        self.config = oops_config()
+        # Add the oops publisher that writes files in the configured place if
+        # the command line option was set.
+        if self.oops_dir:
+            repo = DateDirRepo(self.oops_dir)
+            self.config.publishers.append(
+                defer_publisher(oops.publish_new_only(repo.publish)))
+        if self.oops_reporter:
+            self.config.template['reporter'] = self.oops_reporter
+        self.observer = OOPSObserver(
+            self.config, self.log_service.observer.emit)
+        addObserver(self.observer.emit)
+
+    def stopService(self):
+        Service.stopService(self)
+        removeObserver(self.observer.emit)
+        self.observer = None
+        self.config = None

=== modified file 'txlongpoll/tests/test_plugin.py'
--- txlongpoll/tests/test_plugin.py	2011-11-08 11:34:06 +0000
+++ txlongpoll/tests/test_plugin.py	2012-03-12 18:17:41 +0000
@@ -1,207 +1,104 @@
 # Copyright 2005-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from cStringIO import StringIO
 from functools import partial
+from getpass import getuser
 import os
 from unittest import defaultTestLoader
 
 from fixtures import TempDir
-from oops_twisted import OOPSObserver
+from formencode import Invalid
 from subunit import IsolatedTestCase
 from testtools import TestCase
-from testtools.content import (
-    Content,
-    UTF8_TEXT,
-    )
 from testtools.matchers import (
     MatchesException,
     Raises,
     )
 from twisted.application.service import MultiService
-from twisted.python.log import (
-    FileLogObserver,
-    theLogPublisher,
-    )
-from twisted.python.usage import UsageError
 from txlongpoll.plugin import (
     AMQServiceMaker,
+    Config,
     Options,
-    setUpOOPSHandler,
     )
 
 
+class TestConfig(TestCase):
+    """Tests for `txlongpoll.plugin.Options`."""
+
+    def test_defaults(self):
+        expected = {
+            'broker': {
+                'host': 'localhost',
+                'password': 'test',
+                'port': 5672,
+                'username': getuser(),
+                'vhost': u'/',
+                },
+            'frontend': {
+                'port': 8001,
+                'prefix': None,
+                },
+            'logfile': 'txlongpoll.log',
+            'oops': {
+                'directory': '',
+                'reporter': 'LONGPOLL',
+                },
+            }
+        observed = Config.to_python({})
+        self.assertEqual(expected, observed)
+
+    def test_parse(self):
+        # Configuration can be parsed from a snippet of YAML.
+        observed = Config.parse(b'logfile: "/some/where.log"')
+        self.assertEqual("/some/where.log", observed["logfile"])
+
+    def test_load(self):
+        # Configuration can be loaded and parsed from a file.
+        filename = os.path.join(
+            self.useFixture(TempDir()).path, "config.yaml")
+        with open(filename, "wb") as stream:
+            stream.write(b'logfile: "/some/where.log"')
+        observed = Config.load(filename)
+        self.assertEqual("/some/where.log", observed["logfile"])
+
+    def test_load_example(self):
+        # The example configuration can be loaded and validated.
+        filename = os.path.join(
+            os.path.dirname(__file__), os.pardir,
+            os.pardir, "etc", "txlongpoll.yaml")
+        Config.load(filename)
+
+    def check_exception(self, config, message):
+        # Check that a UsageError is raised when parsing options.
+        self.assertThat(
+            partial(Config.to_python, config),
+            Raises(MatchesException(Invalid, message)))
+
+    def test_option_broker_port_integer(self):
+        self.check_exception(
+            {"broker": {"port": "bob"}},
+            "broker: port: Please enter an integer value")
+
+    def test_option_frontend_port_integer(self):
+        self.check_exception(
+            {"frontend": {"port": "bob"}},
+            "frontend: port: Please enter an integer value")
+
+
 class TestOptions(TestCase):
     """Tests for `txlongpoll.plugin.Options`."""
 
     def test_defaults(self):
         options = Options()
-        expected = {
-            "brokerhost": "127.0.0.1",
-            "brokerpassword": None,
-            "brokerport": 5672,
-            "brokeruser": None,
-            "brokervhost": "/",
-            "frontendport": None,
-            "logfile": "txlongpoll.log",
-            "oops-dir": None,
-            "oops-exchange": None,
-            "oops-reporter": "LONGPOLL",
-            "oops-routingkey": None,
-            "prefix": None,
-            }
+        expected = {"config-file": "etc/txlongpoll.yaml"}
         self.assertEqual(expected, options.defaults)
 
-    def check_exception(self, options, message, *arguments):
-        # Check that a UsageError is raised when parsing options.
-        self.assertThat(
-            partial(options.parseOptions, arguments),
-            Raises(MatchesException(UsageError, message)))
-
-    def test_option_frontendport_required(self):
-        options = Options()
-        self.check_exception(
-            options,
-            "--frontendport must be specified")
-
-    def test_option_brokeruser_required(self):
-       options = Options()
-       self.check_exception(
-            options,
-            "--brokeruser must be specified",
-            "--frontendport", "1234")
-
-    def test_option_brokerpassword_required(self):
-        options = Options()
-        self.check_exception(
-            options,
-            "--brokerpassword must be specified",
-            "--brokeruser", "Bob",
-            "--frontendport", "1234")
-
     def test_parse_minimal_options(self):
         options = Options()
         # The minimal set of options that must be provided.
-        arguments = [
-            "--brokerpassword", "Hoskins",
-            "--brokeruser", "Bob",
-            "--frontendport", "1234",
-            ]
+        arguments = []
         options.parseOptions(arguments)  # No error.
 
-    def test_parse_int_options(self):
-        # Some options are converted to ints.
-        options = Options()
-        arguments = [
-            "--brokerpassword", "Hoskins",
-            "--brokerport", "4321",
-            "--brokeruser", "Bob",
-            "--frontendport", "1234",
-            ]
-        options.parseOptions(arguments)
-        self.assertEqual(4321, options["brokerport"])
-        self.assertEqual(1234, options["frontendport"])
-
-    def test_parse_broken_int_options(self):
-        # An error is raised if the integer options do not contain integers.
-        options = Options()
-        arguments = [
-            "--brokerpassword", "Hoskins",
-            "--brokerport", "Jr.",
-            "--brokeruser", "Bob",
-            ]
-        self.assertRaises(
-            UsageError, options.parseOptions, arguments)
-
-    def test_oops_exchange_without_reporter(self):
-        # It is an error to omit the OOPS reporter if exchange is specified.
-        options = Options()
-        arguments = [
-            "--brokerpassword", "Hoskins",
-            "--brokeruser", "Bob",
-            "--frontendport", "1234",
-            "--oops-exchange", "something",
-            "--oops-reporter", "",
-            ]
-        expected = MatchesException(
-            UsageError, "A reporter must be supplied")
-        self.assertThat(
-            partial(options.parseOptions, arguments),
-            Raises(expected))
-
-    def test_oops_dir_without_reporter(self):
-        # It is an error to omit the OOPS reporter if directory is specified.
-        options = Options()
-        arguments = [
-            "--brokerpassword", "Hoskins",
-            "--brokeruser", "Bob",
-            "--frontendport", "1234",
-            "--oops-dir", "/some/where",
-            "--oops-reporter", "",
-            ]
-        expected = MatchesException(
-            UsageError, "A reporter must be supplied")
-        self.assertThat(
-            partial(options.parseOptions, arguments),
-            Raises(expected))
-
-
-class TestSetUpOOPSHandler(TestCase):
-    """Tests for `txlongpoll.plugin.setUpOOPSHandler`."""
-
-    def setUp(self):
-        super(TestSetUpOOPSHandler, self).setUp()
-        self.observers = theLogPublisher.observers[:]
-        self.logfile = StringIO()
-        self.addDetail("log", Content(UTF8_TEXT, self.logfile.getvalue))
-        self.log = FileLogObserver(self.logfile)
-
-    def tearDown(self):
-        super(TestSetUpOOPSHandler, self).tearDown()
-        theLogPublisher.observers[:] = self.observers
-
-    def makeObserver(self, settings):
-        options = Options()
-        options["brokerpassword"] = "Hoskins"
-        options["brokeruser"] = "Bob"
-        options["frontendport"] = 1234
-        options.update(settings)
-        observer = setUpOOPSHandler(options, self.log)
-        return options, observer
-
-    def test_minimal(self):
-        options, observer = self.makeObserver({})
-        self.assertIsInstance(observer, OOPSObserver)
-        self.assertEqual([], observer.config.publishers)
-        self.assertEqual(
-            {"reporter": options.defaults["oops-reporter"]},
-            observer.config.template)
-
-    def test_with_all_params(self):
-        settings = {
-            "oops-exchange": "Frank",
-            "oops-reporter": "Sidebottom",
-            "oops-dir": self.useFixture(TempDir()).path,
-            }
-        options, observer = self.makeObserver(settings)
-        self.assertIsInstance(observer, OOPSObserver)
-        self.assertEqual(2, len(observer.config.publishers))
-        self.assertEqual(
-            {"reporter": "Sidebottom"},
-            observer.config.template)
-
-    def test_with_some_params(self):
-        settings = {
-            "oops-exchange": "Frank",
-            "oops-reporter": "Sidebottom",
-            }
-        options, observer = self.makeObserver(settings)
-        self.assertIsInstance(observer, OOPSObserver)
-        self.assertEqual(1, len(observer.config.publishers))
-        self.assertEqual(
-            {"reporter": "Sidebottom"},
-            observer.config.template)
-
 
 class TestAMQServiceMaker(IsolatedTestCase, TestCase):
     """Tests for `txlongpoll.plugin.AMQServiceMaker`."""
@@ -211,26 +108,18 @@
         self.assertEqual("Harry", service_maker.tapname)
         self.assertEqual("Hill", service_maker.description)
 
-    def makeOptions(self, settings):
-        options = Options()
-        options["brokerpassword"] = "Hoskins"
-        options["brokeruser"] = "Bob"
-        options["frontendport"] = 1234
-        options.update(settings)
-        return options
-
     def test_makeService(self):
-        logfile = os.path.join(
-            self.useFixture(TempDir()).path, "txlongpoll.log")
-        options = self.makeOptions({"logfile": logfile})
+        options = Options()
         service_maker = AMQServiceMaker("Harry", "Hill")
         service = service_maker.makeService(options)
         self.assertIsInstance(service, MultiService)
-        self.assertEqual(2, len(service.services))
-        client_service, server_service = service.services
-        self.assertEqual(options["brokerhost"], client_service.args[0])
-        self.assertEqual(options["brokerport"], client_service.args[1])
-        self.assertEqual(options["frontendport"], server_service.args[0])
+        self.assertEqual(4, len(service.services))
+        self.assertSequenceEqual(
+            ["amqp", "frontend", "log", "oops"],
+            sorted(service.namedServices))
+        self.assertEqual(
+            len(service.namedServices), len(service.services),
+            "Not all services are named.")
 
 
 def test_suite():

=== added file 'txlongpoll/tests/test_services.py'
--- txlongpoll/tests/test_services.py	1970-01-01 00:00:00 +0000
+++ txlongpoll/tests/test_services.py	2012-03-12 18:17:41 +0000
@@ -0,0 +1,120 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `txlongpoll.services`."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+import os
+import signal
+import sys
+
+from fixtures import TempDir
+from oops_twisted import OOPSObserver
+from txlongpoll.services import (
+    LogService,
+    OOPSService,
+    )
+from testtools import TestCase
+from testtools.content import content_from_file
+from testtools.deferredruntest import AsynchronousDeferredRunTest
+from twisted.application.service import MultiService
+from twisted.python.log import (
+    FileLogObserver,
+    theLogPublisher,
+    )
+from twisted.python.logfile import LogFile
+
+
+class TestServicesBase:
+
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
+
+    def setUp(self):
+        super(TestServicesBase, self).setUp()
+        self.observers = theLogPublisher.observers[:]
+        self.services = MultiService()
+        self.services.privilegedStartService()
+        self.services.startService()
+
+    def tearDown(self):
+        super(TestServicesBase, self).tearDown()
+        d = self.services.stopService()
+        # The log file must be read in right after services have stopped,
+        # before the temporary directory where the log lives is removed.
+        d.addBoth(lambda ignore: self.addDetailFromLog())
+        d.addBoth(lambda ignore: self.assertNoObserversLeftBehind())
+        return d
+
+    def addDetailFromLog(self):
+        content = content_from_file(self.log_filename, buffer_now=True)
+        self.addDetail("log", content)
+
+    def assertNoObserversLeftBehind(self):
+        self.assertEqual(self.observers, theLogPublisher.observers)
+
+
+class TestLogService(TestServicesBase, TestCase):
+    """Tests for `txlongpoll.services.LogService`."""
+
+    def test_log_to_stdout(self):
+        log_service = LogService("-")
+        log_service.setServiceParent(self.services)
+        self.assertIsInstance(log_service.observer, FileLogObserver)
+        self.assertEqual("-", log_service.filename)
+        self.assertEqual(sys.stdout, log_service.logfile)
+        # The SIGUSR1 signal handler is untouched.
+        self.assertEqual(
+            signal.getsignal(signal.SIGUSR1),
+            signal.SIG_DFL)
+
+    def test_log_to_file(self):
+        tempdir = self.useFixture(TempDir()).path
+        log_filename = os.path.join(tempdir, "test.log")
+        log_service = LogService(log_filename)
+        log_service.setServiceParent(self.services)
+        self.assertIsInstance(log_service.observer, FileLogObserver)
+        self.assertEqual(log_filename, log_service.filename)
+        self.assertIsInstance(log_service.logfile, LogFile)
+        self.assertEqual(log_filename, log_service.logfile.path)
+        # The SIGUSR1 signal handler is set.
+        self.assertEqual(
+            signal.getsignal(signal.SIGUSR1),
+            log_service._signal_handler)
+
+
+class TestOOPSService(TestServicesBase, TestCase):
+    """Tests for `txlongpoll.services.OOPSService`."""
+
+    def setUp(self):
+        super(TestOOPSService, self).setUp()
+        # OOPSService relies upon LogService.
+        self.tempdir = self.useFixture(TempDir()).path
+        self.log_filename = os.path.join(self.tempdir, "test.log")
+        self.log_service = LogService(self.log_filename)
+        self.log_service.setServiceParent(self.services)
+
+    def test_minimal(self):
+        oops_service = OOPSService(self.log_service, None, None)
+        oops_service.setServiceParent(self.services)
+        observer = oops_service.observer
+        self.assertIsInstance(observer, OOPSObserver)
+        self.assertEqual([], observer.config.publishers)
+        self.assertEqual({}, observer.config.template)
+
+    def test_with_all_params(self):
+        oops_dir = os.path.join(self.tempdir, "oops")
+        oops_service = OOPSService(self.log_service, oops_dir, "Sidebottom")
+        oops_service.setServiceParent(self.services)
+        observer = oops_service.observer
+        self.assertIsInstance(observer, OOPSObserver)
+        self.assertEqual(1, len(observer.config.publishers))
+        self.assertEqual(
+            {"reporter": "Sidebottom"},
+            observer.config.template)


Follow ups