Code review comment for lp:~alecu/ubuntuone-client/proxy-tunnel-client

Revision history for this message
Manuel de la Peña (mandel) wrote :

In TunnelClientProtocolTestCase I see that we do a number of times the following:

75 + other_proto = protocol.Protocol()
76 + other_factory = protocol.ClientFactory()
77 + other_factory.buildProtocol = lambda _addr: other_proto
78 + tunnel_client_factory = tunnel_client.TunnelClientFactory(host, port,
79 + other_factory)
80 + tunnel_client_proto = tunnel_client_factory.buildProtocol(fake_addr)
81 + tunnel_client_proto.transport = FakeTransport()
82 + tunnel_client_proto.connectionMade()

Would't it be a good idea to move that to the setUp?

In TunnelClientTestCase you refactor the tests as follows:

             @defer.inlineCallbacks
             def _assert_connection(self, is_ssl=False):
                 """Asseert the connection."""
                 tunnel_client = TunnelClient("0.0.0.0", self.tunnel_server.port)
                 factory = client.HTTPClientFactory(self.dest_url)
                 scheme, host, port, path = client._parse(self.dest_url)
                 if is_ssl:
                     context_factory = ssl.ClientContextFactory()
                     tunnel_client.connectSSL(host, port, factory, context_factory)
                 else:
                     tunnel_client.connectTCP(host, port, factory)
                 result = yield factory.deferred
                 self.assertEqual(result, SAMPLE_CONTENT)

            def test_connects_right(self):
                """Uses the CONNECT method on the tunnel."""
                self._assert_connection()

            def test_starts_tls_connection(self):
                """TLS is started after connecting; control passed to the client."""
                self._assert_connection(is_ssl=True)

Although if you think it does not bring anything to the code you can leave the tests as they are.

Since all the above comments are just about tests and writing less code I'll approve and will trust you to apply or not the comments if you feel they are needed :)

review: Approve

« Back to merge proposal