Refactor BlimpConnection to TCPConnection

blimp/net/engine_connection_manager_unittest.cc

Index: blimp/net/engine_connection_manager_unittest.cc
diff --git a/blimp/net/engine_connection_manager_unittest.cc b/blimp/net/engine_connection_manager_unittest.cc
index bd67c77e0471143fadc00f9ee4089b05bfcb2864..8201c9a1897158ace6b0db18d336801b90a63184 100644
--- a/blimp/net/engine_connection_manager_unittest.cc
+++ b/blimp/net/engine_connection_manager_unittest.cc
@@ -13,13 +13,15 @@
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "blimp/net/blimp_connection.h"
 #include "blimp/net/blimp_transport.h"
 #include "blimp/net/common.h"
 #include "blimp/net/connection_error_observer.h"
+#include "blimp/net/tcp_client_transport.h"
+#include "blimp/net/tcp_connection.h"
 #include "blimp/net/test_common.h"
 #include "net/base/completion_callback.h"
 #include "net/base/io_buffer.h"
+#include "net/base/ip_address.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -35,61 +37,43 @@ namespace blimp {
 class EngineConnectionManagerTest : public testing::Test {
  public:
   EngineConnectionManagerTest()
-      : manager_(new EngineConnectionManager(&connection_handler_)) {}
+      : manager_(new EngineConnectionManager(&connection_handler_, nullptr)) {}
 
   ~EngineConnectionManagerTest() override {}
 
-  std::unique_ptr<BlimpConnection> CreateConnection() {
-    return base::MakeUnique<BlimpConnection>(
-        base::MakeUnique<MessagePort>(base::MakeUnique<MockPacketReader>(),
-                                      base::MakeUnique<MockPacketWriter>()));
-  }
-
  protected:
   testing::StrictMock<MockConnectionHandler> connection_handler_;
   std::unique_ptr<EngineConnectionManager> manager_;
+  base::MessageLoopForIO message_loop_;
 };
 
 TEST_F(EngineConnectionManagerTest, ConnectionSucceeds) {
-  std::unique_ptr<testing::StrictMock<MockTransport>> transport1(
-      new testing::StrictMock<MockTransport>());
-  std::unique_ptr<testing::StrictMock<MockTransport>> transport2(
-      new testing::StrictMock<MockTransport>());
-
-  net::CompletionCallback connect_cb_1;
-  net::CompletionCallback connect_cb_2;
-  EXPECT_CALL(*transport1, Connect(_))
-      .Times(2)
-      .WillRepeatedly(SaveArg<0>(&connect_cb_1));
-  EXPECT_CALL(*transport2, Connect(_))
-      .Times(2)
-      .WillRepeatedly(SaveArg<0>(&connect_cb_2));
-
-  std::unique_ptr<MessagePort> port1 =
-      base::MakeUnique<MessagePort>(base::MakeUnique<MockPacketReader>(),
-                                    base::MakeUnique<MockPacketWriter>());
-  std::unique_ptr<MessagePort> port2 =
-      base::MakeUnique<MessagePort>(base::MakeUnique<MockPacketReader>(),
-                                    base::MakeUnique<MockPacketWriter>());
-  EXPECT_CALL(connection_handler_, HandleConnectionPtr(_)).Times(2);
-
-  EXPECT_CALL(*transport1, TakeMessagePortPtr())
-      .WillOnce(Return(port1.release()));
-  EXPECT_CALL(*transport2, TakeMessagePortPtr())
-      .WillOnce(Return(port2.release()));
-
-  ASSERT_TRUE(connect_cb_1.is_null());
-  manager_->AddTransport(std::move(transport1));
-  ASSERT_FALSE(connect_cb_1.is_null());
-
-  ASSERT_TRUE(connect_cb_2.is_null());
-  manager_->AddTransport(std::move(transport2));
-  ASSERT_FALSE(connect_cb_2.is_null());
-
-  base::ResetAndReturn(&connect_cb_1).Run(net::OK);
-  base::ResetAndReturn(&connect_cb_2).Run(net::OK);
-  ASSERT_FALSE(connect_cb_1.is_null());
-  ASSERT_FALSE(connect_cb_2.is_null());
+  EXPECT_CALL(connection_handler_, HandleConnectionPtr(_)).Times(1);
+
+  // We explicitly do not rely on peeking at the connection's port as different
+  // implementations will have different ways of connecting to a client.
+  // Instead, we retry for a freely available port number and use that instead.
+  int port = 25467;

[Garrett Casto, 2016/10/25 18:33:50]

I would think that you would want to do what the browser_tests due. That is, use
port 0 to select a random free port and then use that. Retry like this is a code
smell, you don't want the allocated ports to change test behavior.

[Kevin M, 2016/10/25 18:49:23]

+1, this is why we need the local listening port getter.

[perumaal, 2016/10/25 23:02:39]

Again, this was to avoid relying on ports at all in the first place.
But I can do it as a separate change and perhaps only for gRPC.
Also, I didn't want to pollute the interface with a GetLocalAddress. 
I think it's ok to rely on ports for now especially as this would be in
GrpcEngineTransport and not the GrpcStream interface, which is kind of nice.
(Re: File-descriptors + gRPC)

+
+  do {
+    net::IPAddress ip_addr(127, 0, 0, 1);

[Garrett Casto, 2016/10/25 18:33:50]

Nit: Use IPAddress::IPv4Localhost.

[perumaal, 2016/10/25 23:02:39]

Done.

+    net::IPEndPoint engine_endpoint(ip_addr, port);
+
+    manager_->ConnectTransport(
+        engine_endpoint,
+        EngineConnectionManager::EngineTransportType::TCP);
+
+    net::TestCompletionCallback connect_callback;
+    TCPClientTransport client(engine_endpoint, nullptr);
+    client.Connect(connect_callback.callback());
+    auto client_connect_result = connect_callback.WaitForResult();
+    if (client_connect_result != net::OK) {
+      ++port;
+      LOG(ERROR) << "Retrying with a different port : " << port;
+    } else {
+      break;
+    }
+  } while (port > 0);
 }
 
 }  // namespace blimp