[Media Router] Use DialMediaSinkService in MediaRouterMojoImpl

chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc

 // Copyright 2017 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 "chrome/browser/media/router/discovery/dial/dial_media_sink_service.h"
+#include "base/run_loop.h"
 #include "base/test/mock_callback.h"
+#include "base/timer/mock_timer.h"
 #include "chrome/browser/media/router/discovery/dial/dial_device_data.h"
 #include "chrome/browser/media/router/discovery/dial/dial_registry.h"
 #include "chrome/browser/media/router/test_helper.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
 using ::testing::Return;
@@ skipping 32 matching lines @@
 
 namespace media_router {
 
 class TestDialRegistry : public DialRegistry {
  public:
   TestDialRegistry() {}
   ~TestDialRegistry() {}
 
   MOCK_METHOD1(RegisterObserver, void(DialRegistry::Observer* observer));
   MOCK_METHOD1(UnregisterObserver, void(DialRegistry::Observer* observer));
+
+  MOCK_METHOD0(StartPeriodicDiscovery, void());
+  MOCK_METHOD0(StopPeriodicDiscovery, void());
 };
 
 class MockDeviceDescriptionService : public DeviceDescriptionService {
  public:
   MockDeviceDescriptionService(DeviceDescriptionParseSuccessCallback success_cb,
                                DeviceDescriptionParseErrorCallback error_cb)
       : DeviceDescriptionService(success_cb, error_cb) {}
   ~MockDeviceDescriptionService() override {}
 
   MOCK_METHOD2(GetDeviceDescriptions,
                void(const std::vector<DialDeviceData>& devices,
                     net::URLRequestContextGetter* request_context));
 };
 
-class TestDialMediaSinkService : public DialMediaSinkService {
- public:
-  TestDialMediaSinkService(
-      const MediaSinkService::OnSinksDiscoveredCallback& callback,
-      net::URLRequestContextGetter* request_context,
-      TestDialRegistry* test_dial_registry,
-      MockDeviceDescriptionService* mock_description_service)
-      : DialMediaSinkService(callback, request_context),
-        test_dial_registry_(test_dial_registry),
-        mock_description_service_(mock_description_service) {}
-
-  DialRegistry* dial_registry() override { return test_dial_registry_; }
-
-  DeviceDescriptionService* GetDescriptionService() override {
-    return mock_description_service_;
-  }
-
- private:
-  TestDialRegistry* test_dial_registry_;
-  MockDeviceDescriptionService* mock_description_service_;
-};
-
 class DialMediaSinkServiceTest : public ::testing::Test {
  public:
   DialMediaSinkServiceTest()
-      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
-        mock_description_service_(mock_success_cb_.Get(), mock_error_cb_.Get()),
-        media_sink_service_(mock_sink_discovered_cb_.Get(),
-                            profile_.GetRequestContext(),
-                            &test_dial_registry_,
-                            &mock_description_service_) {}
-
-  DialMediaSinkService* media_sink_service() { return &media_sink_service_; }
+      : media_sink_service_(
+            new DialMediaSinkService(mock_sink_discovered_cb_.Get(),
+                                     profile_.GetRequestContext())) {
+    media_sink_service_->SetDialRegistryForTest(&test_dial_registry_);
+    mock_description_service_ = new MockDeviceDescriptionService(

[Kevin M, 2017/05/08 18:22:19]

Best practice point: try to bind pointers to unique_ptrs at allocation time.
Otherwise there is a window of time in which the pointer is unowned, which could
lead to regressions in the future if someone refactored the code (e.g. adding an
exception handler.)

So:

auto mock_description_service =
base::MakeUniuqe<MockDeviceDescriptionService>(...);
mock_description_service_ = mock_description_service.get();
mss->SetForTest(std::move(mock_description_service);

[zhaobin, 2017/05/08 20:13:47]

Done.

+        mock_success_cb_.Get(), mock_error_cb_.Get());
+    media_sink_service_->SetDescriptionServiceForTest(
+        base::WrapUnique(mock_description_service_));
+    mock_timer_ =
+        new base::MockTimer(true /*retain_user_task*/, false /*is_repeating*/);

[Kevin M, 2017/05/08 18:22:19]

Suggestion: remove inline comments. A 2-bool function signature ought to be
simple enough to not need extra annotations.

[mark a. foltz, 2017/05/08 18:29:02]

Please keep them.  Unlike a simple setter, MockTimer() gives no clue as to what
its arguments mean.  This is when inline comments are most helpful.

It usually means the designer of the class should have used static factories
with descriptive names or a builder pattern instead of
lists-of-boolean-initializers.

[zhaobin, 2017/05/08 20:13:48]

Acknowledged.

+    media_sink_service_->SetTimerForTest(base::WrapUnique(mock_timer_));
+  }
 
   void TestFetchCompleted(const std::vector<MediaSinkInternal>& old_sinks,
                           const std::vector<MediaSinkInternal>& new_sinks) {
-    media_sink_service()->mrp_sinks_ =
+    media_sink_service_->mrp_sinks_ =
         std::set<MediaSinkInternal>(old_sinks.begin(), old_sinks.end());
-    media_sink_service()->current_sinks_ =
+    media_sink_service_->current_sinks_ =
         std::set<MediaSinkInternal>(new_sinks.begin(), new_sinks.end());
     EXPECT_CALL(mock_sink_discovered_cb_, Run(new_sinks));
-    media_sink_service()->OnFetchCompleted();
+    media_sink_service_->OnFetchCompleted();
+
+    base::RunLoop().RunUntilIdle();
   }
 
  protected:
   const content::TestBrowserThreadBundle thread_bundle_;
   TestingProfile profile_;
 
   base::MockCallback<MediaSinkService::OnSinksDiscoveredCallback>
       mock_sink_discovered_cb_;
   base::MockCallback<
       MockDeviceDescriptionService::DeviceDescriptionParseSuccessCallback>
       mock_success_cb_;
   base::MockCallback<
       MockDeviceDescriptionService::DeviceDescriptionParseErrorCallback>
       mock_error_cb_;
 
   TestDialRegistry test_dial_registry_;
-  MockDeviceDescriptionService mock_description_service_;
+  MockDeviceDescriptionService* mock_description_service_;
+  base::MockTimer* mock_timer_;
 
-  TestDialMediaSinkService media_sink_service_;
+  scoped_refptr<DialMediaSinkService> media_sink_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(DialMediaSinkServiceTest);
 };
 
 TEST_F(DialMediaSinkServiceTest, TestStart) {
-  EXPECT_CALL(test_dial_registry_, RegisterObserver(&media_sink_service_));
-  media_sink_service()->Start();
+  EXPECT_CALL(test_dial_registry_, RegisterObserver(media_sink_service_.get()));
+  EXPECT_CALL(test_dial_registry_, StartPeriodicDiscovery());
+  media_sink_service_->Start();
 
   DialRegistry::DeviceList deviceList;
   DialDeviceData first_device("first", GURL("http://127.0.0.1/dd.xml"),
                               base::Time::Now());
   DialDeviceData second_device("second", GURL("http://127.0.0.2/dd.xml"),
                                base::Time::Now());
   DialDeviceData third_device("third", GURL("http://127.0.0.3/dd.xml"),
                               base::Time::Now());
   deviceList.push_back(first_device);
   deviceList.push_back(second_device);
   deviceList.push_back(third_device);
 
-  EXPECT_CALL(mock_description_service_, GetDeviceDescriptions(deviceList, _));
+  EXPECT_CALL(*mock_description_service_, GetDeviceDescriptions(deviceList, _));
 
-  media_sink_service()->OnDialDeviceEvent(deviceList);
-  EXPECT_TRUE(media_sink_service_.finish_timer_->IsRunning());
+  media_sink_service_->OnDialDeviceEvent(deviceList);
+  EXPECT_TRUE(media_sink_service_->finish_timer_->IsRunning());
+
+  base::RunLoop().RunUntilIdle();
 }
 
 TEST_F(DialMediaSinkServiceTest, TestOnDeviceDescriptionAvailable) {
   DialDeviceData device_data("first", GURL("http://127.0.0.1/dd.xml"),
                              base::Time::Now());
   ParsedDialDeviceDescription device_description;
   device_description.model_name = "model name";
   device_description.friendly_name = "friendly name";
   device_description.app_url = GURL("http://192.168.1.1/apps");
   device_description.unique_id = "unique id";
 
-  media_sink_service()->OnDeviceDescriptionAvailable(device_data,
-                                                     device_description);
-  EXPECT_TRUE(media_sink_service()->current_sinks_.empty());
+  media_sink_service_->OnDeviceDescriptionAvailable(device_data,
+                                                    device_description);
+  EXPECT_TRUE(media_sink_service_->current_sinks_.empty());
 
   std::vector<DialDeviceData> deviceList{device_data};
-  EXPECT_CALL(mock_description_service_, GetDeviceDescriptions(deviceList, _));
+  EXPECT_CALL(*mock_description_service_, GetDeviceDescriptions(deviceList, _));
 
-  media_sink_service()->OnDialDeviceEvent(deviceList);
-  media_sink_service()->OnDeviceDescriptionAvailable(device_data,
-                                                     device_description);
+  media_sink_service_->OnDialDeviceEvent(deviceList);
+  media_sink_service_->OnDeviceDescriptionAvailable(device_data,
+                                                    device_description);
 
-  EXPECT_EQ(size_t(1), media_sink_service()->current_sinks_.size());
+  EXPECT_EQ(size_t(1), media_sink_service_->current_sinks_.size());
 }
 
-TEST_F(DialMediaSinkServiceTest, TestFetchCompleted) {
+TEST_F(DialMediaSinkServiceTest, TestTimer) {
+  DialDeviceData device_data("first", GURL("http://127.0.0.1/dd.xml"),
+                             base::Time::Now());
+  ParsedDialDeviceDescription device_description;
+  device_description.model_name = "model name";
+  device_description.friendly_name = "friendly name";
+  device_description.app_url = GURL("http://192.168.1.1/apps");
+  device_description.unique_id = "unique id";
+
+  std::vector<DialDeviceData> deviceList{device_data};
+  EXPECT_CALL(*mock_description_service_, GetDeviceDescriptions(deviceList, _));
+
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  media_sink_service_->OnDialDeviceEvent(deviceList);
+  media_sink_service_->OnDeviceDescriptionAvailable(device_data,
+                                                    device_description);
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
+  EXPECT_CALL(mock_sink_discovered_cb_, Run(_));
+  mock_timer_->Fire();
+
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  device_description.app_url = GURL("http://192.168.1.11/apps");
+  media_sink_service_->OnDeviceDescriptionAvailable(device_data,
+                                                    device_description);
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(DialMediaSinkServiceTest, TestFetchCompleted_SameSink) {
   std::vector<MediaSinkInternal> old_sinks;
   std::vector<MediaSinkInternal> new_sinks = CreateDialMediaSinks();
   TestFetchCompleted(old_sinks, new_sinks);
 
   // Same sink
   EXPECT_CALL(mock_sink_discovered_cb_, Run(new_sinks)).Times(0);
-  media_sink_service()->OnFetchCompleted();
+  media_sink_service_->OnFetchCompleted();
+
+  base::RunLoop().RunUntilIdle();
 }
 
 TEST_F(DialMediaSinkServiceTest, TestFetchCompleted_OneNewSink) {
   std::vector<MediaSinkInternal> old_sinks = CreateDialMediaSinks();
   std::vector<MediaSinkInternal> new_sinks = CreateDialMediaSinks();
   MediaSink sink3("sink3", "sink_name_3", MediaSink::IconType::CAST);
   DialSinkExtraData extra_data3 = CreateDialSinkExtraData(
       "model_name3", "192.168.1.3", "https://example3.com");
   new_sinks.push_back(MediaSinkInternal(sink3, extra_data3));
   TestFetchCompleted(old_sinks, new_sinks);
@@ skipping 25 matching lines @@
       "model_name3", "192.168.1.3", "https://example3.com");
 
   std::vector<MediaSinkInternal> new_sinks;
   new_sinks.push_back(MediaSinkInternal(sink1, extra_data2));
   new_sinks.push_back(MediaSinkInternal(sink3, extra_data3));
 
   TestFetchCompleted(old_sinks, new_sinks);
 }
 
 }  // namespace media_router