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..56351c9d94bdd67c1d2819672d3a67b53d7e91c0 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(HeartbeatSender::AckOrTimeout));
+};
+
 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)));

[Wez, 2014/12/18 00:46:20]

How is this related to the purpose of this test?

   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,8 @@ 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(HeartbeatSender::AckOrTimeout::Ack))
+      .Times(1);

[Wez, 2014/12/18 00:46:20]

Tests should not mix EXPECT_CALL() in with logic that touches the relevant
object - the behaviour is then undefined. If you want to expect a sequence of
callbacks from these various test-cases then use an InSequence to force the
ordering of your EXPECT_CALL() calls and run things that way. Or create a
separate callback object for each run.

[Łukasz Anforowicz, 2014/12/18 19:02:33]

According to GMock documentation the behavior is well defined.

https://code.google.com/p/googlemock/wiki/ForDummies (I read that when writing
the code to double-check my expectations of what would happen, not when replying
to the CR feedback :-) has an explict section titled "Using Multiple
Expectations" and says: "when a mock method is invoked, Google Mock will search
the expectations in the reverse order they are defined, and stop when an active
expectation that matches the arguments is found (you can think of it as "newer
rules override older ones."). If the matching expectation cannot take any more
calls, you will get an upper-bound-violated failure."

So - in the test above I do not want to expect a *sequence* of calls.  I want to
waive the earlier expectation that ack_callback is not called.

   ProcessResponseWithInterval(
       true,  // <- This is a response to offline-reason.
       kTestInterval);
@@ -309,7 +325,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();