Merge lp:~spiv/twisted/lineReceiver-bug-3277 into lp:twisted

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

I'm largely submitting this Launchpad merge proposal to experiment with
Launchpad, although I think the patch is good.)

To run the tests in this branch requires lp:testscenarios to be installed. I'm
not sure what to do about that, but the easy improvement in test coverage is
probably worth doing something to accomodate that.

This patch simply adds a TestCase that defines tests that both LineReceiver and
LineOnlyReceiver should meet. It turns out that actually there are some
inconsistencies in how these two classes behave for their supposedly common
features, so this patch also fixes the code so that the tests pass.

One unfixed issue I noticed is that LineOnlyReceiver does not implement the
IPushProducer interface, but LineReceiver does. I'm not sure if this is
intentional or not, and adding the feature to LineOnlyReceiver wasn't completely
trivial so I've punted on this issue and made that particular scenario skip.

=== modified file 'twisted/protocols/basic.py'
--- twisted/protocols/basic.py	2008-12-14 21:41:04 +0000
+++ twisted/protocols/basic.py	2009-07-04 14:02:58 +0000
@@ -133,7 +133,8 @@
 
     def dataReceived(self, data):
         """Translates bytes into lines, and calls lineReceived."""
-        lines  = (self._buffer+data).split(self.delimiter)
+        dataSoFar = self._buffer + data
+        lines = dataSoFar.split(self.delimiter)
         self._buffer = lines.pop(-1)
         for line in lines:
             if self.transport.disconnecting:
@@ -143,7 +144,7 @@
                 # the one that told it to close.
                 return
             if len(line) > self.MAX_LENGTH:
-                return self.lineLengthExceeded(line)
+                return self.lineLengthExceeded(dataSoFar)
             else:
                 self.lineReceived(line)
         if len(self._buffer) > self.MAX_LENGTH:
@@ -163,6 +164,7 @@
         """Called when the maximum line length has been reached.
         Override if it needs to be dealt with in some special way.
         """
+        self.transport.loseConnection()
         return error.ConnectionLost('Line length exceeded')
 
 
@@ -232,7 +234,7 @@
             else:
                 linelength = len(line)
                 if linelength > self.MAX_LENGTH:
-                    exceeded = line + self.__buffer
+                    exceeded = line + self.delimiter + self.__buffer
                     self.__buffer = ''
                     return self.lineLengthExceeded(exceeded)
                 why = self.lineReceived(line)
@@ -291,7 +293,8 @@
         be more than one line, or may be only the initial portion of the
         line.
         """
-        return self.transport.loseConnection()
+        self.transport.loseConnection()
+        return error.ConnectionLost('Line length exceeded')
 
 
 class StringTooLongError(AssertionError):

=== modified file 'twisted/test/test_protocols.py'
--- twisted/test/test_protocols.py	2009-02-14 21:04:19 +0000
+++ twisted/test/test_protocols.py	2009-07-04 14:15:06 +0000
@@ -7,7 +7,7 @@
 
 import struct
 
-from twisted.trial import unittest
+from twisted.trial import unittest, runner
 from twisted.protocols import basic, wire, portforward
 from twisted.internet import reactor, protocol, defer, task, error
 from twisted.test import proto_helpers
@@ -85,25 +85,6 @@
             self.setLineMode(line[self.MAX_LENGTH + 1:])
 
 
-class LineOnlyTester(basic.LineOnlyReceiver):
-    """
-    A buffering line only receiver.
-    """
-    delimiter = '\n'
-    MAX_LENGTH = 64
-
-    def connectionMade(self):
-        """
-        Create/clean data received on connection.
-        """
-        self.received = []
-
-    def lineReceived(self, line):
-        """
-        Save received data.
-        """
-        self.received.append(line)
-
 class WireTestCase(unittest.TestCase):
     """
     Test wire protocols.
@@ -300,7 +281,10 @@
 
 class LineOnlyReceiverTestCase(unittest.TestCase):
     """
-    Test line only receiveer.
+    Test line only receiver interface.
+
+    This test case is applied to both LineReceiver and LineOnlyReceiver by the
+    test_suite function.
     """
     buffer = """foo
     bleakness
@@ -308,26 +292,103 @@
     plastic forks
     """
 
