Added unittests for SystemLogUploader.

chrome/browser/chromeos/policy/system_log_upload_job_unittest.cc

+// Copyright 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 "ash/test/ash_test_base.h"
+#include "base/run_loop.h"
+#include "base/test/test_mock_time_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "chrome/browser/chromeos/policy/system_log_upload_job.h"
+#include "chrome/browser/chromeos/policy/upload_job.h"
+
+namespace policy {
+
+namespace {
+
+const char* const kMockUploadUrl = "http://example.com/upload";
+
+const char* const kTestSystemLogFileNames[] = {"my_system_log_file.txt"};
+
+class MockUploadJob : public policy::UploadJob {
+ public:
+  // If |error_code| is a null pointer OnSuccess() will be invoked when the
+  // Start() method is called, otherwise OnFailure() will be invoked with the
+  // respective |error_code|.
+  MockUploadJob(const GURL& upload_url,
+                UploadJob::Delegate* delegate,
+                scoped_ptr<UploadJob::ErrorCode> error_code);
+  ~MockUploadJob() override;
+
+  // policy::UploadJob:
+  void AddDataSegment(const std::string& name,
+                      const std::string& filename,
+                      const std::map<std::string, std::string>& header_entries,
+                      scoped_ptr<std::string> data) override;
+  void Start() override;
+
+ protected:
+  const GURL upload_url_;
+  UploadJob::Delegate* delegate_;
+  scoped_ptr<UploadJob::ErrorCode> error_code_;
+};
+
+MockUploadJob::MockUploadJob(const GURL& upload_url,
+                             UploadJob::Delegate* delegate,
+                             scoped_ptr<UploadJob::ErrorCode> error_code)
+    : upload_url_(upload_url),
+      delegate_(delegate),
+      error_code_(error_code.Pass()) {
+}
+
+MockUploadJob::~MockUploadJob() {
+}
+
+void MockUploadJob::AddDataSegment(
+    const std::string& name,
+    const std::string& filename,
+    const std::map<std::string, std::string>& header_entries,
+    scoped_ptr<std::string> data) {
+}
+
+void MockUploadJob::Start() {
+  DCHECK(delegate_);
+  EXPECT_EQ(kMockUploadUrl, upload_url_.spec());

[Andrew T Wilson (Slow), 2015/07/03 16:19:59]

What is this testing, exactly? And why are we testing it here instead of in the
constructor? It just seems weird because you're basically just testing that your
tests only pass in kMockUploadUrl to the constructor of your test class -- you
aren't testing any logic in the real classes, right?

[Polina Bondarenko, 2015/07/10 13:24:22]

Yes, removed.

+  if (error_code_) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&UploadJob::Delegate::OnFailure,
+                              base::Unretained(delegate_), *error_code_));
+    return;

[Andrew T Wilson (Slow), 2015/07/03 16:20:00]

This is OK as is, but usually I prefer:

if (error_code_) {
  HANDLE_ERROR
} else {
  HANDLE NO ERROR
}

rather than

if (error_code_) {
  HANDLE_ERROR()
  return;
}
HANDLE_NO_ERROR()

[Polina Bondarenko, 2015/07/10 13:24:22]

Done.

+  }
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&UploadJob::Delegate::OnSuccess, base::Unretained(delegate_)));
+}
+
+// Generate the fake system log files.
+scoped_ptr<SystemLogUploadJob::SystemLogs> GenerateTestSystemLogFiles() {
+  scoped_ptr<SystemLogUploadJob::SystemLogs> system_logs(
+      new SystemLogUploadJob::SystemLogs());
+  for (auto const file_path : kTestSystemLogFileNames) {
+    system_logs->push_back(std::make_pair(file_path, file_path));

[Andrew T Wilson (Slow), 2015/07/03 16:20:00]

Is there any code that tests that this file data is correctly processed and
included in upload data?

[Polina Bondarenko, 2015/07/10 13:24:21]

Added.

+  }
+  return system_logs;
+}
+
+class MockSystemLogDelegate : public SystemLogUploadJob::Delegate {
+ public:
+  explicit MockSystemLogDelegate(
+      scoped_ptr<UploadJob::ErrorCode> upload_job_error_code);
+  ~MockSystemLogDelegate() override;
+
+  void LoadSystemLogs(
+      const SystemLogUploadJob::LogUploadCallback& upload_callback) override;
+  scoped_ptr<UploadJob> CreateUploadJob(const GURL&,
+                                        UploadJob::Delegate*) override;
+
+ private:
+  scoped_ptr<UploadJob::ErrorCode> upload_job_error_code_;
+};
+
+MockSystemLogDelegate::MockSystemLogDelegate(
+    scoped_ptr<UploadJob::ErrorCode> upload_job_error_code)
+    : upload_job_error_code_(upload_job_error_code.Pass()) {
+}
+
+MockSystemLogDelegate::~MockSystemLogDelegate() {
+}
+
+void MockSystemLogDelegate::LoadSystemLogs(
+    const SystemLogUploadJob::LogUploadCallback& upload_callback) {
+  scoped_ptr<SystemLogUploadJob::SystemLogs> test_files =
+      GenerateTestSystemLogFiles();
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(upload_callback, base::Owned(test_files.release())));
+}
+
+scoped_ptr<UploadJob> MockSystemLogDelegate::CreateUploadJob(
+    const GURL& upload_url,
+    UploadJob::Delegate* delegate) {
+  return make_scoped_ptr(new MockUploadJob(GURL(kMockUploadUrl), delegate,
+                                           upload_job_error_code_.Pass()));
+}
+
+}  // namespace
+
+class SystemLogUploadTest : public ash::test::AshTestBase {

[Andrew T Wilson (Slow), 2015/07/03 16:19:59]

Why are we deriving from AshTestBase? I don't think we need anything in there?

[Polina Bondarenko, 2015/07/10 13:24:21]

Done.

+ public:
+  void VerifyResults(SystemLogUploadJob* job,
+                     SystemLogUploadJob::Status expected_status);
+
+ protected:
+  SystemLogUploadTest();
+
+  base::RunLoop run_loop_;
+
+ private:
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemLogUploadTest);
+};
+
+SystemLogUploadTest::SystemLogUploadTest()
+    : task_runner_(new base::TestMockTimeTaskRunner()) {
+}
+
+void SystemLogUploadTest::VerifyResults(
+    SystemLogUploadJob* job,
+    SystemLogUploadJob::Status expected_status) {
+  EXPECT_EQ(expected_status, job->status());
+
+  run_loop_.Quit();
+}
+
+TEST_F(SystemLogUploadTest, Success) {
+  scoped_ptr<SystemLogUploadJob> job(new SystemLogUploadJob(
+      make_scoped_ptr(new MockSystemLogDelegate(nullptr))));
+
+  job->Run(
+      base::Bind(&SystemLogUploadTest::VerifyResults, base::Unretained(this),
+                 base::Unretained(job.get()), SystemLogUploadJob::SUCCEEDED),
+      base::Bind(&SystemLogUploadTest::VerifyResults, base::Unretained(this),

[Andrew T Wilson (Slow), 2015/07/03 16:19:59]

Seems like it should be an error for this second callback to ever be invoked -
maybe you should have some kind of callback that just has a FAIL() statement if
it's ever called?

[Polina Bondarenko, 2015/07/10 13:24:22]

Done.

+                 base::Unretained(job.get()), SystemLogUploadJob::SUCCEEDED));
+  run_loop_.Run();
+}
+
+TEST_F(SystemLogUploadTest, Failure) {
+  using ErrorCode = UploadJob::ErrorCode;
+  scoped_ptr<ErrorCode> error_code(
+      new ErrorCode(UploadJob::AUTHENTICATION_ERROR));
+  scoped_ptr<SystemLogUploadJob> job(new SystemLogUploadJob(
+      make_scoped_ptr(new MockSystemLogDelegate(error_code.Pass()))));
+
+  job->Run(
+      base::Bind(&SystemLogUploadTest::VerifyResults, base::Unretained(this),
+                 base::Unretained(job.get()), SystemLogUploadJob::FAILED),
+      base::Bind(&SystemLogUploadTest::VerifyResults, base::Unretained(this),

[Andrew T Wilson (Slow), 2015/07/03 16:20:00]

ditto here, success callback shouldn't be called, right?

[Polina Bondarenko, 2015/07/10 13:24:21]

Done.

+                 base::Unretained(job.get()), SystemLogUploadJob::FAILED));
+  run_loop_.Run();
+}
+
+}  // namespace policy