Code review comment for lp:~free.ekanayaka/landscape-client/amp-cleanup-6

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Hi Chris, thanks for the careful review. I've filed:

http://twistedmatrix.com/trac/ticket/6502

describing the issue.

[1]

You're right, that code is only meant for unit tests. I've renamed MethodCallClientFactory.connection to fake_connection and added a comment:

    # XXX support exposing fake asynchronous connections created by tests, so
    # they can be flushed transparently and emulate a synchronous behavior. See
    # also http://twistedmatrix.com/trac/ticket/6502, once that's fixed this
    # hack can be removed.
    fake_connection = None

The FakeReactor.connect_unix is the place where it gets set for unit-tests:

    connection = FakeConnection(factory.buildProtocol(path),
                                server.buildProtocol(path))
    factory.fake_connection = connection

[2]

Yeah, that's indeed a hack. The problem is that FakeReactor instances for clients and servers need to be different. I added a comment:

    # XXX probably this shouldn't be a class attribute, but we need client-side
    # FakeReactor instaces to be aware of listening sockets created by
    # server-side FakeReactor instances.
    _socket_paths = {}

Maybe the way to solve it would be to add API for declaring a reactor "aware" of another reactor (so tests would need to call it explicitly). Suggestions are welcome.

« Back to merge proposal