Merge lp:~allenap/txlongpoll/oops-amqp into lp:txlongpoll

[Gavin Panella]

Report OOPSes via AMQP.

This builds on Rob's work and adds some simple tests for the plugin code.

[lp:~allenap/txlongpoll/oops-amqp]

97. By Gavin Panella on 2011-11-07

Simplify options_diff().

[Raphaël Badin (rvb)]

This branch looks good.

[0]

Maybe the tests would be more clear if they tested one thing each like this: http://paste.ubuntu.com/731893/ (warning: I've not even run that code ;) ). What do you think?

[1]

In test_parse_int_options, why do you test the option values parsed *and* the default options, why not a simple test that the option value is what it should be (like 1234 for the frontend port)? (you already have a test for the default option values)

[2]

Maybe:
s/test_with_all_the_things/test_with_all_the_params
s/test_with_some_of_the_things/test_with_some_of_the_params
;)

review: Approve

[lp:~allenap/txlongpoll/oops-amqp]

98. By Gavin Panella on 2011-11-08

Convenience test script.

99. By Gavin Panella on 2011-11-08

Split up test_parse_minimal_options() as suggested by rvb.

100. By Gavin Panella on 2011-11-08

Simplify test_parse_int_options().

101. By Gavin Panella on 2011-11-08

Remove humour from test names :)

102. By Gavin Panella on 2011-11-08

Add testrepository support.

103. By Gavin Panella on 2011-11-08

Create the txlongpoll.log in a temp dir.

[Gavin Panella (allenap)]

Raphaël, I've made all the excellent changes you suggested. I've also
added testrepository support, a convenience script for testing, and
fixed a situation where a test was leaving a log file in the tree, but
these are fairly trivial so r=me for my own changes.

Thanks for the review!

=== modified file '.bzrignore'
--- .bzrignore	2011-06-28 14:52:07 +0000
+++ .bzrignore	2011-11-08 11:40:27 +0000
@@ -11,3 +11,4 @@
 ./parts
 ./tags
 dropin.cache
+./.testrepository

=== added file '.testr.conf'
--- .testr.conf	1970-01-01 00:00:00 +0000
+++ .testr.conf	2011-11-08 11:40:27 +0000
@@ -0,0 +1,5 @@
+[DEFAULT]
+test_command = bin/testpy -m subunit/run -- discover $LISTOPT $IDOPTION
+test_id_option = --load-list $IDFILE
+# The "ignore" value below works around a bug in subunit.
+test_list_option = --list=ignore

=== modified file 'Makefile'
--- Makefile	2011-11-07 11:28:33 +0000
+++ Makefile	2011-11-08 11:40:27 +0000
@@ -22,8 +22,8 @@
 	$(BUILDOUT)
 
 
-check: bin/test
-	bin/test -vv
+check: bin/testpy
+	bin/testpy -m subunit/run -- discover
 
 
 dist: $(BUILDOUT_BIN)
@@ -58,7 +58,7 @@
 	$(BUILDOUT) install runtime
 
 
-bin/test: $(BUILDOUT_BIN) $(BUILDOUT_CFG) setup.py
+bin/testpy: $(BUILDOUT_BIN) $(BUILDOUT_CFG) setup.py
 	$(BUILDOUT) install test
 
 

=== added file 'NEWS'
--- NEWS	1970-01-01 00:00:00 +0000
+++ NEWS	2011-11-08 11:40:27 +0000
@@ -0,0 +1,11 @@
+txlongpoll NEWS
++++++++++++++++
+
+Changes and improvements to txlongpoll, grouped by release.
+
+NEXT
+----
+
+* OOPS reports can be reported directly over AMQP if the oops-exchange option
+  is given. The oops-prefix option has been renamed to oops-reporter to fit in
+  with the long term terminology oops has adopted. (Robert Collins)

=== modified file 'README'
--- README	2011-11-07 12:03:18 +0000
+++ README	2011-11-08 11:40:27 +0000
@@ -46,6 +46,14 @@
 
 will just do the first part.
 
+There is testrepository <https://launchpad.net/testrepository>
+support, so you can also do the following:
+
+ $ testr init
+ $ testr run
+
+This is probably the best way to develop txlongpoll.
+
 
 Running
 -------

=== modified file 'buildout.cfg'
--- buildout.cfg	2011-11-04 20:09:20 +0000
+++ buildout.cfg	2011-11-08 11:40:27 +0000
@@ -20,13 +20,12 @@
 [runtime]
 recipe = zc.recipe.egg:scripts
 eggs = txlongpoll
-entry-points =  twistd=twisted.scripts.twistd:run
-interpreter = py
+entry-points = twistd=twisted.scripts.twistd:run
 
 [test]
