Make SignalSenderVerificationTest more robust

dbus/signal_sender_verification_unittest.cc

Index: dbus/signal_sender_verification_unittest.cc
diff --git a/dbus/signal_sender_verification_unittest.cc b/dbus/signal_sender_verification_unittest.cc
index 91db93fb421157083e004739baeab124979a0ec1..97a57aa75f3b85b049123223ff268c2a39c57314 100644
--- a/dbus/signal_sender_verification_unittest.cc
+++ b/dbus/signal_sender_verification_unittest.cc
@@ -42,6 +42,7 @@ class SignalSenderVerificationTest : public testing::Test {
     ASSERT_TRUE(test_service_->StartService());
     ASSERT_TRUE(test_service_->WaitUntilServiceIsStarted());
     ASSERT_TRUE(test_service_->HasDBusThread());
+    ASSERT_TRUE(test_service_->has_ownership());
 
     // Same setup for the second TestService. This service should not have the
     // ownership of the name at this point.
@@ -49,6 +50,7 @@ class SignalSenderVerificationTest : public testing::Test {
     ASSERT_TRUE(test_service2_->StartService());
     ASSERT_TRUE(test_service2_->WaitUntilServiceIsStarted());
     ASSERT_TRUE(test_service2_->HasDBusThread());
+    ASSERT_FALSE(test_service2_->has_ownership());
 
     // Create the client, using the D-Bus thread.
     dbus::Bus::Options bus_options;
@@ -62,6 +64,10 @@ class SignalSenderVerificationTest : public testing::Test {
         dbus::ObjectPath("/org/chromium/TestObject"));
     ASSERT_TRUE(bus_->HasDBusThread());
 
+    object_proxy_->SetNameOwnerChangedCallback(
+        base::Bind(&SignalSenderVerificationTest::OnNameOwnerChanged,
+                   base::Unretained(this)));
+
     // Connect to the "Test" signal of "org.chromium.TestInterface" from
     // the remote object.
     object_proxy_->ConnectToSignal(
@@ -75,6 +81,23 @@ class SignalSenderVerificationTest : public testing::Test {
     message_loop_.Run();
   }
 
+  void OnOwnership(bool expected, bool success) {
+    ASSERT_EQ(expected, success);
+  }
+
+  void OnNameOwnerChanged(dbus::Signal* signal) {
+    dbus::MessageReader reader(signal);
+    std::string name, old_owner, new_owner;
+    ASSERT_TRUE(reader.PopString(&name));
+    ASSERT_TRUE(reader.PopString(&old_owner));
+    ASSERT_TRUE(reader.PopString(&new_owner));
+    latest_name_owner_ = new_owner;
+    message_loop_.PostTask(
+        FROM_HERE,
+        base::Bind(&SignalSenderVerificationTest::QuitMessageLoop,
+                   base::Unretained(this)));

[satorux1, 2012/11/12 07:56:43]

Cannot we call QuitMessageLoop() directly here? Otherwise, please add some
comment about why we need to do this through the message loop

[Haruki Sato, 2012/11/13 06:23:28]

Done.

+  }
+
   virtual void TearDown() {
     bus_->ShutdownOnDBusThreadAndBlock();
 
@@ -115,6 +138,10 @@ class SignalSenderVerificationTest : public testing::Test {
     message_loop_.Run();
   }
 
+  void QuitMessageLoop() {
+    message_loop_.Quit();
+  }
+
   MessageLoop message_loop_;
   scoped_ptr<base::Thread> dbus_thread_;
   scoped_refptr<dbus::Bus> bus_;
@@ -123,6 +150,9 @@ class SignalSenderVerificationTest : public testing::Test {
   scoped_ptr<dbus::TestService> test_service2_;
   // Text message from "Test" signal.
   std::string test_signal_string_;
+
+  // The known latest name owner of TestService. Updated in OnNameOwnerChanged.
+  std::string latest_name_owner_;
 };
 
 TEST_F(SignalSenderVerificationTest, TestSignalAccepted) {
@@ -157,7 +187,7 @@ TEST_F(SignalSenderVerificationTest, TestSignalRejected) {
   EXPECT_EQ(samples1->TotalCount() + 1, samples2->TotalCount());
 }
 
-TEST_F(SignalSenderVerificationTest, DISABLED_TestOwnerChanged) {
+TEST_F(SignalSenderVerificationTest, TestOwnerChanged) {
   const char kMessage[] = "hello, world";
 
   // Send the test signal from the exported object.
@@ -168,8 +198,18 @@ TEST_F(SignalSenderVerificationTest, DISABLED_TestOwnerChanged) {
   ASSERT_EQ(kMessage, test_signal_string_);
 
   // Release and aquire the name ownership.
+  latest_name_owner_ = "dummyowner";
   test_service_->ShutdownAndBlock();
-  test_service2_->RequestOwnership();
+  // OnNameOwnerChanged will PostTask to quit the message loop.
+  message_loop_.Run();
+  ASSERT_TRUE(latest_name_owner_.empty());

[satorux1, 2012/11/12 07:56:43]

Add a comment like:

// latest_name_owner_ becomes empty as the owner is gone.

[Haruki Sato, 2012/11/13 06:23:28]

Done.

+
+  test_service2_->RequestOwnership(
+      base::Bind(&SignalSenderVerificationTest::OnOwnership,
+                 base::Unretained(this), true));
+  // OnNameOwnerChanged will PostTask to quit the message loop.
+  message_loop_.Run();
+  ASSERT_FALSE(latest_name_owner_.empty());

[satorux1, 2012/11/12 07:56:43]

// latest_name_owner_ becomes non empty as the new owner appears.

Besides, I think we should keep:

ASSERT_TRUE(test_service2_->has_ownership());

[Haruki Sato, 2012/11/13 06:23:28]

Added comment.

 
   // Now the second service owns the name.
   const char kNewMessage[] = "hello, new world";