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..b849b32b12d915597bbb64e33dad91feb71fb64c 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"
@@ -27,9 +29,7 @@ class TestOperation : public MessageTransferOperation {
  public:
   TestOperation(const std::vector<cryptauth::RemoteDevice>& devices_to_connect,
                 BleConnectionManager* connection_manager)
-      : MessageTransferOperation(devices_to_connect, connection_manager),
-        has_operation_started_(false),
-        has_operation_finished_(false) {}
+      : MessageTransferOperation(devices_to_connect, connection_manager) {}
   ~TestOperation() override {}
 
   bool HasDeviceAuthenticated(const cryptauth::RemoteDevice& remote_device) {
@@ -62,6 +62,9 @@ class TestOperation : public MessageTransferOperation {
       const cryptauth::RemoteDevice& remote_device) override {
     device_map_[remote_device].received_messages.push_back(
         std::move(message_wrapper));
+
+    if (should_unregister_device_on_message_received_)
+      UnregisterDevice(remote_device);
   }
 
   void OnOperationStarted() override { has_operation_started_ = true; }
@@ -72,6 +75,26 @@ class TestOperation : public MessageTransferOperation {
     return kTestMessageType;
   }
 
+  bool ShouldWaitForResponse() override { return should_wait_for_response_; }
+
+  uint32_t GetResponseTimeoutSeconds() override {
+    return response_timeout_seconds_;
+  }
+
+  void set_should_wait_for_response(bool should_wait_for_response) {
+    should_wait_for_response_ = should_wait_for_response;
+  }
+
+  void set_response_timeout_seconds(uint32_t response_timeout_seconds) {
+    response_timeout_seconds_ = response_timeout_seconds;
+  }
+
+  void set_should_unregister_device_on_message_received(
+      bool should_unregister_device_on_message_received) {
+    should_unregister_device_on_message_received_ =
+        should_unregister_device_on_message_received;
+  }
+
   bool has_operation_started() { return has_operation_started_; }
 
   bool has_operation_finished() { return has_operation_finished_; }
@@ -87,8 +110,22 @@ class TestOperation : public MessageTransferOperation {
 
   std::map<cryptauth::RemoteDevice, DeviceMapValue> device_map_;
 
-  bool has_operation_started_;
-  bool has_operation_finished_;
+  bool should_wait_for_response_ = true;
+  uint32_t response_timeout_seconds_ = 5;
+  bool should_unregister_device_on_message_received_ = false;
+  bool has_operation_started_ = false;
+  bool has_operation_finished_ = false;
+};
+
+class TestTimerFactory : public TimerFactory {
+ public:
+  ~TestTimerFactory() override {}
+
+  // TimerFactory:
+  std::unique_ptr<base::Timer> CreateOneShotTimer() override {
+    return base::MakeUnique<base::MockTimer>(false /* retain_user_task */,
+                                             false /* is_repeating */);
+  }
 };
 
 DeviceStatus CreateFakeDeviceStatus() {
@@ -133,6 +170,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 +195,12 @@ class MessageTransferOperationTest : public testing::Test {
     EXPECT_EQ(has_finished, operation_->has_operation_finished());
   }
 
+  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_;
@@ -246,6 +290,7 @@ TEST_F(MessageTransferOperationTest, TestFailsThenConnects) {
       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]));
 
   EXPECT_TRUE(operation_->GetReceivedMessages(test_devices_[0]).empty());
 }
@@ -256,6 +301,47 @@ TEST_F(MessageTransferOperationTest,
   InitializeOperation();
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
 
+  // Simulate how subclasses behave after a successful response: unregister the
+  // device.
+  operation_->set_should_unregister_device_on_message_received(true);
+
+  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]));
+  EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[0]));
+
+  fake_ble_connection_manager_->ReceiveMessage(
+      test_devices_[0],
+      MessageWrapper(CreateTetherAvailabilityResponse()).ToRawMessage());
+
+  EXPECT_EQ(1u, operation_->GetReceivedMessages(test_devices_[0]).size());
+  std::shared_ptr<MessageWrapper> message =
+      operation_->GetReceivedMessages(test_devices_[0])[0];
+  EXPECT_EQ(MessageType::TETHER_AVAILABILITY_RESPONSE,
+            message->GetMessageType());
+  EXPECT_EQ(CreateTetherAvailabilityResponse().SerializeAsString(),
+            message->GetProto()->SerializeAsString());
+
+  EXPECT_FALSE(GetResponseTimerForDevice(test_devices_[0]));
+}
+
+TEST_F(MessageTransferOperationTest,
+       TestSuccessfulConnectionAndReceiveMessage_ResponseTimeoutSeconds) {
+  uint32_t response_timeout_seconds = 90;
+
+  ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
+  InitializeOperation();
+  EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
+
+  operation_->set_response_timeout_seconds(response_timeout_seconds);
+
   fake_ble_connection_manager_->SetDeviceStatus(
       test_devices_[0], cryptauth::SecureChannel::Status::CONNECTING);
   fake_ble_connection_manager_->SetDeviceStatus(
@@ -266,6 +352,10 @@ 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]));

