Reporting of policy errors via host-offline-reason: part 2b

remoting/host/heartbeat_sender_unittest.cc

Index: remoting/host/heartbeat_sender_unittest.cc
diff --git a/remoting/host/heartbeat_sender_unittest.cc b/remoting/host/heartbeat_sender_unittest.cc
index 80b1c339210d4dd14f205f2ef40db26ff40fe67c..a9f6083b0fbb3579f66a3b91a101cafe5362c0b9 100644
--- a/remoting/host/heartbeat_sender_unittest.cc
+++ b/remoting/host/heartbeat_sender_unittest.cc
@@ -7,6 +7,7 @@
 #include <set>
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"

[Lambros, 2014/11/18 02:33:53]

Do we need weak_ptr.h here? Isn't WeakPtr a hidden implementation detail of
MockCallback?

 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/run_loop.h"
@@ -14,6 +15,7 @@
 #include "remoting/base/constants.h"
 #include "remoting/base/rsa_key_pair.h"
 #include "remoting/base/test_rsa_key_pair.h"
+#include "remoting/host/mock_callback.h"
 #include "remoting/signaling/iq_sender.h"
 #include "remoting/signaling/mock_signal_strategy.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -42,17 +44,6 @@ const char kTestJid[] = "user@gmail.com/chromoting123";
 const char kStanzaId[] = "123";
 const int kTestInterval = 123;
 
-class MockListener : public HeartbeatSender::Listener {
- public:
-  // Overridden from HeartbeatSender::Listener
-  virtual void OnUnknownHostIdError() override {
-    NOTREACHED();
-  }
-
-  // Overridden from HeartbeatSender::Listener
-  MOCK_METHOD0(OnHeartbeatSuccessful, void());
-};
-
 }  // namespace
 
 ACTION_P(AddListener, list) {
@@ -78,9 +69,13 @@ class HeartbeatSenderTest
         .WillRepeatedly(RemoveListener(&signal_strategy_listeners_));
     EXPECT_CALL(signal_strategy_, GetLocalJid())
         .WillRepeatedly(Return(kTestJid));
+    EXPECT_CALL(mock_unknown_host_id_error_callback_, Run())
+        .Times(0);
 
     heartbeat_sender_.reset(new HeartbeatSender(
-        &mock_listener_, kHostId, &signal_strategy_, key_pair_, kTestBotJid));
+        mock_heartbeat_successful_callback_.GetCallback(),
+        mock_unknown_host_id_error_callback_.GetCallback(),
+        kHostId, &signal_strategy_, key_pair_, kTestBotJid));
   }
 
   virtual void TearDown() override {
@@ -98,7 +93,8 @@ class HeartbeatSenderTest
 
   base::MessageLoop message_loop_;
   MockSignalStrategy signal_strategy_;
-  MockListener mock_listener_;
+  MockClosure mock_heartbeat_successful_callback_;
+  MockClosure mock_unknown_host_id_error_callback_;
   std::set<SignalStrategy::Listener*> signal_strategy_listeners_;
   scoped_refptr<RsaKeyPair> key_pair_;
   scoped_ptr<HeartbeatSender> heartbeat_sender_;
@@ -241,7 +237,7 @@ void HeartbeatSenderTest::ProcessResponseWithInterval(
 
 // Verify that ProcessResponse parses set-interval result.
 TEST_F(HeartbeatSenderTest, ProcessResponseSetInterval) {
-  EXPECT_CALL(mock_listener_, OnHeartbeatSuccessful());
+  EXPECT_CALL(mock_heartbeat_successful_callback_, Run());
 
   ProcessResponseWithInterval(false, kTestInterval);
 
@@ -275,12 +271,10 @@ TEST_F(HeartbeatSenderTest, DoSetHostOfflineReason) {
   base::RunLoop().RunUntilIdle();
 }
 
-static void MarkCallbackAsRun(bool* didCallbackRun) {
-  *didCallbackRun = true;
-}
-
 // Make sure SetHostOfflineReason triggers a callback when bot responds.
 TEST_F(HeartbeatSenderTest, ProcessHostOfflineResponses) {
+  MockClosure mock_ack_callback;
+
   EXPECT_CALL(signal_strategy_, GetLocalJid())
       .WillRepeatedly(Return(kTestJid));
   EXPECT_CALL(signal_strategy_, GetNextId())
@@ -290,33 +284,35 @@ TEST_F(HeartbeatSenderTest, ProcessHostOfflineResponses) {
   EXPECT_CALL(signal_strategy_, GetState())
       .WillOnce(Return(SignalStrategy::DISCONNECTED))
       .WillRepeatedly(Return(SignalStrategy::CONNECTED));
-  EXPECT_CALL(mock_listener_, OnHeartbeatSuccessful())
+  EXPECT_CALL(mock_heartbeat_successful_callback_, Run())
       .WillRepeatedly(Return());
 
-  bool didCallbackRun = false;
+  // Callback should not run, until response to offline-reason.
+  EXPECT_CALL(mock_ack_callback, Run()).Times(0);
+
   heartbeat_sender_->SetHostOfflineReason(
       "test_error",
-      base::Bind(MarkCallbackAsRun, base::Unretained(&didCallbackRun)));
-  ASSERT_FALSE(didCallbackRun);
+      mock_ack_callback.GetCallback());
   heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
   base::RunLoop().RunUntilIdle();
-  ASSERT_FALSE(didCallbackRun) << "Callback shouldn't run before ack from bot";
 
   ProcessResponseWithInterval(
-      false /* <- this not a response to offline-reason*/,
+      false,  // <- This is not a response to offline-reason.
       kTestInterval);
   base::RunLoop().RunUntilIdle();
-  ASSERT_FALSE(didCallbackRun) << "Callback shouldn't run "
-      << "when getting a response to an earlier, non-offline stanza";
 
-  ProcessResponseWithInterval(true, kTestInterval);
+  // Callback should run once, when we get response to offline-reason.
+  EXPECT_CALL(mock_ack_callback, Run()).Times(1);
+  ProcessResponseWithInterval(
+      true,  // <- This is a response to offline-reason.
+      kTestInterval);
   base::RunLoop().RunUntilIdle();
-  ASSERT_TRUE(didCallbackRun) << "Ack from bot should trigger the callback";
 
-  didCallbackRun = false;
+  // When subsequent responses to offline-reason come,
+  // the callback should not be called again.
+  EXPECT_CALL(mock_ack_callback, Run()).Times(0);
   ProcessResponseWithInterval(true, kTestInterval);
   base::RunLoop().RunUntilIdle();
-  ASSERT_FALSE(didCallbackRun) << "Callback should only run once";
 
   heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
   base::RunLoop().RunUntilIdle();