Improve integration of SerialPort (via an endpoint)

[brandl]

There is no easy way to create a SerialPort instance and use it with twistd (like e.g. TCPServer). It would be nice if the SerialPort could be treated in the same way, or at least similarly.

This can be accomplished by implementing a serial endpoint that can be instantiated with (server|client)FromString (*). Use the plugin framework and implement an appropriate string parser to extend the existing machinery.

See also the discussion on the mailing list, starting with  http://twistedmatrix.com/pipermail/twisted-python/2011-January/023429.html

(*) I don't know if a serial port is more like a server or a client. Maybe this depends on the application...?

[ashfall]

(In [34676]) Branching to 'serialports-endpoint-4847'

[ashfall]

 Build Results

A few points:

• The imports become tricky if there isn't serial support in the system, I've used some workarounds to avoid them for now, let me know if it needs more work.

• Also, in the build results, this error appears repeatedly:

===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/buildslave/slaves/twisted/bot-idnar-debian/easy2.6debian/Twisted/install/lib/Twisted-12.1.0-py2.6-linux-x86_64.egg/twisted/internet/test/test_serialport.py", line 66, in test_connectionMadeLost
    port = DummySerialPort(SerialProtocol(), "", reactor=DoNothing())
exceptions.TypeError: __init__() takes exactly 11 non-keyword arguments (4 given)

twisted.internet.test.test_serialport.SerialPortTests.test_connectionMadeLost
-------------------------------------------------------------------------------

But I'm not able to reproduce this locally. Not sure what's to be done here.

[exarkun]

Thanks! Sorry about the long review delay. I lost track of this ticket :(

When you started work on this, I said I couldn't think of any compelling reason to make the endpoint a client rather than a server. Unfortunately, now I can.

IStreamServerEndpoint.listen is defined as returning a Deferred which fires with an IListeningPort. The branch implements this method to fire the Deferred with a SerialPort instance, and SerialPort does not implement IListeningPort. I see two options for addressing this:

1. Create an IListeningPort implementation to use with serial ports. This would probably be fairly simple, but I think it involves understanding and addressing reconnection semantics for serial ports (a somewhat convoluted topic with many platform-specific intricacies). I suspect that eventually we will want to address this, but perhaps it isn't the highest priority right now.
2. Create an IStreamClientEndpoint implementation for serial ports, instead. (I recommend this one for now)

Apart from that, here are a few general review comments to consider, but which may not be directly actionable, depending on what code is left after choosing and implementing one of the above strategies:

1. Some trailing whitespace is added and should be cleaned up
2. Everything in test modules is already private, so you don't need to rename StdioFactory to _StdioFactory.
3. When testing that parameters are passed properly, it's a good idea to give each of them different values. If you pass 1 for each of timeout, xonxoff, and rtscts, then the tests can't really demonstrate that arguments are passed in the right order. Maybe you swapped timeout and rtscts, but since they're both 1, who knows.
4. It's probably okay to skip testing the private attributes on the endpoint. It's sufficient to test that the actual SerialPort gets passed the right arguments, based on the arguments passed to SerialPortEndpoint. The intermediate step of private attributes as storage for those values is an uninteresting implementation detail (you may see tests that do this, but they're either misguided or else using this testing approach because a better approach was too difficult in that particular case).
5. Be sure to use something like TestCase.patch to do monkey-patching, so things get cleaned up.

Thanks again.

[exarkun]

Oh, I forgot to mention - the strange exception you saw is related to the non-use of TestCase.patch in test_serialPortInstanceParam. One you fix the latter, the former issue will disappear.

[ashfall]

 New build results.

Replying to exarkun:

Thanks for the review.

1. Create an IListeningPort implementation to use with serial ports. This would probably be fairly simple, but I think it involves understanding and addressing reconnection semantics for serial ports (a somewhat convoluted topic with many platform-specific intricacies). I suspect that eventually we will want to address this, but perhaps it isn't the highest priority right now.

Should I file a ticket for this, since it is something that needs to be done in the future?

3. When testing that parameters are passed properly, it's a good idea to give each of them different values. If you pass 1 for each of timeout, xonxoff, and rtscts, then the tests can't really demonstrate that arguments are passed in the right order. Maybe you swapped timeout and rtscts, but since they're both 1, who knows.

Sorry about that.

5. Be sure to use something like TestCase.patch to do monkey-patching, so things get cleaned up.

Done, and the exception went away. Thanks.

[exarkun]

Should I file a ticket for this, since it is something that needs to be done in the future?

Sure, that'd be great.

When testing that parameters are passed properly, it's a good idea to give each of them different values. If you pass 1 for each of timeout, xonxoff, and rtscts, then the tests can't really demonstrate that arguments are passed in the right order. Maybe you swapped timeout and rtscts, but since they're both 1, who knows.

Sorry about that.

No apology needed :) However, I notice that the test in the branch still uses 0s for all three parameters?

Also, one other thing I noticed is that xonxoff and rtscts are both booleans, so we may as well use True and False instead of 1 and 0 for them.

One comment I forgot in the previous review was about the address passed to the factory's buildProtocol. We should invent a new SerialAddress (along the lines of the other new addresses you've been adding) and use that here.

