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

chromeos/components/tether/message_transfer_operation_unittest.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.

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

You don't have any tests here for when the operation does NOT want to wait for a
response. Please add them. You'll probably want to add another setter function
on TestOperation to change this.

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

Done. Also added a test for different response timeout.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 
 namespace chromeos {
 
 namespace tether {
 
 namespace {
 
@@ skipping 65 matching lines @@
     bool has_device_authenticated;
     std::vector<std::shared_ptr<MessageWrapper>> received_messages;
   };
 
   std::map<cryptauth::RemoteDevice, DeviceMapValue> device_map_;
 
   bool has_operation_started_;
   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(
       WifiStatus_StatusCode::WifiStatus_StatusCode_CONNECTED);
   wifi_status.set_ssid("Google A");
 
   DeviceStatus device_status;
   device_status.set_battery_percentage(75);
   device_status.set_cell_provider("Google Fi");
   device_status.set_connection_strength(4);
@@ skipping 22 matching lines @@
     EXPECT_EQ(3u, MessageTransferOperation::kMaxConnectionAttemptsPerDevice);
   }
 
   void SetUp() override {
     fake_ble_connection_manager_ = base::MakeUnique<FakeBleConnectionManager>();
   }
 
   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 */);
   }
 
   bool IsDeviceRegistered(const cryptauth::RemoteDevice& remote_device) const {
     DCHECK(operation_);
     return std::find(operation_->remote_devices_.begin(),
                      operation_->remote_devices_.end(),
                      remote_device) != operation_->remote_devices_.end();
   }
 
   void InitializeOperation() {
     VerifyOperationStartedAndFinished(false /* has_started */,
                                       false /* has_finished */);
     operation_->Initialize();
     VerifyOperationStartedAndFinished(true /* has_started */,
                                       false /* has_finished */);
   }
 
   void VerifyOperationStartedAndFinished(bool has_started, bool has_finished) {
     EXPECT_EQ(has_started, operation_->has_operation_started());
     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_;
   std::unique_ptr<TestOperation> operation_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MessageTransferOperationTest);
 };
 
 TEST_F(MessageTransferOperationTest, TestCannotConnectAndReachesRetryLimit) {
@@ skipping 89 matching lines @@
   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]));

[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],
       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());
+
+  // 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) {
   // Construct with two copies of the same device.
   ConstructOperation(
       std::vector<cryptauth::RemoteDevice>{test_devices_[0], test_devices_[0]});
   InitializeOperation();
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
 
   fake_ble_connection_manager_->SetDeviceStatus(
@@ skipping 83 matching lines @@
 
   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,
+       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();
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[0]));
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[1]));
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[2]));
   EXPECT_TRUE(IsDeviceRegistered(test_devices_[3]));
 
   // Authenticate |test_devices_[0]|'s channel.
   fake_ble_connection_manager_->RegisterRemoteDevice(
@@ skipping 50 matching lines @@
       test_devices_[3], cryptauth::SecureChannel::Status::AUTHENTICATING);
   fake_ble_connection_manager_->SetDeviceStatus(
       test_devices_[3], cryptauth::SecureChannel::Status::DISCONNECTED);
   EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[3]));
   EXPECT_FALSE(IsDeviceRegistered(test_devices_[3]));
 }
 
 }  // namespace tether
 
 }  // namespace chromeos