Added unittests for SystemLogUploader.

chrome/browser/chromeos/policy/system_log_uploader_unittest.cc

Index: chrome/browser/chromeos/policy/system_log_uploader_unittest.cc
diff --git a/chrome/browser/chromeos/policy/system_log_uploader_unittest.cc b/chrome/browser/chromeos/policy/system_log_uploader_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8cbe898acea01db7dd076e74223bb25a55bae546
--- /dev/null
+++ b/chrome/browser/chromeos/policy/system_log_uploader_unittest.cc
@@ -0,0 +1,145 @@
+// Copyright (c) 2015 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 "base/test/test_simple_task_runner.h"
+#include "base/time/time.h"
+#include "chrome/browser/chromeos/policy/system_log_uploader.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace policy {
+
+namespace {
+
+// MockSystemLogDelegate - mock class that does nothing.
+class MockSystemLogDelegate : public SystemLogUploadJob::Delegate {
+ public:
+  MockSystemLogDelegate() {}
+  ~MockSystemLogDelegate() override {}
+
+  void LoadSystemLogs(const LogUploadCallback& upload_callback) override {}
+  scoped_ptr<UploadJob> CreateUploadJob(const GURL&,

[Andrew T Wilson (Slow), 2015/07/17 15:59:28]

Maybe explain that neither of these should ever be called (since you are just
testing retry logic)?

Can you explain why you aren't just passing nullptr in as the
SystemLogUploadJob::Delegate (why do we need this class at all if this object
should never be used)?

[Polina Bondarenko, 2015/07/29 15:36:02]

Changed the system log upload logic here:
https://codereview.chromium.org/1193333017/

Now MockUploadJob is created for every scheduled log upload.

+                                        UploadJob::Delegate*) override {
+    return nullptr;
+  }
+};
+
+// MockSystemLogUploadJob - mock class that invokes appropriate
+// SystemLogUploader's callback.
+class MockSystemLogUploadJob : public SystemLogUploadJob {
+ public:
+  // Store response from server on system log upload.
+  explicit MockSystemLogUploadJob(
+      scoped_ptr<MockSystemLogDelegate> syslog_delegate,
+      const base::Closure& succeeded_callback,
+      const base::Closure& failed_callback,
+      bool response)
+      : SystemLogUploadJob(syslog_delegate.Pass(),
+                           succeeded_callback,
+                           failed_callback) {
+    // Store only appropriate callback to be invoked.
+    if (response)
+      callback = succeeded_callback;
+    else
+      callback = failed_callback;
+  }
+
+  ~MockSystemLogUploadJob() override {}
+
+  // Runs succeeded or failed callback according to response_ stored.
+  void Run() override { callback.Run(); }

[Andrew T Wilson (Slow), 2015/07/17 15:59:28]

This is OK, but would be even better if we could easily post a task with the
response to better match the behavior of the real class (which never calls the
callback from within the Run() method. But I'm fine with this.

BTW, would the callback free this class? Why don't we need to call
ResetAndReturn() here?

[Polina Bondarenko, 2015/07/29 15:36:02]

Removed this MockSystemLogUploadJob at all.

+
+ private:
+  base::Closure callback;
+};
+
+// MockSystemLogUploader - overrides only the creation of the system log
+// upload job of SystemLogUploader.
+class MockSystemLogUploader : public SystemLogUploader {
+ public:
+  MockSystemLogUploader(
+      const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+      bool response)
+      : SystemLogUploader(task_runner), response_(response) {}
+
+ private:
+  SystemLogUploadJob* CreateSystemLogUploadJob(
+      const base::Closure& succeeded_callback,
+      const base::Closure& failed_callback) override {
+    // Passes |response_| to the upload job.
+    return new MockSystemLogUploadJob(
+        make_scoped_ptr(new MockSystemLogDelegate()), succeeded_callback,
+        failed_callback, response_);
+  }
+
+  // Passed to the upload job.
+  bool response_;
+};
+
+}  //  namespace
+
+class SystemLogUploaderTest : public testing::Test {
+ public:
+  SystemLogUploaderTest() : task_runner_(new base::TestSimpleTaskRunner()) {}
+
+  // Given a pending task to upload system logs.
+  void RunPendingUploadTaskAndCheckNext(const MockSystemLogUploader& uploader,
+                                        base::TimeDelta expected_delay) {
+    EXPECT_FALSE(task_runner_->GetPendingTasks().empty());
+    task_runner_->RunPendingTasks();
+
+    // The previours task scheduled new log upload task.

[Andrew T Wilson (Slow), 2015/07/17 15:59:28]

nit: The previous task should have uploaded another log upload task.

[Polina Bondarenko, 2015/07/29 15:36:02]

Done.

+    EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+
+    CheckPendingTaskDelay(uploader, expected_delay);
+  }
+
+  void CheckPendingTaskDelay(const MockSystemLogUploader& uploader,
+                             base::TimeDelta expected_delay) {
+    // The next task should be scheduled sometime between
+    // |last_upload_attempt| + |expected_delay| and
+    // |now| + |expected_delay|.
+    base::Time now = base::Time::NowFromSystemTime();
+    base::Time next_task = now + task_runner_->NextPendingTaskDelay();
+
+    EXPECT_LE(next_task, now + expected_delay);
+    EXPECT_GE(next_task, uploader.last_upload_attempt() + expected_delay);
+  }
+
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+};
+
+// One success task pending.
+TEST_F(SystemLogUploaderTest, SuccessTest) {
+  EXPECT_TRUE(task_runner_->GetPendingTasks().empty());
+
+  MockSystemLogUploader uploader(task_runner_, true);
+
+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+
+  CheckPendingTaskDelay(uploader,
+                        base::TimeDelta::FromMilliseconds(
+                            SystemLogUploader::kDefaultUploadDelayMs));
+}
+
+// Three failed responses recieved.
+TEST_F(SystemLogUploaderTest, ThreeFailureTest) {
+  EXPECT_TRUE(task_runner_->GetPendingTasks().empty());
+
+  MockSystemLogUploader uploader(task_runner_, false);
+
+  EXPECT_EQ(1U, task_runner_->GetPendingTasks().size());
+
+  // Do not retry two times consequentially.
+  RunPendingUploadTaskAndCheckNext(uploader,
+                                   base::TimeDelta::FromMilliseconds(
+                                       SystemLogUploader::kErrorUploadDelayMs));
+  RunPendingUploadTaskAndCheckNext(
+      uploader, base::TimeDelta::FromMilliseconds(
+                    SystemLogUploader::kDefaultUploadDelayMs));

[Andrew T Wilson (Slow), 2015/07/17 15:59:28]

Maybe highlight that we are using the upload_delay and not the error_delay here
because there's just one retry?

[Polina Bondarenko, 2015/07/29 15:36:02]

Done.

+  RunPendingUploadTaskAndCheckNext(uploader,
+                                   base::TimeDelta::FromMilliseconds(
+                                       SystemLogUploader::kErrorUploadDelayMs));
+}
+
+}  // namespace policy