And lastly, the Deferred connect returns is still not firing with the right value. It needs to fire with the protocol instance, not the SerialPort instance.

When testing that parameters are passed properly, it's a good idea to give each of them different values. If you pass 1 for each of timeout, xonxoff, and rtscts, then the tests can't really demonstrate that arguments are passed in the right order. Maybe you swapped timeout and rtscts, but since they're both 1, who knows.

I guess I didn't do a good job explaining the idea here, so let me try again. Consider the following:

class FooTests(TestCase):
    def setUp(self):
        self.foo = Foo(123)

    def test_bar(self):
        """
        Foo.bar creates a new Bar with the same parameters given to Foo's initializer.
        """
        bar = self.foo.bar()
        self.assertEqual(self.foo.x, bar.x)

This test is inadequate to accomplish its stated goal, because it relies on something it assumes Foo.__init__ does, but which it never actually tests that it does.

Consider this implementation:

class Bar:
    def __init__(self, x):
        self.x = x

class Foo:
    def __init__(self, x):
        self._x = 10

    def bar(self):
        return Bar(self._x)

This will pass the unit test, but not actually behave correctly. While I think that your implementation of SerialPortEndpoint is correct (unlike the above code), the tests you have for it suffer from this problem.

To resolve this, make sure to verify Bar.x against the known good value. For example:

class FooTests(TestCase):
    expectedX = 123

    def setUp(self):
        self.foo = Foo(self.expectedX)

    def test_bar(self):
        """
        Foo.bar creates a new Bar with the same parameters given to Foo's initializer.
        """
        bar = self.foo.bar()
        self.assertEqual(self.expectedX, bar.x)

Thanks again!

[ashfall]

Thanks for the review. Addressed the review comments.  New build results

[exarkun]

Thanks.

1. Some trivial conflicts, please merge forward to resolve.
2. in test_rtscts, there's a comment suggesting the default value for xonxoff is passed, which is soooort of true. A value *is* passed, and it is the default. But this is a bit in contradiction to a typical meaning for "the default value", which is not passing any value for the argument at all (as is done for rtscts in test_xonxoff. I'd either drop the comment or drop the explicit False value for xonxoff.
3. There's a missing test for the exception handling in SerialPortEndpoint.connect
4. If the serialport module is not installed, connect will raise an exception instead of returning a Deferred. Likewise if buildProtocol raises an exception. These aren't common error conditions, but they should probably both result in a Deferred that fires with a failure instead of causing an exception to be raised by connect. I'd be satisfied to see these addressed in a followup ticket, but you can fix them here too if you'd prefer.

[ashfall]

(In [35245]) Branching to 'serialports-endpoint-4847-2'

[ashfall]

Thanks for the review, changes made.  New build results

[exarkun]

(In [36523]) Branching to 'serialports-endpoint-4847-3'

[exarkun]

There were some nasty conflicts caused by the Python 3 porting work. I merged forward and resolved those for you. Because of the Python 3 porting work, there's also some Python 3 related comments in this review. Sorry!

1. implements is now implementer. You can see an example of the new spelling in twisted/internet/address.py. All the uses of implements in Python 3-ported modules need to be replaced with implementer, and generally all new code should use implementer. Since twisted/internet/address.py is a ported module, this needs to be done for SerialAddress.
2. twisted/internet/endpoints.py is not a ported module, but the implements call in SerialPortEndpoint should probably be switched as well.
3. There's some new helpers in trunk now, successResultNow and failureResultNow. failureResultNow might be useful, for example, in test_connectFailure. The current code is fine too, but the helper makes the code a bit nicer and shorter.
4. It might be nice to have a test for the case where pyserial (or whatever that module is called) isn't installed. In this case connect should return a Deferred that fails with an ImportError, I think. Having a test that exercises this case would be nice.

Thanks!

[ashfall]

Replying to exarkun:

 Build results

Thanks! Made the changes. If there's a better way to deal with the imports in this endpoint, please do tell in the review.

[tom.prince]

Have a look at twisted.trial.test.test_reporter.AnsiColorizerTests for an example if how to test the behavior of a module that may not be available.

1. The line endpoint._serialport = None in test_importError is masking an AttributeError that gets raised if connect is called when pyserial isn't installed.
2. It doesn't look like any of your tests require a working twisted.internet.serialport module, just that that module exists, so they should be able to be written so that they work even if pyserial isn't available. (i.e. creating a fake module as in AnsiColorizerTests)
3. (minor) When docstrings are referring to instance attributes or arguments, they should be wrapped with C{}. There are a number of instances of this in the test docstrings.
4. The class level comment about expected values isn't clear. Those values are the values passed in setUp when constructing the endpoint, thus what is expected in test_serialPortInstanceParameters. It would perhaps be clearer to just use the default arguments for all the tests except that one (and the others that test specific arguments), and move that block of definitions there.
5. (optional) Use the new deferred helpers, to make the tests synchronous.
6. There are some fairly straightforward merge conflicts.

Thanks again for your persistence. Please address the above and resubmit.