Revert "[CrOS Tether] Update connection retry attempt logic."

This reverts commit d5bd9807.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 533073 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2Q1YmQ5ODA3ZTViYTI0OTBiYzQxNjViN2FlNzg0OTJkOWJlNDgyMGEM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/4001

Original change's description:
> [CrOS Tether] Update connection retry attempt logic.
> 
> Before this CL, we allowed 3 retries for the entire connection flow,
> which includes advertising, scanning, creating a GATT connection, and
> exchanging messages. However, it makes more sense to think of this as a
> two-part process:
>   (1) Advertising and scanning. This portion always occurs when
>       attempting a BLE connection, even if the device we're trying to
>       contact is not nearby and cannot respond.
>   (2) Creating a GATT connection and exchanging message. This portion
>       only occurs when an Android host is nearby to respond to the
>       Chromebook.
> 
> Connection attempts which fail in part (2) above indicate that the
> device actually is nearby and can potentially connect; additionally
> connection failures during this part of the connection often will
> succeed on a retry. Thus, this CL adds extra connection retry attempts
> if a previous attempt failed in part (2).
> 
> Bug: 805218, 672263
> Change-Id: Ifbedc004b96905d3f77662c45bcf2a9210b06c57
> Reviewed-on: https://chromium-review.googlesource.com/889991
> Reviewed-by: Jeremy Klein <jlklein@chromium.org>
> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#533073}

