Instant Tethering: Rewrite DisconnectTetheringOperation test.

This change also factors out TestTimerFactory to reuse code.

R=khorimoto@chromium.org

Bug: none
Test: DisconnectTetheringOperationTest.*
Change-Id: I5988d6bfdbf145511d9ea5c7451c31b9d929b753
Reviewed-on: https://chromium-review.googlesource.com/c/1357614
Commit-Queue: James Hawkins <jhawkins@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613187}

chromeos/components/tether/BUILD.gn

@@ -209,6 +209,8 @@ static_library("test_support") {
     "mock_tether_host_response_recorder.h",
     "proto_test_util.cc",
     "proto_test_util.h",
+    "test_timer_factory.cc",
+    "test_timer_factory.h",
   ]
 
   public_deps = [
@@ -217,6 +219,7 @@ static_library("test_support") {
 
   deps = [
     "//base",
+    "//base/test:test_support",
     "//chromeos",
     "//chromeos/components/multidevice",
     "//chromeos/components/multidevice:test_support",

chromeos/components/tether/connect_tethering_operation_unittest.cc

@@ -16,12 +16,11 @@
 #include "base/time/time.h"
 #include "base/timer/mock_timer.h"
 #include "chromeos/components/multidevice/remote_device_test_util.h"
-#include "chromeos/components/tether/fake_ble_connection_manager.h"
 #include "chromeos/components/tether/message_wrapper.h"
 #include "chromeos/components/tether/mock_tether_host_response_recorder.h"
 #include "chromeos/components/tether/proto/tether.pb.h"
 #include "chromeos/components/tether/proto_test_util.h"
-#include "chromeos/components/tether/timer_factory.h"
+#include "chromeos/components/tether/test_timer_factory.h"
 #include "chromeos/services/device_sync/public/cpp/fake_device_sync_client.h"
 #include "chromeos/services/secure_channel/ble_constants.h"
 #include "chromeos/services/secure_channel/public/cpp/client/fake_client_channel.h"
@@ -65,28 +64,6 @@ class MockOperationObserver : public ConnectTetheringOperation::Observer {
 
 }  // namespace
 
-class TestTimerFactory : public TimerFactory {
- public:
-  ~TestTimerFactory() override = default;
-
-  // TimerFactory:
-  std::unique_ptr<base::OneShotTimer> CreateOneShotTimer() override {
-    EXPECT_FALSE(device_id_for_next_timer_.empty());
-    base::MockOneShotTimer* mock_timer = new base::MockOneShotTimer();
-    device_id_to_timer_map_[device_id_for_next_timer_] = mock_timer;
-    return base::WrapUnique(mock_timer);
-  }
-
-  void set_device_id_for_next_timer(
-      const std::string& device_id_for_next_timer) {
-    device_id_for_next_timer_ = device_id_for_next_timer;
-  }
-
- private:
-  std::string device_id_for_next_timer_;
-  base::flat_map<std::string, base::MockOneShotTimer*> device_id_to_timer_map_;
-};
-
 class ConnectTetheringOperationTest : public testing::Test {
  protected:
   ConnectTetheringOperationTest()

chromeos/components/tether/disconnect_tethering_operation.h

@@ -74,6 +74,10 @@ class DisconnectTetheringOperation : public MessageTransferOperation {
 
  private:
   friend class DisconnectTetheringOperationTest;
+  FRIEND_TEST_ALL_PREFIXES(DisconnectTetheringOperationTest, TestSuccess);
+  FRIEND_TEST_ALL_PREFIXES(DisconnectTetheringOperationTest, TestFailure);
+  FRIEND_TEST_ALL_PREFIXES(DisconnectTetheringOperationTest,
+                           DisconnectRequestSentOnceAuthenticated);
 
   void SetClockForTest(base::Clock* clock_for_test);
 

chromeos/components/tether/disconnect_tethering_operation_unittest.cc

@@ -7,16 +7,21 @@
 #include <memory>
 #include <vector>
 
+#include "base/containers/flat_map.h"
 #include "base/memory/ptr_util.h"
 #include "base/test/metrics/histogram_tester.h"
-#include "base/test/scoped_task_environment.h"
 #include "base/test/simple_test_clock.h"
+#include "base/time/time.h"
+#include "base/timer/mock_timer.h"
 #include "chromeos/components/multidevice/remote_device_test_util.h"
-#include "chromeos/components/tether/fake_ble_connection_manager.h"
 #include "chromeos/components/tether/message_wrapper.h"
 #include "chromeos/components/tether/proto/tether.pb.h"
+#include "chromeos/components/tether/test_timer_factory.h"
 #include "chromeos/services/device_sync/public/cpp/fake_device_sync_client.h"
+#include "chromeos/services/secure_channel/public/cpp/client/fake_client_channel.h"
+#include "chromeos/services/secure_channel/public/cpp/client/fake_connection_attempt.h"
 #include "chromeos/services/secure_channel/public/cpp/client/fake_secure_channel_client.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
@@ -28,126 +33,151 @@ namespace {
 constexpr base::TimeDelta kDisconnectTetheringRequestTime =
     base::TimeDelta::FromSeconds(3);
 
-class TestObserver final : public DisconnectTetheringOperation::Observer {
+// Used to verify the DisonnectTetheringOperation notifies the observer when
+// appropriate.
+class MockOperationObserver : public DisconnectTetheringOperation::Observer {
  public:
-  TestObserver() : success_(false) {}
+  MockOperationObserver() = default;
+  ~MockOperationObserver() = default;
 
-  virtual ~TestObserver() = default;
-
-  std::string last_device_id() { return last_device_id_; }
-
-  bool WasLastOperationSuccessful() {
-    EXPECT_TRUE(!last_device_id_.empty());
-    return success_;
-  }
-
-  // DisconnectTetheringOperation::Observer:
-  void OnOperationFinished(const std::string& device_id,
-                           bool success) override {
-    last_device_id_ = device_id;
-    success_ = success;
-  }
+  MOCK_METHOD2(OnOperationFinished, void(const std::string&, bool));
 
  private:
-  std::string last_device_id_;
-  bool success_;
+  DISALLOW_COPY_AND_ASSIGN(MockOperationObserver);
 };
 
-std::string CreateDisconnectTetheringString() {
-  DisconnectTetheringRequest request;
-  return MessageWrapper(request).ToRawMessage();
-}
-
 }  // namespace
 
 class DisconnectTetheringOperationTest : public testing::Test {
  protected:
   DisconnectTetheringOperationTest()
-      : disconnect_tethering_request_string_(CreateDisconnectTetheringString()),
-        test_device_(multidevice::CreateRemoteDeviceRefListForTest(1)[0]) {}
+      : local_device_(multidevice::RemoteDeviceRefBuilder()
+                          .SetPublicKey("local device")
+                          .Build()),
+        remote_device_(multidevice::CreateRemoteDeviceRefListForTest(1)[0]) {}
 
   void SetUp() override {
     fake_device_sync_client_ =
         std::make_unique<device_sync::FakeDeviceSyncClient>();
+    fake_device_sync_client_->set_local_device_metadata(local_device_);
     fake_secure_channel_client_ =
         std::make_unique<secure_channel::FakeSecureChannelClient>();
-    fake_ble_connection_manager_ = std::make_unique<FakeBleConnectionManager>();
-
-    operation_ = base::WrapUnique(new DisconnectTetheringOperation(
-        test_device_, fake_device_sync_client_.get(),
-        fake_secure_channel_client_.get()));
-
-    test_observer_ = base::WrapUnique(new TestObserver());
-    operation_->AddObserver(test_observer_.get());
-
-    test_clock_.SetNow(base::Time::UnixEpoch());
-    operation_->SetClockForTest(&test_clock_);
 
+    operation_ = ConstructOperation();
     operation_->Initialize();
-  }
 
-  void SimulateDeviceAuthenticationAndVerifyMessageSent() {
-    operation_->OnDeviceAuthenticated(test_device_);
+    ConnectAuthenticatedChannelForDevice(remote_device_);
+  }
 
-    // Verify that the message was passed to |fake_ble_connection_manager_|
-    // correctly.
-    std::vector<FakeBleConnectionManager::SentMessage>& sent_messages =
-        fake_ble_connection_manager_->sent_messages();
-    ASSERT_EQ(1u, sent_messages.size());
-    EXPECT_EQ(test_device_.GetDeviceId(), sent_messages[0].device_id);
-    EXPECT_EQ(disconnect_tethering_request_string_, sent_messages[0].message);
+  std::unique_ptr<DisconnectTetheringOperation> ConstructOperation() {
+    auto connection_attempt =
+        std::make_unique<secure_channel::FakeConnectionAttempt>();
+    connection_attempt_ = connection_attempt.get();
+    // remote_device_to_fake_connection_attempt_map_[remote_device_] =
+    //     fake_connection_attempt.get();
+    fake_secure_channel_client_->set_next_listen_connection_attempt(
+        remote_device_, local_device_, std::move(connection_attempt));
+
+    auto operation = base::WrapUnique(new DisconnectTetheringOperation(
+        remote_device_, fake_device_sync_client_.get(),
+        fake_secure_channel_client_.get()));
+    operation->AddObserver(&mock_observer_);
 
-    test_clock_.Advance(kDisconnectTetheringRequestTime);
+    // Prepare the disconnection timeout timer to be made for the remote device.
+    auto test_timer_factory = std::make_unique<TestTimerFactory>();
+    test_timer_factory->set_device_id_for_next_timer(
+        remote_device_.GetDeviceId());
+    operation->SetTimerFactoryForTest(std::move(test_timer_factory));
 
-    // Now, simulate the message being sent.
-    int last_sequence_number =
-        fake_ble_connection_manager_->last_sequence_number();
-    EXPECT_NE(last_sequence_number, -1);
-    fake_ble_connection_manager_->SetMessageSent(last_sequence_number);
+    test_clock_.SetNow(base::Time::UnixEpoch());
+    operation->SetClockForTest(&test_clock_);
 
-    histogram_tester_.ExpectTimeBucketCount(
-        "InstantTethering.Performance.DisconnectTetheringRequestDuration",
-        kDisconnectTetheringRequestTime, 1);
+    return operation;
   }
 
-  void SimulateConnectionTimeout() {
-    operation_->UnregisterDevice(test_device_);
+  void ConnectAuthenticatedChannelForDevice(
+      multidevice::RemoteDeviceRef remote_device) {
+    auto fake_client_channel =
+        std::make_unique<secure_channel::FakeClientChannel>();
+    connection_attempt_->NotifyConnection(std::move(fake_client_channel));
   }
 
-  const std::string disconnect_tethering_request_string_;
-  const multidevice::RemoteDeviceRef test_device_;
+  const multidevice::RemoteDeviceRef local_device_;
+  const multidevice::RemoteDeviceRef remote_device_;
 
+  secure_channel::FakeConnectionAttempt* connection_attempt_;
   std::unique_ptr<device_sync::FakeDeviceSyncClient> fake_device_sync_client_;
-  std::unique_ptr<secure_channel::SecureChannelClient>
+  std::unique_ptr<secure_channel::FakeSecureChannelClient>
       fake_secure_channel_client_;
-  std::unique_ptr<FakeBleConnectionManager> fake_ble_connection_manager_;
-  std::unique_ptr<TestObserver> test_observer_;
 
   base::SimpleTestClock test_clock_;
+  MockOperationObserver mock_observer_;
+  base::HistogramTester histogram_tester_;
 
   std::unique_ptr<DisconnectTetheringOperation> operation_;
 
-  base::HistogramTester histogram_tester_;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(DisconnectTetheringOperationTest);
 };
 
-TEST_F(DisconnectTetheringOperationTest, DISABLED_TestSuccess) {
-  SimulateDeviceAuthenticationAndVerifyMessageSent();
-  EXPECT_EQ(test_device_.GetDeviceId(), test_observer_->last_device_id());
-  EXPECT_TRUE(test_observer_->WasLastOperationSuccessful());
+TEST_F(DisconnectTetheringOperationTest, TestSuccess) {
+  // Verify that the Observer is called with success and the correct parameters.
+  EXPECT_CALL(mock_observer_, OnOperationFinished(remote_device_.GetDeviceId(),
+                                                  true /* successful */));
+
+  // Advance the clock in order to verify a non-zero request duration is
+  // recorded and verified (below).
+  test_clock_.Advance(kDisconnectTetheringRequestTime);
+
+  // Execute the operation.
+  operation_->OnMessageSent(0 /* sequence_number */);
+
+  // Verify the request duration metric is recorded.
+  histogram_tester_.ExpectTimeBucketCount(
+      "InstantTethering.Performance.DisconnectTetheringRequestDuration",
+      kDisconnectTetheringRequestTime, 1);
 }
 
-TEST_F(DisconnectTetheringOperationTest, DISABLED_TestFailure) {
-  SimulateConnectionTimeout();
-  EXPECT_EQ(test_device_.GetDeviceId(), test_observer_->last_device_id());
-  EXPECT_FALSE(test_observer_->WasLastOperationSuccessful());
+TEST_F(DisconnectTetheringOperationTest, TestFailure) {
+  // Verify that the observer is called with failure and the correct parameters.
+  EXPECT_CALL(mock_observer_, OnOperationFinished(remote_device_.GetDeviceId(),
+                                                  false /* successful */));
+
+  // Finalize the operation; no message has been sent so this represents a
+  // failure case.
+  operation_->UnregisterDevice(remote_device_);
 
   histogram_tester_.ExpectTotalCount(
       "InstantTethering.Performance.DisconnectTetheringRequestDuration", 0);
 }
 
+// Tests that the DisonnectTetheringRequest message is sent to the remote device
+// once the communication channel is connected and authenticated.
+TEST_F(DisconnectTetheringOperationTest,
+       DisconnectRequestSentOnceAuthenticated) {
+  std::unique_ptr<DisconnectTetheringOperation> operation =
+      ConstructOperation();
+  operation->Initialize();
+
+  // Create the client channel for the remote device.
+  auto fake_client_channel =
+      std::make_unique<secure_channel::FakeClientChannel>();
+
+  // No requests as a result of creating the client channel.
+  auto& sent_messages = fake_client_channel->sent_messages();
+  EXPECT_EQ(0u, sent_messages.size());
+
+  // Connect and authenticate the client channel.
+  connection_attempt_->NotifyConnection(std::move(fake_client_channel));
+
+  // Verify the DisconnectTetheringRequest message is sent.
+  auto message_wrapper =
+      std::make_unique<MessageWrapper>(DisconnectTetheringRequest());
+  std::string expected_payload = message_wrapper->ToRawMessage();
+  EXPECT_EQ(1u, sent_messages.size());
+  EXPECT_EQ(expected_payload, sent_messages[0].first);
+}
+
 }  // namespace tether
 
 }  // namespace chromeos

chromeos/components/tether/message_transfer_operation_unittest.cc

@@ -11,7 +11,7 @@
 #include "chromeos/components/multidevice/remote_device_test_util.h"
 #include "chromeos/components/tether/message_wrapper.h"
 #include "chromeos/components/tether/proto_test_util.h"
-#include "chromeos/components/tether/timer_factory.h"
+#include "chromeos/components/tether/test_timer_factory.h"
 #include "chromeos/services/device_sync/public/cpp/fake_device_sync_client.h"
 #include "chromeos/services/secure_channel/public/cpp/client/fake_client_channel.h"
 #include "chromeos/services/secure_channel/public/cpp/client/fake_connection_attempt.h"
@@ -127,36 +127,6 @@ class TestOperation : public MessageTransferOperation {
   base::Optional<int> last_sequence_number_;
 };
 
-class TestTimerFactory : public TimerFactory {
- public:
-  ~TestTimerFactory() override = default;
-
-  // TimerFactory:
-  std::unique_ptr<base::OneShotTimer> CreateOneShotTimer() override {
-    EXPECT_FALSE(device_id_for_next_timer_.empty());
-    base::MockOneShotTimer* mock_timer = new base::MockOneShotTimer();
-    device_id_to_timer_map_[device_id_for_next_timer_] = mock_timer;
-    return base::WrapUnique(mock_timer);
-  }
-
-  base::MockOneShotTimer* GetTimerForDeviceId(const std::string& device_id) {
-    return device_id_to_timer_map_[device_id_for_next_timer_];
-  }
-
-  void ClearTimerForDeviceId(const std::string& device_id) {
-    device_id_to_timer_map_.erase(device_id_for_next_timer_);
-  }
-
-  void set_device_id_for_next_timer(
-      const std::string& device_id_for_next_timer) {
-    device_id_for_next_timer_ = device_id_for_next_timer;
-  }
-
- private:
-  std::string device_id_for_next_timer_;
-  base::flat_map<std::string, base::MockOneShotTimer*> device_id_to_timer_map_;
-};
-
 TetherAvailabilityResponse CreateTetherAvailabilityResponse() {
   TetherAvailabilityResponse response;
   response.set_response_code(

chromeos/components/tether/test_timer_factory.cc

+// Copyright 2018 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 "chromeos/components/tether/test_timer_factory.h"
+
+#include "base/logging.h"
+
+namespace chromeos {
+namespace tether {
+
+TestTimerFactory::TestTimerFactory() = default;
+TestTimerFactory::~TestTimerFactory() = default;
+
+std::unique_ptr<base::OneShotTimer> TestTimerFactory::CreateOneShotTimer() {
+  DCHECK(!device_id_for_next_timer_.empty());
+  auto mock_timer = std::make_unique<base::MockOneShotTimer>();
+  device_id_to_timer_map_[device_id_for_next_timer_] = mock_timer.get();
+  return std::move(mock_timer);
+}
+
+}  // namespace tether
+}  // namespace chromeos
\ No newline at end of file

chromeos/components/tether/test_timer_factory.h

+// Copyright 2018 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.
+
+#ifndef CHROMEOS_COMPONENTS_TETHER_TEST_TIMER_FACTORY_H_
+#define CHROMEOS_COMPONENTS_TETHER_TEST_TIMER_FACTORY_H_
+
+#include <memory>
+#include <string>
+
+#include "base/containers/flat_map.h"
+#include "base/timer/mock_timer.h"
+#include "base/timer/timer.h"
+#include "chromeos/components/tether/timer_factory.h"
+
+namespace chromeos {
+namespace tether {
+
+class TestTimerFactory : public TimerFactory {
+ public:
+  TestTimerFactory();
+  ~TestTimerFactory() override;
+
+  // TimerFactory:
+  std::unique_ptr<base::OneShotTimer> CreateOneShotTimer() override;
+
+  void set_device_id_for_next_timer(
+      const std::string& device_id_for_next_timer) {
+    device_id_for_next_timer_ = device_id_for_next_timer;
+  }
+
+  base::MockOneShotTimer* GetTimerForDeviceId(const std::string& device_id) {
+    return device_id_to_timer_map_[device_id_for_next_timer_];
+  }
+
+  void ClearTimerForDeviceId(const std::string& device_id) {
+    device_id_to_timer_map_.erase(device_id_for_next_timer_);
+  }
+
+ private:
+  std::string device_id_for_next_timer_;
+  base::flat_map<std::string, base::MockOneShotTimer*> device_id_to_timer_map_;
+};
+
+}  // namespace tether
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_TETHER_TEST_TIMER_FACTORY_H_
\ No newline at end of file