Tether MessageTransferOperation: Only wait for a response from a host for a certain amount of time …

chromeos/components/tether/message_transfer_operation_unittest.cc

Index: chromeos/components/tether/message_transfer_operation_unittest.cc
diff --git a/chromeos/components/tether/message_transfer_operation_unittest.cc b/chromeos/components/tether/message_transfer_operation_unittest.cc
index 5db05a38893b61d6e1f8589d3786a34fb20c0bd3..0f007d5b1dae51ace10d2cd028be3d519c0cfe97 100644
--- a/chromeos/components/tether/message_transfer_operation_unittest.cc
+++ b/chromeos/components/tether/message_transfer_operation_unittest.cc
@@ -4,8 +4,10 @@
 
 #include "chromeos/components/tether/message_transfer_operation.h"
 
+#include "base/timer/mock_timer.h"
 #include "chromeos/components/tether/fake_ble_connection_manager.h"
 #include "chromeos/components/tether/message_wrapper.h"
+#include "chromeos/components/tether/timer_factory.h"
 #include "components/cryptauth/remote_device_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -91,6 +93,16 @@ class TestOperation : public MessageTransferOperation {
   bool has_operation_finished_;
 };
 
+class TestTimerFactory : public TimerFactory {
+ public:
+  ~TestTimerFactory() override {}
+
+  // TimerFactory:
+  std::unique_ptr<base::Timer> CreateOneShotTimer() override {
+    return base::MakeUnique<base::MockTimer>(false, false);

[khorimoto, 2017/05/31 17:26:36]

/* parameter_names */

[Ryan Hansberry, 2017/05/31 21:19:04]

Done.

+  }
+};
+
 DeviceStatus CreateFakeDeviceStatus() {
   WifiStatus wifi_status;
   wifi_status.set_status_code(
@@ -133,6 +145,7 @@ class MessageTransferOperationTest : public testing::Test {
   void ConstructOperation(std::vector<cryptauth::RemoteDevice> remote_devices) {
     operation_ = base::WrapUnique(
         new TestOperation(remote_devices, fake_ble_connection_manager_.get()));
+    operation_->SetTimerFactoryForTest(base::MakeUnique<TestTimerFactory>());
     VerifyOperationStartedAndFinished(false /* has_started */,
                                       false /* has_finished */);
   }
@@ -157,6 +170,16 @@ class MessageTransferOperationTest : public testing::Test {
     EXPECT_EQ(has_finished, operation_->has_operation_finished());
   }
 
+  void UnregisterDevice(const cryptauth::RemoteDevice& remote_device) {
+    operation_->UnregisterDevice(remote_device);
+  }
+
+  base::MockTimer* GetResponseTimerForDevice(
+      const cryptauth::RemoteDevice& remote_device) {
+    return static_cast<base::MockTimer*>(
+        operation_->GetResponseTimerForDevice(remote_device));
+  }
+
   const std::vector<cryptauth::RemoteDevice> test_devices_;
 
   std::unique_ptr<FakeBleConnectionManager> fake_ble_connection_manager_;
@@ -266,6 +289,7 @@ TEST_F(MessageTransferOperationTest,
       test_devices_[0], cryptauth::SecureChannel::Status::AUTHENTICATED);
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
   EXPECT_TRUE(operation_->HasDeviceAuthenticated(test_devices_[0]));
+  EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[0]));

[khorimoto, 2017/05/31 17:26:36]

Test that timers are created correctly in all tests in which you expect one to
be created, not only this one.

[Ryan Hansberry, 2017/05/31 21:19:04]

Done.

 
   fake_ble_connection_manager_->ReceiveMessage(
       test_devices_[0],
@@ -278,6 +302,34 @@ TEST_F(MessageTransferOperationTest,
             message->GetMessageType());
   EXPECT_EQ(CreateTetherAvailabilityResponse().SerializeAsString(),
             message->GetProto()->SerializeAsString());
+
+  // Simulate how subclasses behave after a successful response: unregister the

[khorimoto, 2017/05/31 17:26:36]

Instead of manually calling UnregisterDevice(), update TestOperation above to
include an option for unregistering after a response is received.

[Ryan Hansberry, 2017/05/31 21:19:04]

Done.

+  // device.
+  UnregisterDevice(test_devices_[0]);
+  EXPECT_FALSE(GetResponseTimerForDevice(test_devices_[0]));
+}
+
+TEST_F(MessageTransferOperationTest, TestAuthenticatesButTimesOut) {
+  ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
+  InitializeOperation();
+  EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
+
+  fake_ble_connection_manager_->SetDeviceStatus(
+      test_devices_[0], cryptauth::SecureChannel::Status::CONNECTING);
+  fake_ble_connection_manager_->SetDeviceStatus(
+      test_devices_[0], cryptauth::SecureChannel::Status::CONNECTED);
+  fake_ble_connection_manager_->SetDeviceStatus(
+      test_devices_[0], cryptauth::SecureChannel::Status::AUTHENTICATING);
+  fake_ble_connection_manager_->SetDeviceStatus(
+      test_devices_[0], cryptauth::SecureChannel::Status::AUTHENTICATED);
+  EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
+  EXPECT_TRUE(operation_->HasDeviceAuthenticated(test_devices_[0]));
+
+  GetResponseTimerForDevice(test_devices_[0])->Fire();
+
+  EXPECT_FALSE(IsDeviceRegistered(test_devices_[0]));
+  EXPECT_TRUE(operation_->has_operation_finished());
+  EXPECT_FALSE(GetResponseTimerForDevice(test_devices_[0]));
 }
 
 TEST_F(MessageTransferOperationTest, TestRepeatedInputDevice) {
@@ -381,6 +433,35 @@ TEST_F(MessageTransferOperationTest,
             message->GetProto()->SerializeAsString());
 }
 
+TEST_F(MessageTransferOperationTest,
+       AuthenticatesButTimesOutWaitingForResponse) {

[khorimoto, 2017/05/31 17:26:36]

nit: Change name to reflect the fact that this test authenticates before
initialization.

[Ryan Hansberry, 2017/05/31 21:19:04]

Done.

+  ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
+
+  // Simulate the authentication of |test_devices_[0]|'s channel before
+  // initialization.
+  fake_ble_connection_manager_->RegisterRemoteDevice(
+      test_devices_[0], MessageType::CONNECT_TETHERING_REQUEST);
+  fake_ble_connection_manager_->SetDeviceStatus(
+      test_devices_[0], cryptauth::SecureChannel::Status::CONNECTING);
+  fake_ble_connection_manager_->SetDeviceStatus(
+      test_devices_[0], cryptauth::SecureChannel::Status::CONNECTED);
+  fake_ble_connection_manager_->SetDeviceStatus(
+      test_devices_[0], cryptauth::SecureChannel::Status::AUTHENTICATING);
+  fake_ble_connection_manager_->SetDeviceStatus(
+      test_devices_[0], cryptauth::SecureChannel::Status::AUTHENTICATED);
+
+  // Now initialize; the authentication handler should have been invoked.
+  InitializeOperation();
+  EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
+  EXPECT_TRUE(operation_->HasDeviceAuthenticated(test_devices_[0]));
+
+  GetResponseTimerForDevice(test_devices_[0])->Fire();
+
+  EXPECT_FALSE(IsDeviceRegistered(test_devices_[0]));
+  EXPECT_TRUE(operation_->has_operation_finished());
+  EXPECT_FALSE(GetResponseTimerForDevice(test_devices_[0]));
+}
+
 TEST_F(MessageTransferOperationTest, MultipleDevices) {
   ConstructOperation(test_devices_);
   InitializeOperation();