Decouple Blimp transport output from BlimpConnections using MessagePort.

blimp/net/tcp_transport_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <memory>
 #include <string>
 
 #include "base/message_loop/message_loop.h"
 #include "blimp/common/create_blimp_message.h"
 #include "blimp/common/proto/blimp_message.pb.h"
@@ skipping 10 matching lines @@
 #include "net/base/test_completion_callback.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
 using testing::SaveArg;
 
 namespace blimp {
 
 namespace {
 
+// The length of |kPayload1| and |kPayload2| must be identical.

[CJ, 2016/08/12 23:59:15]

Perhaps add why they must be identical? It seems to me it is because the
read_buffer_ and write_buffer need to be the same size, and if that is so, it
would be better to comment on that, versus the payloads that seem sort of
arbitrary. 

[Kevin M, 2016/08/15 17:35:09]

You're right, it does seem arbitrary. I removed that limitation by calculating
the correct buffer size using std::max(). I also moved the constants into
std::string (size() is more readable than arraysize())

+const char kPayload1[] = "foo";
+const char kPayload2[] = "bar";
+
 // Integration test for TCPEngineTransport and TCPClientTransport.
 class TCPTransportTest : public testing::Test {
  protected:
-  TCPTransportTest() {
-    net::IPEndPoint local_address(net::IPAddress(127, 0, 0, 1), 0);
-    engine_.reset(new TCPEngineTransport(local_address, nullptr));
+  TCPTransportTest()
+      : local_address_(net::IPAddress(127, 0, 0, 1), 0),
+        engine_(local_address_, nullptr),
+        write_buffer_(
+            new net::DrainableIOBuffer(new net::IOBuffer(arraysize(kPayload1)),
+                                       arraysize(kPayload1))),
+        read_buffer_(new net::GrowableIOBuffer) {
+    read_buffer_->SetCapacity(arraysize(kPayload1));
   }
 
   net::IPEndPoint GetLocalEndpoint() const {
     net::IPEndPoint local_address;
-    CHECK_EQ(net::OK, engine_->GetLocalAddress(&local_address));
+    CHECK_EQ(net::OK, engine_.GetLocalAddress(&local_address));
     return local_address;
   }
 
   base::MessageLoopForIO message_loop_;
-  std::unique_ptr<TCPEngineTransport> engine_;
+  net::IPEndPoint local_address_;
+  TCPEngineTransport engine_;
+  scoped_refptr<net::DrainableIOBuffer> write_buffer_;
+  scoped_refptr<net::GrowableIOBuffer> read_buffer_;
 };
 
 TEST_F(TCPTransportTest, Connect) {
   net::TestCompletionCallback accept_callback;
-  engine_->Connect(accept_callback.callback());
+  engine_.Connect(accept_callback.callback());
 
   net::TestCompletionCallback connect_callback;
   TCPClientTransport client(GetLocalEndpoint(), nullptr);
   client.Connect(connect_callback.callback());
 
   EXPECT_EQ(net::OK, connect_callback.WaitForResult());
   EXPECT_EQ(net::OK, accept_callback.WaitForResult());
-  EXPECT_TRUE(engine_->TakeConnection() != nullptr);
+  EXPECT_NE(nullptr, client.TakeMessagePort());
 }
 
 TEST_F(TCPTransportTest, TwoClientConnections) {
   net::TestCompletionCallback accept_callback1;
-  engine_->Connect(accept_callback1.callback());
+  engine_.Connect(accept_callback1.callback());
 
   net::TestCompletionCallback connect_callback1;
-  TCPClientTransport client1(GetLocalEndpoint(), nullptr);
-  client1.Connect(connect_callback1.callback());
+  TCPClientTransport client(GetLocalEndpoint(), nullptr);
+  client.Connect(connect_callback1.callback());
+  EXPECT_EQ(net::OK, connect_callback1.WaitForResult());
+  EXPECT_EQ(net::OK, accept_callback1.WaitForResult());
+  EXPECT_NE(nullptr, engine_.TakeMessagePort());
+
+  net::TestCompletionCallback accept_callback2;
+  engine_.Connect(accept_callback2.callback());
 
   net::TestCompletionCallback connect_callback2;
   TCPClientTransport client2(GetLocalEndpoint(), nullptr);
   client2.Connect(connect_callback2.callback());
-
-  EXPECT_EQ(net::OK, connect_callback1.WaitForResult());
-  EXPECT_EQ(net::OK, accept_callback1.WaitForResult());
-  EXPECT_TRUE(engine_->TakeConnection() != nullptr);
-
-  net::TestCompletionCallback accept_callback2;
-  engine_->Connect(accept_callback2.callback());
   EXPECT_EQ(net::OK, connect_callback2.WaitForResult());
   EXPECT_EQ(net::OK, accept_callback2.WaitForResult());
-  EXPECT_TRUE(engine_->TakeConnection() != nullptr);
+  EXPECT_NE(nullptr, engine_.TakeMessagePort());
 }
 
 TEST_F(TCPTransportTest, ExchangeMessages) {
   // Start the Engine transport and connect a client to it.
   net::TestCompletionCallback accept_callback;
-  engine_->Connect(accept_callback.callback());
-  net::TestCompletionCallback client_connect_callback;
+  engine_.Connect(accept_callback.callback());
+  net::TestCompletionCallback clientconnect_callback;

[CJ, 2016/08/12 23:59:15]

Is there a reason to remove the underscore?

[Kevin M, 2016/08/15 17:35:09]

Looks like I typo'd. :\

Thanks, fixed!

   TCPClientTransport client(GetLocalEndpoint(), nullptr);
-  client.Connect(client_connect_callback.callback());
-  EXPECT_EQ(net::OK, client_connect_callback.WaitForResult());
+  client.Connect(clientconnect_callback.callback());
   EXPECT_EQ(net::OK, accept_callback.WaitForResult());
+  EXPECT_EQ(net::OK, clientconnect_callback.WaitForResult());
 
-  // Expect the engine to get two messages from the client, and the client to
-  // get one from the engine.
-  MockBlimpMessageProcessor engine_incoming_processor;
-  MockBlimpMessageProcessor client_incoming_processor;
-  net::CompletionCallback engine_process_message_cb;
-  std::unique_ptr<BlimpMessage> client_message1 =
-      CreateStartConnectionMessage("", 0);
-  int client_message1_size = client_message1->ByteSize();
-  std::unique_ptr<BlimpMessage> client_message2 = CreateCheckpointAckMessage(5);
-  std::unique_ptr<BlimpMessage> engine_message = CreateCheckpointAckMessage(10);
-  EXPECT_CALL(engine_incoming_processor,
-              MockableProcessMessage(EqualsProto(*client_message1), _))
-      .WillOnce(SaveArg<1>(&engine_process_message_cb));
-  EXPECT_CALL(engine_incoming_processor,
-              MockableProcessMessage(EqualsProto(*client_message2), _))
-      .Times(1);
-  EXPECT_CALL(client_incoming_processor,
-              MockableProcessMessage(EqualsProto(*engine_message), _))
-      .Times(1);
+  std::unique_ptr<MessagePort> engine_message_port = engine_.TakeMessagePort();
+  std::unique_ptr<MessagePort> clientmessage_port = client.TakeMessagePort();

[CJ, 2016/08/12 23:59:15]

why are engine variables following the underscore rule, but not the client
variables?

[Kevin M, 2016/08/15 17:35:09]

"engine_" is a class member, which have trailing underscores. "client" is
defined in the function, so no underscores.

[CJ, 2016/08/15 18:17:42]

I was more meaning the typo issue, since it was repeated multiple places. 

 
-  // Attach the ends of the connection to our mock message-processors.
-  std::unique_ptr<BlimpConnection> engine_connnection =
-      engine_->TakeConnection();
-  std::unique_ptr<BlimpConnection> client_connnection = client.TakeConnection();
-  engine_connnection->SetIncomingMessageProcessor(&engine_incoming_processor);
-  client_connnection->SetIncomingMessageProcessor(&client_incoming_processor);
+  // Engine sends kPayload1 to client.
+  net::TestCompletionCallback read_cb1;
+  net::TestCompletionCallback write_cb1;
+  memcpy(write_buffer_->data(), kPayload1, arraysize(kPayload1));
+  engine_message_port->writer()->WritePacket(write_buffer_,
+                                             write_cb1.callback());
+  clientmessage_port->reader()->ReadPacket(read_buffer_, read_cb1.callback());
+  EXPECT_EQ(arraysize(kPayload1),
+            static_cast<size_t>(read_cb1.WaitForResult()));
+  EXPECT_EQ(net::OK, write_cb1.WaitForResult());
+  EXPECT_TRUE(
+      BufferStartsWith(read_buffer_.get(), arraysize(kPayload1), kPayload1));
 
-  // Client sends the first message.
-  net::TestCompletionCallback client_send_callback1;
-  client_connnection->GetOutgoingMessageProcessor()->ProcessMessage(
-      std::move(client_message1), client_send_callback1.callback());
-  EXPECT_EQ(net::OK, client_send_callback1.WaitForResult());
-
-  // Engine finishes processing the client message.
-  EXPECT_FALSE(engine_process_message_cb.is_null());
-  engine_process_message_cb.Run(client_message1_size);
-
-  // Engine sends one message.
-  net::TestCompletionCallback engine_send_callback;
-  engine_connnection->GetOutgoingMessageProcessor()->ProcessMessage(
-      std::move(engine_message), engine_send_callback.callback());
-  EXPECT_EQ(net::OK, engine_send_callback.WaitForResult());
-
-  // Client sends the second message.
-  net::TestCompletionCallback client_send_callback2;
-  client_connnection->GetOutgoingMessageProcessor()->ProcessMessage(
-      std::move(client_message2), client_send_callback2.callback());
-  EXPECT_EQ(net::OK, client_send_callback2.WaitForResult());
+  // Client sends kPayload2 to engine.
+  net::TestCompletionCallback read_cb2;
+  net::TestCompletionCallback write_cb2;
+  memcpy(write_buffer_->data(), kPayload2, arraysize(kPayload2));
+  clientmessage_port->writer()->WritePacket(write_buffer_,
+                                            write_cb2.callback());
+  engine_message_port->reader()->ReadPacket(read_buffer_, read_cb2.callback());
+  EXPECT_EQ(arraysize(kPayload2),
+            static_cast<size_t>(read_cb2.WaitForResult()));
+  EXPECT_EQ(net::OK, write_cb2.WaitForResult());
+  EXPECT_TRUE(
+      BufferStartsWith(read_buffer_.get(), arraysize(kPayload2), kPayload2));
 }
 
 }  // namespace
 
 }  // namespace blimp