-recipe = zc.recipe.testrunner
+recipe = zc.recipe.egg:scripts
 eggs = txlongpoll [test]
-defaults = '--tests-pattern ^tests --exit-with-status'.split()
+interpreter = testpy
 
 [tags]
 recipe = z3c.recipe.tag:tags

=== modified file 'setup.py'
--- setup.py	2011-11-04 16:51:11 +0000
+++ setup.py	2011-11-08 11:40:27 +0000
@@ -18,6 +18,7 @@
     zip_safe=False,
     description='Long polling HTTP frontend for AMQP',
     install_requires=[
+        'oops_amqp',
         'oops_datedir_repo',
         'oops_twisted >= 0.0.3',
         'setproctitle',
@@ -30,5 +31,6 @@
             'rabbitfixture',
             'testresources >= 0.2.4_r58',
             'testtools',
+            # 'subunit',  # Not easy-installable.
             ],
         ))

=== added file 'test'
--- test	1970-01-01 00:00:00 +0000
+++ test	2011-11-08 11:40:27 +0000
@@ -0,0 +1,2 @@
+#!/usr/bin/env bash
+exec "$(dirname "$0")/bin/testpy" -m subunit/run -- "$@"

=== modified file 'twisted/plugins/txlongpoll.py'
--- twisted/plugins/txlongpoll.py	2011-10-06 20:15:29 +0000
+++ twisted/plugins/txlongpoll.py	2011-11-08 11:40:27 +0000
@@ -3,144 +3,9 @@
 
 from __future__ import absolute_import
 
-import signal
-import sys
-
-from oops_datedir_repo import DateDirRepo
-from oops_twisted import (
-    Config as oops_config,
-    defer_publisher,
-    OOPSObserver,
-    )
-import setproctitle
-from twisted.application.internet import (
-    TCPClient,
-    TCPServer,
-    )
-from twisted.application.service import (
-    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 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(*args):
-            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-dir"]:
-        repo = DateDirRepo(options["oops-dir"], options["oops-prefix"])
-        config.publishers.append(defer_publisher(repo.publish))
-
-    observer = OOPSObserver(config, logfile.emit)
-    addObserver(observer.emit)
-
-
-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-prefix", "o", "LONGPOLL", "String prefix for OOPS IDs"],
-        ]
-
-    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)
-
-
-class AMQServiceMaker(object):
-    """Create an asynchronous frontend server for AMQP."""
-
-    implements(IServiceMaker, IPlugin)
-
-    options = Options
-
-    def __init__(self, name, description):
-        self.tapname = name
-        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))
-        services = MultiService()
-        services.addService(client_service)
-        services.addService(server_service)
-
-        return services
-
-
-# Now construct objects which *provide* the relevant interfaces. The name of
+from txlongpoll.plugin import AMQServiceMaker
+
+# Construct objects which *provide* the relevant interfaces. The name of
 # these variables is irrelevant, as long as there are *some* names bound to
 # providers of IPlugin and IServiceMaker.
 

=== added file 'txlongpoll/plugin.py'
--- txlongpoll/plugin.py	1970-01-01 00:00:00 +0000
+++ txlongpoll/plugin.py	2011-11-08 11:40:27 +0000
@@ -0,0 +1,172 @@
+# Copyright 2005-2011 Canonical Ltd.  This software is licensed under
+# the GNU Affero General Public License version 3 (see the file LICENSE).
+
+__all__ = [
+    "AMQServiceMaker",
+    ]
+
+from functools import partial
+import signal
+import sys
+
+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,
+    )
+import setproctitle
+from twisted.application.internet import (
+    TCPClient,
+    TCPServer,
+    )
+from twisted.application.service import (
+    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 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 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."],
+        ]
+
+    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."""
+
+    implements(IServiceMaker, IPlugin)
+
+    options = Options
+
+    def __init__(self, name, description):
+        self.tapname = name
+        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))
+        services = MultiService()
+        services.addService(client_service)
+        services.addService(server_service)
+
+        return services

=== added file 'txlongpoll/tests/test_plugin.py'
--- txlongpoll/tests/test_plugin.py	1970-01-01 00:00:00 +0000
+++ txlongpoll/tests/test_plugin.py	2011-11-08 11:40:27 +0000
@@ -0,0 +1,237 @@
+# 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
+import os
+from unittest import defaultTestLoader
+
+from fixtures import TempDir
+from oops_twisted import OOPSObserver
+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,
+    Options,
+    setUpOOPSHandler,
+    )
+
+
+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,
+            }
+        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",
+            ]
+        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`."""
+
+    def test_init(self):
+        service_maker = AMQServiceMaker("Harry", "Hill")
+        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})
+        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])
+
+
+def test_suite():
+    return defaultTestLoader.loadTestsFromName(__name__)