Reporting of policy errors via host-offline-reason: part 3

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 4a6888c08c03745a147b4bc642cc146fa333db58..80b528680ab195206d249ddd7367810529af9a79 100644
--- a/remoting/host/heartbeat_sender_unittest.cc
+++ b/remoting/host/heartbeat_sender_unittest.cc
@@ -14,7 +14,6 @@
 #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,6 +41,7 @@ const char kHostId[] = "0";
 const char kTestJid[] = "user@gmail.com/chromoting123";
 const char kStanzaId[] = "123";
 const int kTestInterval = 123;
+const base::TimeDelta kTestTimeout = base::TimeDelta::FromSeconds(123);
 
 }  // namespace
 
@@ -53,6 +53,16 @@ ACTION_P(RemoveListener, list) {
   list->erase(arg0);
 }
 
+class MockClosure {
+ public:
+  MOCK_CONST_METHOD0(Run, void());
+};
+
+class MockAckCallback {
+ public:
+  MOCK_CONST_METHOD1(Run, void(bool success));
+};
+
 class HeartbeatSenderTest
     : public testing::Test {
  protected:
@@ -72,8 +82,10 @@ class HeartbeatSenderTest
         .Times(0);
 
     heartbeat_sender_.reset(new HeartbeatSender(
-        mock_heartbeat_successful_callback_.GetCallback(),
-        mock_unknown_host_id_error_callback_.GetCallback(),
+        base::Bind(&MockClosure::Run,
+                   base::Unretained(&mock_heartbeat_successful_callback_)),
+        base::Bind(&MockClosure::Run,
+                   base::Unretained(&mock_unknown_host_id_error_callback_)),
         kHostId, &signal_strategy_, key_pair_, kTestBotJid));
   }
 
@@ -246,6 +258,8 @@ TEST_F(HeartbeatSenderTest, ProcessResponseSetInterval) {
 // Make sure SetHostOfflineReason sends a correct stanza.
 TEST_F(HeartbeatSenderTest, DoSetHostOfflineReason) {
   XmlElement* sent_iq = NULL;
+  MockAckCallback mock_ack_callback;
+
   EXPECT_CALL(signal_strategy_, GetLocalJid())
       .WillRepeatedly(Return(kTestJid));
   EXPECT_CALL(signal_strategy_, GetNextId())
@@ -255,10 +269,11 @@ TEST_F(HeartbeatSenderTest, DoSetHostOfflineReason) {
   EXPECT_CALL(signal_strategy_, GetState())
       .WillOnce(Return(SignalStrategy::DISCONNECTED))
       .WillRepeatedly(Return(SignalStrategy::CONNECTED));
+  EXPECT_CALL(mock_ack_callback, Run(_)).Times(0);
 
   heartbeat_sender_->SetHostOfflineReason(
-      "test_error",
-      base::Bind(base::DoNothing));
+      "test_error", kTestTimeout,
+      base::Bind(&MockAckCallback::Run, base::Unretained(&mock_ack_callback)));
   heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
   base::RunLoop().RunUntilIdle();
 
@@ -272,7 +287,7 @@ TEST_F(HeartbeatSenderTest, DoSetHostOfflineReason) {
 
 // Make sure SetHostOfflineReason triggers a callback when bot responds.
 TEST_F(HeartbeatSenderTest, ProcessHostOfflineResponses) {
-  MockClosure mock_ack_callback;
+  MockAckCallback mock_ack_callback;
 
   EXPECT_CALL(signal_strategy_, GetLocalJid())
       .WillRepeatedly(Return(kTestJid));
@@ -287,11 +302,11 @@ TEST_F(HeartbeatSenderTest, ProcessHostOfflineResponses) {
       .WillRepeatedly(Return());
 
   // Callback should not run, until response to offline-reason.
-  EXPECT_CALL(mock_ack_callback, Run()).Times(0);
+  EXPECT_CALL(mock_ack_callback, Run(_)).Times(0);
 
   heartbeat_sender_->SetHostOfflineReason(
-      "test_error",
-      mock_ack_callback.GetCallback());
+      "test_error", kTestTimeout,
+      base::Bind(&MockAckCallback::Run, base::Unretained(&mock_ack_callback)));
   heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
   base::RunLoop().RunUntilIdle();
 
@@ -301,7 +316,7 @@ TEST_F(HeartbeatSenderTest, ProcessHostOfflineResponses) {
   base::RunLoop().RunUntilIdle();
 
   // Callback should run once, when we get response to offline-reason.
-  EXPECT_CALL(mock_ack_callback, Run()).Times(1);
+  EXPECT_CALL(mock_ack_callback, Run(true /* success */)).Times(1);

[Wez, 2015/01/09 02:55:45]

In your other comments you indicated that true is failure, not success.

[Łukasz Anforowicz, 2015/01/09 19:00:13]

No.  I was trying to indicate that "false" is bound to "success" argument, not
that "false" means "success" (success=false means failure).

[Wez, 2015/01/10 01:43:32]

D'oh! OK.  The problem is that the meaning is ambiguous, so I think as long as
the API defines the meaning of the parameter, I'd remove the comment, to avoid
confusion. If you'd prefer to just use an enum for extreme clarity then that's
also fine with me - apologies for suggesting otherwise!

[Łukasz Anforowicz, 2015/01/12 18:05:14]

:-)

The declaration of SetHostOfflineReason names the parameter of the callback, so
I think the meaning of the parameter should be easy to understand by future
maintainers:

  void SetHostOfflineReason(
      ...
      const base::Callback<void(bool success)>& ack_callback);

I could in theory add an extra comment, but I think it would not add much value
and might just go unnoticed while definitely adding extra noise to the overall
comments volume.

   ProcessResponseWithInterval(
       true,  // <- This is a response to offline-reason.
       kTestInterval);
@@ -309,7 +324,7 @@ TEST_F(HeartbeatSenderTest, ProcessHostOfflineResponses) {
 
   // When subsequent responses to offline-reason come,
   // the callback should not be called again.
-  EXPECT_CALL(mock_ack_callback, Run()).Times(0);
+  EXPECT_CALL(mock_ack_callback, Run(_)).Times(0);
   ProcessResponseWithInterval(true, kTestInterval);
   base::RunLoop().RunUntilIdle();