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..e71be2da5377ef83bc77513a5a0ed49832496e24
--- /dev/null
+++ b/remoting/host/ack_or_timeout_reporter_unittest.cc
@@ -0,0 +1,139 @@
+// 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/location.h"
+#include "base/memory/weak_ptr.h"
+#include "base/sequenced_task_runner.h"

[Lambros, 2014/12/05 22:19:48]

Don't need here, already in ack_or_timeout_reporter.h

[Łukasz Anforowicz, 2014/12/05 23:59:28]

Done.

+#include "base/time/time.h"
+
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+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(testing::_))

[Lambros, 2014/12/05 22:19:48]

nit: We normally add using... declarations for all these testing:: names. See
for example remoting/protocol/jingle_session_unittest.cc

[Łukasz Anforowicz, 2014/12/05 23:59:28]

Done.

+      .WillOnce(testing::SaveArg<0>(captured_ack_callback));
+  EXPECT_CALL(*mock_task_runner,
+              PostDelayedTask(testing::_, testing::_, kTestTimeout))
+      .WillOnce(testing::DoAll(testing::SaveArg<1>(captured_timeout_callback),
+                               testing::Return(true)));
+
+  // 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(testing::_)).Times(0);
+
+  CallReportAckOrTimeoutAndCaptureCallbacks(
+      base::Bind(&MockAckOrTimeoutCallback::Run, weak_factory.GetWeakPtr()),
+      &captured_ack_callback, &captured_timeout_callback);
+
+  {
+    // |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 is expectation is depended on by |MinimumHeartbeatSupporter| to

[Lambros, 2014/12/05 22:19:48]

nit: This expectation...

[Łukasz Anforowicz, 2014/12/05 23:59:28]

Done.

+  // 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.

[Lambros, 2014/12/05 22:19:48]

Is this testing anything - there are no expectations here? Why would it not be
safe? And why do we care about being able to run these more than once?

[Łukasz Anforowicz, 2014/12/05 23:59:28]

I'll add a comment.  This tests 1) that ack_or_timeout_callback is not called
anymore and 2) that nothing crashes / references bad memory when keeping calling
the callbacks.

+  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(testing::_)).Times(0);
+
+  CallReportAckOrTimeoutAndCaptureCallbacks(
+      base::Bind(&MockAckOrTimeoutCallback::Run, weak_factory.GetWeakPtr()),
+      &captured_ack_callback, &captured_timeout_callback);
+
+  {
+    // |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.

[Lambros, 2014/12/05 22:19:48]

I think you can remove this? It's just a duplicate of the comment above.

[Łukasz Anforowicz, 2014/12/05 23:59:28]

I don't see how this is a duplicate :-/

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

This is the same comment as on line 99 (in patch set #12). I don't think we need
to duplicate comments within the same file, but I don't feel strongly about it.

+  // This is 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.
+  captured_ack_callback.Run();
+  captured_timeout_callback.Run();
+}
+
+}  // namespace remoting