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

remoting/host/heartbeat_sender_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/heartbeat_sender.h"
 
 #include <set>
 
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #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"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/webrtc/libjingle/xmllite/xmlelement.h"
 #include "third_party/webrtc/libjingle/xmpp/constants.h"
 
 using buzz::QName;
 using buzz::XmlElement;
 
 using testing::_;
 using testing::DeleteArg;
 using testing::DoAll;
 using testing::Invoke;
 using testing::NotNull;
 using testing::Return;
 using testing::SaveArg;
 
 namespace remoting {
 
 namespace {
 
 const char kTestBotJid[] = "remotingunittest@bot.talk.google.com";
 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
 
 ACTION_P(AddListener, list) {
   list->insert(arg0);
 }
 ACTION_P(RemoveListener, list) {
   EXPECT_TRUE(list->find(arg0) != list->end());
   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:
   void SetUp() override {
     key_pair_ = RsaKeyPair::FromString(kTestRsaKeyPair);
     ASSERT_TRUE(key_pair_.get());
 
     EXPECT_CALL(signal_strategy_, GetState())
         .WillOnce(Return(SignalStrategy::DISCONNECTED));
     EXPECT_CALL(signal_strategy_, AddListener(NotNull()))
         .WillRepeatedly(AddListener(&signal_strategy_listeners_));
     EXPECT_CALL(signal_strategy_, RemoveListener(NotNull()))
         .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_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));
   }
 
   void TearDown() override {
     heartbeat_sender_.reset();
     EXPECT_TRUE(signal_strategy_listeners_.empty());
   }
 
   void ValidateHeartbeatStanza(XmlElement* stanza,
                                const char* expected_sequence_id,
@@ skipping 152 matching lines @@
   EXPECT_CALL(mock_heartbeat_successful_callback_, Run());
 
   ProcessResponseWithInterval(false, kTestInterval);
 
   EXPECT_EQ(kTestInterval * 1000, heartbeat_sender_->interval_ms_);
 }
 
 // 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())
       .WillOnce(Return(kStanzaId));
   EXPECT_CALL(signal_strategy_, SendStanzaPtr(NotNull()))
       .WillOnce(DoAll(SaveArg<0>(&sent_iq), Return(true)));
   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();
 
   scoped_ptr<XmlElement> stanza(sent_iq);
   ASSERT_TRUE(stanza != NULL);
   ValidateHeartbeatStanza(stanza.get(), "0", "test_error");
 
   heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
   base::RunLoop().RunUntilIdle();
 }
 
 // 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));
   EXPECT_CALL(signal_strategy_, GetNextId())
       .WillOnce(Return(kStanzaId));
   EXPECT_CALL(signal_strategy_, SendStanzaPtr(NotNull()))
       .WillOnce(DoAll(DeleteArg<0>(), Return(true)));
   EXPECT_CALL(signal_strategy_, GetState())
       .WillOnce(Return(SignalStrategy::DISCONNECTED))
       .WillRepeatedly(Return(SignalStrategy::CONNECTED));
   EXPECT_CALL(mock_heartbeat_successful_callback_, Run())
       .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();
 
   ProcessResponseWithInterval(
       false,  // <- This is not a response to offline-reason.
       kTestInterval);
   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);
   base::RunLoop().RunUntilIdle();
 
   // 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();
 
   heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
   base::RunLoop().RunUntilIdle();
 }
 
 // Validate a heartbeat stanza.
 void HeartbeatSenderTest::ValidateHeartbeatStanza(
     XmlElement* stanza,
@@ skipping 23 matching lines @@
   EXPECT_TRUE(heartbeat_stanza->NextNamed(signature_tag) == NULL);
 
   scoped_refptr<RsaKeyPair> key_pair = RsaKeyPair::FromString(kTestRsaKeyPair);
   ASSERT_TRUE(key_pair.get());
   std::string expected_signature =
       key_pair->SignMessage(std::string(kTestJid) + ' ' + expected_sequence_id);
   EXPECT_EQ(expected_signature, signature->BodyText());
 }
 
 }  // namespace remoting