+    scenarios = [
+        ('LineReceiver', {'klass': basic.LineReceiver}),
+        ('LineOnlyReceiver', {'klass': basic.LineOnlyReceiver})]
+
+    def makeLineReceiver(self):
+        """
+        Construct a line receiver for testing with.
+
+        It will be a simple subclass of whatever self.lineReceiverClass is.
+        The subclass will log calls to lineReceived and lineLengthExceeded to a
+        '.calls' attribute.
+        """
+        transport = protocol.FileWrapper(proto_helpers.StringIOWithoutClosing())
+        baseClass = self.klass
+        class LoggingLineReceiver(baseClass):
+            """
+            A line receiver subclass that records calls made to it, but
+            otherwise behaves like its base class.
+            """
+            delimiter = '\n'
+            MAX_LENGTH = 64
+
+            def connectionMade(self):
+                self.calls = []
+                return baseClass.connectionMade(self)
+
+            def lineReceived(self, line):
+                self.calls.append(('lineReceived', line))
+
+            def lineLengthExceeded(self, line):
+                self.calls.append(('lineLengthExceeded', line))
+                return baseClass.lineLengthExceeded(self, line)
+            
+        lineReceiver = LoggingLineReceiver()
+        lineReceiver.makeConnection(transport)
+        return lineReceiver
+
     def test_buffer(self):
         """
         Test buffering over line protocol: data received should match buffer.
         """
-        t = proto_helpers.StringTransport()
-        a = LineOnlyTester()
-        a.makeConnection(t)
+        lineReceiver = self.makeLineReceiver()
         for c in self.buffer:
-            a.dataReceived(c)
-        self.assertEquals(a.received, self.buffer.split('\n')[:-1])
+            lineReceiver.dataReceived(c)
+        expectedLines = self.buffer.split('\n')[:-1]
+        expectedCalls = [('lineReceived', line) for line in expectedLines]
+        self.assertEquals(expectedCalls, lineReceiver.calls)
 
     def test_lineTooLong(self):
         """
-        Test sending a line too long: it should close the connection.
+        When a line greater than MAX_LENGTH is received, lineLengthExceeded is
+        called.  The default implementation lineLengthExceeded closes the
+        connection, and returns a ConnectionLost error.
         """
-        t = proto_helpers.StringTransport()
-        a = LineOnlyTester()
-        a.makeConnection(t)
-        res = a.dataReceived('x'*200)
+        lineReceiver = self.makeLineReceiver()
+        res = lineReceiver.dataReceived('x'*200)
+        self.failUnlessEqual(
+            [('lineLengthExceeded', 'x'*200)], lineReceiver.calls)
+        self.assertTrue(lineReceiver.transport.closed)
         self.assertIsInstance(res, error.ConnectionLost)
+        self.assertEqual(('Line length exceeded',), res.args)
+
+    def testLongLineWithDelimiter(self):
+        """
+        When MAX_LENGTH is exceeded *and* a delimiter has been received,
+        lineLengthExceeded is called with the right bytes.
+
+        See http://twistedmatrix.com/trac/ticket/3277
+        """
+        # Set up a line receiver with a short MAX_LENGTH that logs
+        # lineLengthExceeded events.
+        lineReceiver = self.makeLineReceiver()
+        lineReceiver.MAX_LENGTH = 10
+        # Call dataReceived with two lines, the first longer than MAX_LENGTH.
+        longLine = ('x' * 11) + '\n'
+        nextLine = 'next line\n'
+        lineReceiver.dataReceived(longLine + nextLine)
+        # We expect lineLengthExceeded to be called with exactly what we just
+        # passed dataReceived.  lineReceived is not called.
+        expectedCalls = [('lineLengthExceeded', longLine + nextLine)]
+        self.assertEqual(expectedCalls, lineReceiver.calls)
+
+    def testLineReceiverAsProducer(self):
+        """
+        Test produce/unproduce in receiving.
+        """
+        lineReceiver = self.makeLineReceiver()
+        if isinstance(lineReceiver, basic.LineOnlyReceiver):
+            raise unittest.SkipTest(
+                'LineOnlyReceiver does not implement IPushProducer')
+        lineReceiver.transport.registerProducer(lineReceiver, False)
+        lineReceiver.dataReceived('hello world\n')
+        lineReceiver.transport.unregisterProducer()
+        lineReceiver.dataReceived('goodbye\n')
+        self.assertEquals(
+            [('lineReceived', 'hello world'), ('lineReceived', 'goodbye')],
+            lineReceiver.calls)
 
 
 
@@ -778,3 +839,15 @@
         """
         s = proto_helpers.StringTransport()
         self.assertRaises(TypeError, s.write, u'foo')
+
+
+def test_suite():
+    import twisted.test.test_protocols as this_module
+    from testscenarios import generate_scenarios
+    loader = runner.TestLoader()
+    result = unittest.TestSuite()
+    tests = []
+    for testClass in loader.findTestClasses(this_module):
+        tests.append(loader.loadClass(testClass))
+    result.addTests(generate_scenarios(tests))
+    return result