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

remoting/host/ack_or_timeout_reporter_unittest.cc

Index: remoting/host/ack_or_timeout_reporter_unittest.cc
diff --git a/remoting/host/ack_or_timeout_reporter_unittest.cc b/remoting/host/ack_or_timeout_reporter_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f45b918ced0b503fdbd7e4d4a37583a8c7f0dabe
--- /dev/null
+++ b/remoting/host/ack_or_timeout_reporter_unittest.cc
@@ -0,0 +1,150 @@
+// Copyright 2014 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/ack_or_timeout_reporter.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::DoAll;
+using testing::SaveArg;
+using testing::Return;
+
+namespace remoting {
+
+class MockSequencedTaskRunner : public base::SequencedTaskRunner {
+ public:
+  MOCK_METHOD3(PostDelayedTask,
+               bool(const tracked_objects::Location&,
+                    const base::Closure&,
+                    base::TimeDelta));
+
+  MOCK_CONST_METHOD0(RunsTasksOnCurrentThread, bool());
+
+  MOCK_METHOD3(PostNonNestableDelayedTask,
+               bool(const tracked_objects::Location&,
+                    const base::Closure&,
+                    base::TimeDelta));
+
+ private:
+  ~MockSequencedTaskRunner() {}
+};
+
+class MockFunctionThatAcks {
+ public:
+  MOCK_CONST_METHOD1(Run, void(const base::Closure&));
+};
+
+class MockAckOrTimeoutCallback {
+ public:
+  MOCK_CONST_METHOD1(Run, void(AckOrTimeout));
+};
+
+const base::TimeDelta kTestTimeout = base::TimeDelta::FromSeconds(123);
+
+static void CallReportAckOrTimeoutAndCaptureCallbacks(
+    const base::Callback<void(AckOrTimeout)>& ack_or_timeout_callback,
+    /* out */ base::Closure* captured_ack_callback,
+    /* out */ base::Closure* captured_timeout_callback) {
+  // Setup mocks.
+  MockFunctionThatAcks mock_function_that_acks;
+  scoped_refptr<MockSequencedTaskRunner> mock_task_runner(
+      new MockSequencedTaskRunner());
+
+  // Capture |captured_ack_callback| and |captured_timeout_callback|.
+  EXPECT_CALL(mock_function_that_acks, Run(_))
+      .WillOnce(SaveArg<0>(captured_ack_callback));
+  EXPECT_CALL(*mock_task_runner, PostDelayedTask(_, _, kTestTimeout))
+      .WillOnce(DoAll(SaveArg<1>(captured_timeout_callback), Return(true)));

[Wez, 2014/12/09 00:51:58]

You shouldn't be checking for this call; tests should verify a specific intended
behaviour, not a specific sequence of calls.

In this case, you want to verify that ReportAckOrTimeout() will call you back
with Ack if passed a function that Acks, or with Timeout if you call it with a
function that never acks, or with one that acks, but too late - in the latter
case you should ideally "fast forward" time via a fake base::TickClock, rather
than actually waiting for a (short) timeout to expire.

[Łukasz Anforowicz, 2014/12/09 18:47:07]

I also don't like tight coupling between tests and production code in this case.

Can you please explain more about basing the test on base::TickClock?  How would
I tie together a test-controlled TickClock with either
sequenced_task_runner->PostDelayedTask and/or OneShotTimer?

Also - I note that:
- The test doesn't enforce a specific *sequence* of calls - the calls can happen
in any order.
- This is not that different from other, existing tests (i.e. HeartbeatSender
uses a public interface of XmppSignalStrategy - this interface gets mocked in
test code).

+
+  // Call function-under-test.
+  ReportAckOrTimeout(base::Bind(&MockFunctionThatAcks::Run,
+                                base::Unretained(&mock_function_that_acks)),
+                     kTestTimeout, mock_task_runner, ack_or_timeout_callback);
+}
+
+TEST(AckOrTimeoutReporterTest, FirstAckCalledAsynchronously) {
+  MockAckOrTimeoutCallback mock_ack_or_timeout_callback;
+  base::WeakPtrFactory<MockAckOrTimeoutCallback> weak_factory(
+      &mock_ack_or_timeout_callback);
+
+  base::Closure captured_ack_callback;
+  base::Closure captured_timeout_callback;
+
+  // |mock_ack_or_timeout_callback| should not be called until
+  // |captured_ack_callback| or |captured_timeout_callback| is called.
+  EXPECT_CALL(mock_ack_or_timeout_callback, Run(_)).Times(0);
+
+  CallReportAckOrTimeoutAndCaptureCallbacks(
+      base::Bind(&MockAckOrTimeoutCallback::Run, weak_factory.GetWeakPtr()),
+      &captured_ack_callback, &captured_timeout_callback);
+  ASSERT_FALSE(captured_ack_callback.is_null());
+  ASSERT_FALSE(captured_timeout_callback.is_null());
+
+  {
+    // |mock_ack_or_timeout_callback| should be called as soon as
+    // |captured_ack_callback| runs.
+    EXPECT_CALL(mock_ack_or_timeout_callback, Run(AckOrTimeout::Ack));
+    captured_ack_callback.Run();
+  }
+
+  // |ack_or_timeout_callback| should be dropped after being called once.
+  // This expectation is depended on by |MinimumHeartbeatSupporter| to
+  // manage its lifetime (i.e. release references to self after getting an
+  // ack).
+  EXPECT_FALSE(weak_factory.HasWeakPtrs());
+
+  // It should be safe to call the callbacks multiple times.
+  // We are veryfying that such callbacks do not trigger

[Lambros, 2014/12/06 01:25:05]

nit: verifying

[Łukasz Anforowicz, 2014/12/09 18:47:07]

Done.

+  // ack_or_timeout_callback by having EXPECT_CALL...Times(0) above.
+  captured_ack_callback.Run();
+  captured_timeout_callback.Run();
+}
+
+TEST(AckOrTimeoutReporterTest, FirstTimeoutCalledAsynchronously) {
+  MockAckOrTimeoutCallback mock_ack_or_timeout_callback;
+  base::WeakPtrFactory<MockAckOrTimeoutCallback> weak_factory(
+      &mock_ack_or_timeout_callback);
+
+  base::Closure captured_ack_callback;
+  base::Closure captured_timeout_callback;
+
+  // |mock_ack_or_timeout_callback| should not be called until
+  // |captured_ack_callback| or |captured_timeout_callback| is called.
+  EXPECT_CALL(mock_ack_or_timeout_callback, Run(_)).Times(0);
+
+  CallReportAckOrTimeoutAndCaptureCallbacks(
+      base::Bind(&MockAckOrTimeoutCallback::Run, weak_factory.GetWeakPtr()),
+      &captured_ack_callback, &captured_timeout_callback);
+  ASSERT_FALSE(captured_ack_callback.is_null());
+  ASSERT_FALSE(captured_timeout_callback.is_null());
+
+  {
+    // |mock_ack_or_timeout_callback| should be called as soon as
+    // |captured_ack_callback| runs.
+    EXPECT_CALL(mock_ack_or_timeout_callback, Run(AckOrTimeout::Timeout));
+    captured_timeout_callback.Run();
+  }
+
+  // |ack_or_timeout_callback| should be dropped after being called once.
+  // This expectation is depended on by |MinimumHeartbeatSupporter| to
+  // manage its lifetime (i.e. release references to self after getting an
+  // ack).
+  EXPECT_FALSE(weak_factory.HasWeakPtrs());
+
+  // It should be safe to call the callbacks multiple times.
+  // We are veryfying that such callbacks do not trigger
+  // ack_or_timeout_callback by having EXPECT_CALL...Times(0) above.
+  captured_ack_callback.Run();
+  captured_timeout_callback.Run();
+}
+
+}  // namespace remoting