Change-Id: I60d645c8baf8c53eb46cbf9eb9341046db7e0c31
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 805218, 672263
Reviewed-on: https://chromium-review.googlesource.com/894290
Cr-Commit-Position: refs/heads/master@{#533160}

chromeos/components/tether/connect_tethering_operation_unittest.cc

@@ -32,6 +32,8 @@ namespace {
 const char kTestSsid[] = "testSsid";
 const char kTestPassword[] = "testPassword";
 
+const size_t kMaxConnectionAttemptsPerDevice = 3;
+
 constexpr base::TimeDelta kConnectTetheringResponseTime =
     base::TimeDelta::FromSeconds(15);
 
@@ -111,7 +113,11 @@ class ConnectTetheringOperationTest : public testing::Test {
   ConnectTetheringOperationTest()
       : connect_tethering_request_string_(
             CreateConnectTetheringRequestString()),
-        test_device_(cryptauth::GenerateTestRemoteDevices(1)[0]) {}
+        test_device_(cryptauth::GenerateTestRemoteDevices(1)[0]) {
+    // These tests are written under the assumption that there are a maximum of
+    // 3 connection attempts; they need to be edited if this value changes.
+    EXPECT_EQ(3u, MessageTransferOperation::kMaxConnectionAttemptsPerDevice);
+  }
 
   void SetUp() override {
     fake_ble_connection_manager_ = std::make_unique<FakeBleConnectionManager>();
@@ -272,9 +278,8 @@ TEST_F(ConnectTetheringOperationTest, TestCannotConnect) {
       .Times(0);
 
   // Simulate the device failing to connect.
-  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
-      test_device_.GetDeviceId(),
-      MessageTransferOperation::kMaxEmptyScansPerDevice);
+  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
+      test_device_.GetDeviceId(), kMaxConnectionAttemptsPerDevice);
 
   // The maximum number of connection failures has occurred.
   EXPECT_TRUE(test_observer_->has_received_failure());

chromeos/components/tether/fake_ble_connection_manager.cc

@@ -61,7 +61,7 @@ void FakeBleConnectionManager::SetMessageSent(int sequence_number) {
   NotifyMessageSent(sequence_number);
 }
 
-void FakeBleConnectionManager::SimulateUnansweredConnectionAttempts(
+void FakeBleConnectionManager::SimulateFailedConnectionAttempts(
     const std::string& device_id,
     size_t num_attempts) {
   for (size_t i = 0; i < num_attempts; ++i) {
@@ -73,22 +73,6 @@ void FakeBleConnectionManager::SimulateUnansweredConnectionAttempts(
   }
 }
 
-void FakeBleConnectionManager::SimulateGattErrorConnectionAttempts(
-    const std::string& device_id,
-    size_t num_attempts) {
-  for (size_t i = 0; i < num_attempts; ++i) {
-    SetDeviceStatus(device_id, cryptauth::SecureChannel::Status::CONNECTING,
-                    StateChangeDetail::STATE_CHANGE_DETAIL_NONE);
-    SetDeviceStatus(device_id, cryptauth::SecureChannel::Status::CONNECTED,
-                    StateChangeDetail::STATE_CHANGE_DETAIL_NONE);
-    SetDeviceStatus(device_id, cryptauth::SecureChannel::Status::AUTHENTICATING,
-                    StateChangeDetail::STATE_CHANGE_DETAIL_NONE);
-    SetDeviceStatus(
-        device_id, cryptauth::SecureChannel::Status::DISCONNECTED,
-        StateChangeDetail::STATE_CHANGE_DETAIL_GATT_CONNECTION_WAS_ATTEMPTED);
-  }
-}
-
 bool FakeBleConnectionManager::IsRegistered(const std::string& device_id) {
   return base::ContainsKey(device_id_map_, device_id);
 }

chromeos/components/tether/fake_ble_connection_manager.h

@@ -33,19 +33,11 @@ class FakeBleConnectionManager : public BleConnectionManager {
   void ReceiveMessage(const std::string& device_id, const std::string& payload);
   void SetMessageSent(int sequence_number);
 
-  // Simulates |num_attempts| consecutive failed "unanswered" connection
-  // attempts for the device with ID |device_id|. Specifically, this function
-  // updates the device's status to CONNECTING then DISCONNECTED on each
-  // attempt.
-  void SimulateUnansweredConnectionAttempts(const std::string& device_id,
-                                            size_t num_attempts);
-
-  // Simulates |num_attempts| consecutive failed "GATT error" connection
-  // attempts for the device with ID |device_id|. Specifically, this function
-  // updates the device's status to CONNECTING, then CONNECTED, then
-  // AUTHENTICATING, then DISCONNECTED on each attempt.
-  void SimulateGattErrorConnectionAttempts(const std::string& device_id,
-                                           size_t num_attempts);
+  // Simulates |num_attempts| consecutive failed connection attempts for the
+  // device with ID |device_id|. Specifically, this function updates the
+  // device's status to CONNECTING then DISCONNECTED on each attempt.
+  void SimulateFailedConnectionAttempts(const std::string& device_id,
+                                        size_t num_attempts);
 
   std::vector<SentMessage>& sent_messages() { return sent_messages_; }
   // Returns -1 if no sequence numbers have been used yet.

chromeos/components/tether/host_scanner_operation_unittest.cc

@@ -30,6 +30,8 @@ namespace tether {
 
 namespace {
 
+const size_t kMaxConnectionAttemptsPerDevice = 3;
+
 const char kDefaultCarrier[] = "Google Fi";
 
 constexpr base::TimeDelta kTetherAvailabilityResponseTime =
@@ -127,7 +129,11 @@ class HostScannerOperationTest : public testing::Test {
   HostScannerOperationTest()
       : tether_availability_request_string_(
             CreateTetherAvailabilityRequestString()),
-        test_devices_(cryptauth::GenerateTestRemoteDevices(5)) {}
+        test_devices_(cryptauth::GenerateTestRemoteDevices(5)) {
+    // These tests are written under the assumption that there are a maximum of
+    // 3 connection attempts; they need to be edited if this value changes.
+    EXPECT_EQ(3u, MessageTransferOperation::kMaxConnectionAttemptsPerDevice);
+  }
 
   void SetUp() override {
     fake_ble_connection_manager_ = std::make_unique<FakeBleConnectionManager>();
@@ -369,9 +375,8 @@ TEST_F(HostScannerOperationTest, TestMultipleDevices) {
       kTetherAvailabilityResponseTime, 2);
 
   // Simulate device 1 failing to connect.
-  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
-      test_devices_[1].GetDeviceId(),
-      MessageTransferOperation::kMaxEmptyScansPerDevice);
+  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
+      test_devices_[1].GetDeviceId(), kMaxConnectionAttemptsPerDevice);
 
   // The scan should still not be over, and no new scan results should have
   // come in.
@@ -379,9 +384,8 @@ TEST_F(HostScannerOperationTest, TestMultipleDevices) {
   EXPECT_EQ(2u, test_observer_->scanned_devices_so_far().size());
 
   // Simulate device 3 failing to connect.
-  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
-      test_devices_[3].GetDeviceId(),
-      MessageTransferOperation::kMaxEmptyScansPerDevice);
+  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
+      test_devices_[3].GetDeviceId(), kMaxConnectionAttemptsPerDevice);
 
   // The scan should still not be over, and no new scan results should have
   // come in.

chromeos/components/tether/keep_alive_operation_unittest.cc

@@ -22,6 +22,8 @@ namespace tether {
 
 namespace {
 
+const size_t kMaxConnectionAttemptsPerDevice = 3;
+
 constexpr base::TimeDelta kKeepAliveTickleResponseTime =
     base::TimeDelta::FromSeconds(3);
 
@@ -140,9 +142,8 @@ TEST_F(KeepAliveOperationTest, TestSendsKeepAliveTickleAndReceivesResponse) {
 
 TEST_F(KeepAliveOperationTest, TestCannotConnect) {
   // Simulate the device failing to connect.
-  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
-      test_device_.GetDeviceId(),
-      MessageTransferOperation::kMaxEmptyScansPerDevice);
+  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
+      test_device_.GetDeviceId(), kMaxConnectionAttemptsPerDevice);
 
   // The maximum number of connection failures has occurred.
   EXPECT_TRUE(test_observer_->has_run_callback());

chromeos/components/tether/message_transfer_operation.cc

@@ -34,6 +34,12 @@ std::vector<cryptauth::RemoteDevice> RemoveDuplicatesFromVector(
 
 }  // namespace
 
+// static
+uint32_t MessageTransferOperation::kMaxConnectionAttemptsPerDevice = 3;
+
+// static
+uint32_t MessageTransferOperation::kDefaultTimeoutSeconds = 10;
+
 MessageTransferOperation::MessageTransferOperation(
     const std::vector<cryptauth::RemoteDevice>& devices_to_connect,
     BleConnectionManager* connection_manager)
@@ -103,22 +109,42 @@ void MessageTransferOperation::OnSecureChannelStatusChanged(
     return;
   }
 
-  switch (new_status) {
-    case cryptauth::SecureChannel::Status::AUTHENTICATED:
-      StartTimerForDevice(*remote_device);
-      OnDeviceAuthenticated(*remote_device);
-      break;
-    case cryptauth::SecureChannel::Status::DISCONNECTED:
-      HandleDeviceDisconnection(*remote_device, status_change_detail);
-      break;
-    default:
-      // Note: In success cases, the channel advances from DISCONNECTED to
-      // CONNECTING to CONNECTED to AUTHENTICATING to AUTHENTICATED. If the
-      // channel fails to advance at any of those stages, it transitions back to
-      // DISCONNECTED and starts over. There is no need for special handling for
-      // any of these interim states since they will eventually progress to
-      // either AUTHENTICATED or DISCONNECTED.
-      break;
+  // TODO(khorimoto): Use |status_change_detail| to provide better retry
+  // handling. See https://crbug.com/805218.
+
+  if (new_status == cryptauth::SecureChannel::Status::AUTHENTICATED) {
+    StartTimerForDevice(*remote_device);
+    OnDeviceAuthenticated(*remote_device);
+  } else if (new_status == cryptauth::SecureChannel::Status::DISCONNECTED) {
+    // Note: In success cases, the channel advances from DISCONNECTED to
+    // CONNECTING to CONNECTED to AUTHENTICATING to AUTHENTICATED. If the
+    // channel fails to advance at any of those stages, it transitions back to
+    // DISCONNECTED and starts over.
+
+    uint32_t num_attempts_so_far;
+    if (remote_device_to_num_attempts_map_.find(*remote_device) ==
+        remote_device_to_num_attempts_map_.end()) {
+      num_attempts_so_far = 0;
+    } else {
+      num_attempts_so_far = remote_device_to_num_attempts_map_[*remote_device];
+    }
+
+    num_attempts_so_far++;
+    remote_device_to_num_attempts_map_[*remote_device] = num_attempts_so_far;
+
+    PA_LOG(INFO) << "Connection attempt failed for device with ID "
+                 << remote_device->GetTruncatedDeviceIdForLogs() << ". "
+                 << "Number of failures so far: " << num_attempts_so_far;
+
+    if (num_attempts_so_far >= kMaxConnectionAttemptsPerDevice) {
+      PA_LOG(INFO) << "Connection retry limit reached for device with ID "
+                   << remote_device->GetTruncatedDeviceIdForLogs() << ". "
+                   << "Unregistering device.";
+
+      // If the number of failures so far is equal to the maximum allowed number
+      // of connection attempts, give up and unregister the device.
+      UnregisterDevice(*remote_device);
+    }
   }
 }
 
@@ -148,7 +174,7 @@ void MessageTransferOperation::UnregisterDevice(
   // cause the original reference to be deleted.
   cryptauth::RemoteDevice remote_device_copy = remote_device;
 
-  remote_device_to_attempts_map_.erase(remote_device_copy);
+  remote_device_to_num_attempts_map_.erase(remote_device_copy);
   remote_devices_.erase(std::remove(remote_devices_.begin(),
                                     remote_devices_.end(), remote_device_copy),
                         remote_devices_.end());
@@ -177,70 +203,6 @@ void MessageTransferOperation::SetTimerFactoryForTest(
   timer_factory_ = std::move(timer_factory_for_test);
 }
 
-void MessageTransferOperation::HandleDeviceDisconnection(
-    const cryptauth::RemoteDevice& remote_device,
-    BleConnectionManager::StateChangeDetail status_change_detail) {
-  ConnectAttemptCounts& attempts_for_device =
-      remote_device_to_attempts_map_[remote_device];
-
-  switch (status_change_detail) {
-    case BleConnectionManager::StateChangeDetail::STATE_CHANGE_DETAIL_NONE:
-      PA_LOG(ERROR) << "State transitioned to DISCONNECTED, but no "
-                    << "StateChangeDetail was provided. Treating this as a "
-                    << "failure to discover the device.";
-    // Note: Intentional fall-through to next case.
-    case BleConnectionManager::StateChangeDetail::
-        STATE_CHANGE_DETAIL_COULD_NOT_ATTEMPT_CONNECTION:
-      ++attempts_for_device.empty_scan_attempts;
-      PA_LOG(INFO) << "Connection attempt failed; could not discover the "
-                   << "device with ID "
-                   << remote_device.GetTruncatedDeviceIdForLogs() << ". "
-                   << "Number of failures to establish connection: "
-                   << attempts_for_device.empty_scan_attempts;
-
-      if (attempts_for_device.empty_scan_attempts >= kMaxEmptyScansPerDevice) {
-        PA_LOG(INFO) << "Reached retry limit for failing to discover the "
-                     << "device with ID "
-                     << remote_device.GetTruncatedDeviceIdForLogs() << ". "
-                     << "Unregistering device.";
-        UnregisterDevice(remote_device);
-      }
-      break;
-    case BleConnectionManager::StateChangeDetail::
-        STATE_CHANGE_DETAIL_GATT_CONNECTION_WAS_ATTEMPTED:
-      ++attempts_for_device.gatt_connection_attempts;
-      PA_LOG(INFO) << "Connection attempt failed; GATT connection error for "
-                   << "device with ID "
-                   << remote_device.GetTruncatedDeviceIdForLogs() << ". "
-                   << "Number of GATT error: "
-                   << attempts_for_device.gatt_connection_attempts;
-
-      if (attempts_for_device.gatt_connection_attempts >=
-          kMaxGattConnectionAttemptsPerDevice) {
-        PA_LOG(INFO) << "Reached retry limit for GATT connection errors for "
-                     << "device with ID "
-                     << remote_device.GetTruncatedDeviceIdForLogs() << ". "
-                     << "Unregistering device.";
-        UnregisterDevice(remote_device);
-      }
-      break;
-    case BleConnectionManager::StateChangeDetail::
-        STATE_CHANGE_DETAIL_INTERRUPTED_BY_HIGHER_PRIORITY:
-      // If the connection attempt was interrupted by a higher-priority message,
-      // this is not a true failure. There is nothing to do until the next state
-      // change occurs.
-      break;
-    case BleConnectionManager::StateChangeDetail::
-        STATE_CHANGE_DETAIL_DEVICE_WAS_UNREGISTERED:
-      // This state change is expected to be handled as a result of calls to
-      // UnregisterDevice(). There is no need for special handling.
-      break;
-    default:
-      NOTREACHED();
-      break;
-  }
-}
-
 void MessageTransferOperation::StartTimerForDevice(
     const cryptauth::RemoteDevice& remote_device) {
   PA_LOG(INFO) << "Starting timer for operation with message type "

chromeos/components/tether/message_transfer_operation.h

@@ -23,21 +23,6 @@ class TimerFactory;
 // from remote devices.
 class MessageTransferOperation : public BleConnectionManager::Observer {
  public:
-  // The number of times to attempt to connect to a device without receiving any
-  // response before giving up. When a connection to a device is attempted, a
-  // BLE discovery session listens for advertisements from the remote device as
-  // the first step of the connection; if no advertisement is picked up, it is
-  // likely that the remote device is not nearby or is not currently responding
-  // to Instant Tethering requests.
-  static constexpr const uint32_t kMaxEmptyScansPerDevice = 3;
-
-  // The number of times to attempt a GATT connection to a device, after a BLE
-  // discovery session has already detected a nearby device. GATT connections
-  // may fail for a variety of reasons, but most failures are ephemeral. Thus,
-  // more connection attempts are allowed in such cases since it is likely that
-  // a subsequent attempt will succeed. See https://crbug.com/805218.
-  static constexpr const uint32_t kMaxGattConnectionAttemptsPerDevice = 6;
-
   MessageTransferOperation(
       const std::vector<cryptauth::RemoteDevice>& devices_to_connect,
       BleConnectionManager* connection_manager);
@@ -106,29 +91,22 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
   friend class HostScannerOperationTest;
   friend class MessageTransferOperationTest;
 
+  static uint32_t kMaxConnectionAttemptsPerDevice;
+
   // The default number of seconds an operation should wait before a timeout
   // occurs. Once this amount of time passes, the connection will be closed.
   // Classes deriving from MessageTransferOperation should override
   // GetTimeoutSeconds() if they desire a different duration.
-  static constexpr const uint32_t kDefaultTimeoutSeconds = 10;
+  static uint32_t kDefaultTimeoutSeconds;
 
-  struct ConnectAttemptCounts {
-    uint32_t empty_scan_attempts = 0;
-    uint32_t gatt_connection_attempts = 0;
-  };
-
-  void HandleDeviceDisconnection(
-      const cryptauth::RemoteDevice& remote_device,
-      BleConnectionManager::StateChangeDetail status_change_detail);
+  void SetTimerFactoryForTest(
+      std::unique_ptr<TimerFactory> timer_factory_for_test);
   void StartTimerForDevice(const cryptauth::RemoteDevice& remote_device);
   void StopTimerForDeviceIfRunning(
       const cryptauth::RemoteDevice& remote_device);
   void OnTimeout(const cryptauth::RemoteDevice& remote_device);
   cryptauth::RemoteDevice* GetRemoteDevice(const std::string& device_id);
 
-  void SetTimerFactoryForTest(
-      std::unique_ptr<TimerFactory> timer_factory_for_test);
-
   std::vector<cryptauth::RemoteDevice> remote_devices_;
   BleConnectionManager* connection_manager_;
   std::unique_ptr<TimerFactory> timer_factory_;
@@ -136,8 +114,8 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
   bool initialized_ = false;
   bool shutting_down_ = false;
   MessageType message_type_for_connection_;
-  std::map<cryptauth::RemoteDevice, ConnectAttemptCounts>
-      remote_device_to_attempts_map_;
+  std::map<cryptauth::RemoteDevice, uint32_t>
+      remote_device_to_num_attempts_map_;
   std::map<cryptauth::RemoteDevice, std::unique_ptr<base::Timer>>
       remote_device_to_timer_map_;
   base::WeakPtrFactory<MessageTransferOperation> weak_ptr_factory_;

chromeos/components/tether/message_transfer_operation_unittest.cc

@@ -21,6 +21,8 @@ namespace tether {
 
 namespace {
 
+const size_t kMaxConnectionAttemptsPerDevice = 3;
+
 // Arbitrarily chosen value. The MessageType used in this test does not matter
 // except that it must be consistent throughout the test.
 const MessageType kTestMessageType = MessageType::TETHER_AVAILABILITY_REQUEST;
@@ -157,11 +159,8 @@ class MessageTransferOperationTest : public testing::Test {
   MessageTransferOperationTest()
       : test_devices_(cryptauth::GenerateTestRemoteDevices(4)) {
     // These tests are written under the assumption that there are a maximum of
-    // 3 "empty scan" connection attempts and 6 "GATT" connection attempts; the
-    // tests need to be edited if these values change.
-    EXPECT_EQ(3u, MessageTransferOperation::kMaxEmptyScansPerDevice);
-    EXPECT_EQ(6u,
-              MessageTransferOperation::kMaxGattConnectionAttemptsPerDevice);
+    // 3 connection attempts; they need to be edited if this value changes.
+    EXPECT_EQ(3u, MessageTransferOperation::kMaxConnectionAttemptsPerDevice);
   }
 
   void SetUp() override {
@@ -241,7 +240,7 @@ class MessageTransferOperationTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(MessageTransferOperationTest);
 };
 
-TEST_F(MessageTransferOperationTest, CannotReceiveResponse_RetryLimitReached) {
+TEST_F(MessageTransferOperationTest, TestCannotConnectAndReachesRetryLimit) {
   ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
   InitializeOperation();
   EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
@@ -293,52 +292,6 @@ TEST_F(MessageTransferOperationTest, CannotReceiveResponse_RetryLimitReached) {
   EXPECT_TRUE(operation_->GetReceivedMessages(test_devices_[0]).empty());
 }
 
-TEST_F(MessageTransferOperationTest,
-       CannotCompleteGattConnection_RetryLimitReached) {
-  ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
-  InitializeOperation();
-  EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
-      test_devices_[0].GetDeviceId()));
-
-  fake_ble_connection_manager_->SimulateGattErrorConnectionAttempts(
-      test_devices_[0].GetDeviceId(),
-      MessageTransferOperation::kMaxGattConnectionAttemptsPerDevice);
-  EXPECT_FALSE(fake_ble_connection_manager_->IsRegistered(
-      test_devices_[0].GetDeviceId()));
-
-  VerifyOperationStartedAndFinished(true /* has_started */,
-                                    true /* has_finished */);
-  EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[0]));
-  EXPECT_TRUE(operation_->GetReceivedMessages(test_devices_[0]).empty());
-}
-
-TEST_F(MessageTransferOperationTest, MixedConnectionAttemptFailures) {
-  ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
-  InitializeOperation();
-  EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
-      test_devices_[0].GetDeviceId()));
-
-  // Fail to establish a connection one fewer time than the maximum allowed. The
-  // device should still be registered since the maximum was not hit.
-  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
-      test_devices_[0].GetDeviceId(),
-      MessageTransferOperation::kMaxEmptyScansPerDevice - 1);
-  EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
-      test_devices_[0].GetDeviceId()));
-
-  // Now
-  fake_ble_connection_manager_->SimulateGattErrorConnectionAttempts(
-      test_devices_[0].GetDeviceId(),
-      MessageTransferOperation::kMaxGattConnectionAttemptsPerDevice);
-  EXPECT_FALSE(fake_ble_connection_manager_->IsRegistered(
-      test_devices_[0].GetDeviceId()));
-
-  VerifyOperationStartedAndFinished(true /* has_started */,
-                                    true /* has_finished */);
-  EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[0]));
-  EXPECT_TRUE(operation_->GetReceivedMessages(test_devices_[0]).empty());
-}
-
 TEST_F(MessageTransferOperationTest, TestFailsThenConnects) {
   ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
   InitializeOperation();
@@ -424,22 +377,22 @@ TEST_F(MessageTransferOperationTest, TestDevicesUnregisteredAfterDeletion) {
 
 TEST_F(MessageTransferOperationTest,
        TestSuccessfulConnectionAndReceiveMessage_TimeoutSeconds) {
-  const uint32_t kTimeoutSeconds = 90;
+  const uint32_t timeout_seconds = 90;
 
   ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
   InitializeOperation();
   EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
       test_devices_[0].GetDeviceId()));
 
-  operation_->set_timeout_seconds(kTimeoutSeconds);
+  operation_->set_timeout_seconds(timeout_seconds);
 
   TransitionDeviceStatusFromDisconnectedToAuthenticated(test_devices_[0]);
   EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
       test_devices_[0].GetDeviceId()));
   EXPECT_TRUE(operation_->HasDeviceAuthenticated(test_devices_[0]));
-  VerifyTimerCreatedForDevice(test_devices_[0], kTimeoutSeconds);
+  VerifyTimerCreatedForDevice(test_devices_[0], timeout_seconds);
 
-  EXPECT_EQ(base::TimeDelta::FromSeconds(kTimeoutSeconds),
+  EXPECT_EQ(base::TimeDelta::FromSeconds(timeout_seconds),
             GetTimerForDevice(test_devices_[0])->GetCurrentDelay());
 
   fake_ble_connection_manager_->ReceiveMessage(
@@ -612,9 +565,8 @@ TEST_F(MessageTransferOperationTest, MultipleDevices) {
   // Fail 3 times to connect to |test_devices_[1]|.
   test_timer_factory_->set_device_id_for_next_timer(
       test_devices_[1].GetDeviceId());
-  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
-      test_devices_[1].GetDeviceId(),
-      MessageTransferOperation::kMaxEmptyScansPerDevice);
+  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
+      test_devices_[1].GetDeviceId(), kMaxConnectionAttemptsPerDevice);
   EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[1]));
   EXPECT_FALSE(fake_ble_connection_manager_->IsRegistered(
       test_devices_[1].GetDeviceId()));
@@ -632,9 +584,8 @@ TEST_F(MessageTransferOperationTest, MultipleDevices) {
   // Fail 3 times to connect to |test_devices_[3]|.
   test_timer_factory_->set_device_id_for_next_timer(
       test_devices_[3].GetDeviceId());
-  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
-      test_devices_[3].GetDeviceId(),
-      MessageTransferOperation::kMaxEmptyScansPerDevice);
+  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
+      test_devices_[3].GetDeviceId(), kMaxConnectionAttemptsPerDevice);
   EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[3]));
   EXPECT_FALSE(fake_ble_connection_manager_->IsRegistered(
       test_devices_[3].GetDeviceId()));