[Media Router] Use DialMediaSinkService in MediaRouterMojoImpl

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

Index: chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc
diff --git a/chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc b/chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc
index cf542a83254f980a0d8d90d34d6bd3d22e73241a..2897f236744d8d24c903ad7d9aaa019d9eff28d5 100644
--- a/chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc
+++ b/chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc
@@ -3,6 +3,7 @@
 // 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 "chrome/browser/media/router/discovery/dial/dial_device_data.h"
 #include "chrome/browser/media/router/discovery/dial/dial_registry.h"
@@ -56,6 +57,9 @@ class TestDialRegistry : public DialRegistry {
 
   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 {
@@ -88,6 +92,8 @@ class TestDialMediaSinkService : public DialMediaSinkService {
   }
 
  private:
+  ~TestDialMediaSinkService() override {}

[Kevin M, 2017/05/03 21:07:27]

Is this necessary? The object will be deleted from inside a scoped_refptr.

[zhaobin, 2017/05/04 23:52:36]

Code removed.

+
   TestDialRegistry* test_dial_registry_;
   MockDeviceDescriptionService* mock_description_service_;
 };
@@ -95,23 +101,27 @@ class TestDialMediaSinkService : public DialMediaSinkService {
 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_; }
+      : mock_description_service_(mock_success_cb_.Get(), mock_error_cb_.Get()),
+        media_sink_service_(
+            new TestDialMediaSinkService(mock_sink_discovered_cb_.Get(),
+                                         profile_.GetRequestContext(),
+                                         &test_dial_registry_,
+                                         &mock_description_service_)) {}
+
+  DialMediaSinkService* media_sink_service() {

[Kevin M, 2017/05/03 21:07:27]

Getter method isn't necessary? Can just use arrow operator on
media_sink_service_ directly

[zhaobin, 2017/05/04 23:52:36]

Done.

+    return media_sink_service_.get();
+  }
 
   void TestFetchCompleted(const std::vector<MediaSinkInternal>& old_sinks,
                           const std::vector<MediaSinkInternal>& new_sinks) {
+    base::RunLoop run_loop;

[Kevin M, 2017/05/03 21:07:27]

Nit..

*The runloop doesn't have to be in scope when tasks are posted.
*You can call RunUntilIdle on a RunLoop rvalue at the end of the function, e.g.

base::RunLoop().RunUntilIdle()


here and elsewhere

[zhaobin, 2017/05/04 23:52:36]

Done.

     media_sink_service()->mrp_sinks_ =
         std::set<MediaSinkInternal>(old_sinks.begin(), old_sinks.end());
     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();

[Kevin M, 2017/05/03 21:07:27]

Also, see my previous comment about Timer dependency injection. Working with the
timer is a higher fidelity test than spoofing calls to
media_sink_service::OnFetchCompleted().

[zhaobin, 2017/05/04 23:52:36]

This aims to test OnFetchCompleted() function only. Will create a new function
to test timer.

+    run_loop.RunUntilIdle();
   }
 
  protected:
@@ -130,11 +140,13 @@ class DialMediaSinkServiceTest : public ::testing::Test {
   TestDialRegistry test_dial_registry_;
   MockDeviceDescriptionService mock_description_service_;
 
-  TestDialMediaSinkService media_sink_service_;
+  scoped_refptr<TestDialMediaSinkService> media_sink_service_;

[Kevin M, 2017/05/03 21:07:27]

DISALLOW_COPY_AND_ASSIGN(DialMediaSinkServiceTest);

[zhaobin, 2017/05/04 23:52:36]

Done.

 };
 
 TEST_F(DialMediaSinkServiceTest, TestStart) {
-  EXPECT_CALL(test_dial_registry_, RegisterObserver(&media_sink_service_));
+  base::RunLoop run_loop;
+  EXPECT_CALL(test_dial_registry_, RegisterObserver(media_sink_service()));
+  EXPECT_CALL(test_dial_registry_, StartPeriodicDiscovery());
   media_sink_service()->Start();
 
   DialRegistry::DeviceList deviceList;
@@ -151,7 +163,8 @@ TEST_F(DialMediaSinkServiceTest, TestStart) {
   EXPECT_CALL(mock_description_service_, GetDeviceDescriptions(deviceList, _));
 
   media_sink_service()->OnDialDeviceEvent(deviceList);
-  EXPECT_TRUE(media_sink_service_.finish_timer_->IsRunning());
+  EXPECT_TRUE(media_sink_service()->finish_timer_->IsRunning());

[Kevin M, 2017/05/03 21:07:27]

No need to use a getter function; you can use the arrow operator on the field.

e.g."media_sink_service_->finish_timer_->IsRunning()"

[zhaobin, 2017/05/04 23:52:36]

Done.

+  run_loop.RunUntilIdle();
 }
 
 TEST_F(DialMediaSinkServiceTest, TestOnDeviceDescriptionAvailable) {
@@ -177,14 +190,16 @@ TEST_F(DialMediaSinkServiceTest, TestOnDeviceDescriptionAvailable) {
   EXPECT_EQ(size_t(1), media_sink_service()->current_sinks_.size());
 }
 
-TEST_F(DialMediaSinkServiceTest, TestFetchCompleted) {
+TEST_F(DialMediaSinkServiceTest, TestFetchCompleted_SameSink) {
   std::vector<MediaSinkInternal> old_sinks;
   std::vector<MediaSinkInternal> new_sinks = CreateDialMediaSinks();
   TestFetchCompleted(old_sinks, new_sinks);
 
   // Same sink
+  base::RunLoop run_loop;
   EXPECT_CALL(mock_sink_discovered_cb_, Run(new_sinks)).Times(0);
   media_sink_service()->OnFetchCompleted();
+  run_loop.RunUntilIdle();
 }
 
 TEST_F(DialMediaSinkServiceTest, TestFetchCompleted_OneNewSink) {