[Kyle Horimoto, 2017/05/31 21:55:33]

You can combine the checks for having a response timer and asserting that the
timeout is correct (below) into one function (e.g.,
VerifyTimerCreatedWithCorrectDelay()), then call that from each test that needs
to test that.

You should also move the timeout value (initialized to 5 in TestOperation) to a
constant in the anonymous namespace so that you can assert that it is equal.

[Ryan Hansberry, 2017/06/01 00:31:58]

Done.

+
+  EXPECT_EQ(base::TimeDelta::FromSeconds(response_timeout_seconds),
+            GetResponseTimerForDevice(test_devices_[0])->GetCurrentDelay());
 
   fake_ble_connection_manager_->ReceiveMessage(
       test_devices_[0],
@@ -280,6 +370,66 @@ TEST_F(MessageTransferOperationTest,
             message->GetProto()->SerializeAsString());
 }
 
+TEST_F(MessageTransferOperationTest,
+       TestSuccessfulConnectionAndReceiveMessage_ShouldNotWaitForResponse) {
+  ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
+  InitializeOperation();
+  EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
+
+  operation_->set_should_wait_for_response(false);
+
+  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]));
+
+  // A Timer should not have been created because the operation should not wait
+  // for a response.
+  EXPECT_FALSE(GetResponseTimerForDevice(test_devices_[0]));
+
+  fake_ble_connection_manager_->ReceiveMessage(
+      test_devices_[0],
+      MessageWrapper(CreateTetherAvailabilityResponse()).ToRawMessage());
+
+  EXPECT_EQ(1u, operation_->GetReceivedMessages(test_devices_[0]).size());
+  std::shared_ptr<MessageWrapper> message =
+      operation_->GetReceivedMessages(test_devices_[0])[0];
+  EXPECT_EQ(MessageType::TETHER_AVAILABILITY_RESPONSE,
+            message->GetMessageType());
+  EXPECT_EQ(CreateTetherAvailabilityResponse().SerializeAsString(),
+            message->GetProto()->SerializeAsString());
+}
+
+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]));
+  EXPECT_TRUE(GetResponseTimerForDevice(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) {
   // Construct with two copies of the same device.
   ConstructOperation(
@@ -297,6 +447,7 @@ TEST_F(MessageTransferOperationTest, TestRepeatedInputDevice) {
       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]));
 
   fake_ble_connection_manager_->ReceiveMessage(
       test_devices_[0],
@@ -366,6 +517,7 @@ TEST_F(MessageTransferOperationTest,
   InitializeOperation();
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
   EXPECT_TRUE(operation_->HasDeviceAuthenticated(test_devices_[0]));
+  EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[0]));
 
   // Receiving a message should work at this point.
   fake_ble_connection_manager_->ReceiveMessage(
@@ -381,6 +533,36 @@ TEST_F(MessageTransferOperationTest,
             message->GetProto()->SerializeAsString());
 }
 
+TEST_F(MessageTransferOperationTest,
+       AlreadyAuthenticatedBeforeInitialization_TimesOutWaitingForResponse) {
+  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]));
+  EXPECT_TRUE(GetResponseTimerForDevice(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();
@@ -402,6 +584,7 @@ TEST_F(MessageTransferOperationTest, MultipleDevices) {
       test_devices_[0], cryptauth::SecureChannel::Status::AUTHENTICATED);
   EXPECT_TRUE(operation_->HasDeviceAuthenticated(test_devices_[0]));
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
+  EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[0]));
 
   // Fail 3 times to connect to |test_devices_[1]|.
   fake_ble_connection_manager_->SetDeviceStatus(
@@ -418,6 +601,7 @@ TEST_F(MessageTransferOperationTest, MultipleDevices) {
       test_devices_[1], cryptauth::SecureChannel::Status::DISCONNECTED);
   EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[1]));
   EXPECT_FALSE(IsDeviceRegistered(test_devices_[1]));
+  EXPECT_FALSE(GetResponseTimerForDevice(test_devices_[1]));
 
   // Authenticate |test_devices_[2]|'s channel.
   fake_ble_connection_manager_->RegisterRemoteDevice(
@@ -432,6 +616,7 @@ TEST_F(MessageTransferOperationTest, MultipleDevices) {
       test_devices_[2], cryptauth::SecureChannel::Status::AUTHENTICATED);
   EXPECT_TRUE(operation_->HasDeviceAuthenticated(test_devices_[2]));
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[2]));
+  EXPECT_TRUE(GetResponseTimerForDevice(test_devices_[2]));
 
   // Fail to authenticate |test_devices_[3]|'s channel.
   fake_ble_connection_manager_->RegisterRemoteDevice(
@@ -446,6 +631,7 @@ TEST_F(MessageTransferOperationTest, MultipleDevices) {
       test_devices_[3], cryptauth::SecureChannel::Status::DISCONNECTED);
   EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[3]));
   EXPECT_FALSE(IsDeviceRegistered(test_devices_[3]));
+  EXPECT_FALSE(GetResponseTimerForDevice(test_devices_[3]));
 }
 
 }  // namespace tether