Add tests for the JobController's observer.

content/browser/background_fetch/background_fetch_job_controller_unittest.cc

 // Copyright 2017 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 "content/browser/background_fetch/background_fetch_job_controller.h"
 
+#include <string>
+#include <vector>
+
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
+#include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "content/browser/background_fetch/background_fetch_data_manager.h"
 #include "content/browser/background_fetch/background_fetch_job_info.h"
 #include "content/browser/background_fetch/background_fetch_request_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/mock_download_item.h"
 #include "content/public/test/mock_download_manager.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
 const char kOrigin[] = "https://example.com/";
 const char kJobGuid[] = "TestRequestGuid";
 constexpr int64_t kServiceWorkerRegistrationId = 9001;
 const char kTestUrl[] = "http://www.example.com/example.html";
 const char kTag[] = "testTag";
 
 }  // namespace
 
 namespace content {
 
 // Use the basic MockDownloadItem, but override it to provide a valid GUID.
 class MockDownloadItemWithValues : public MockDownloadItem {

[Peter Beverloo, 2017/03/10 00:55:14]

How about moving //chrome/browser/ntp_snippets/fake_download_item.h to a
location where this test can access it? That seems like a more scalable approach
that we can extend upon in the future.

[harkness, 2017/03/10 13:50:57]

Ah, good find, I hadn't seen that. I'll move it to somewhere publicly
accessible.

  public:
+  MockDownloadItemWithValues(const std::string& guid) : guid_(guid) {}
+
   const std::string& GetGuid() const override { return guid_; }
   void SetGuid(const std::string& guid) { guid_ = guid; }
 
+  DownloadState GetState() const override { return state_; }
+  void SetState(DownloadState state) { state_ = state; }
+
  private:
   std::string guid_;
+  DownloadState state_ = DownloadItem::DownloadState::IN_PROGRESS;
 };
 
 // Use the basic MockDownloadManager, but override it so that it implements the
 // functionality that the JobController requires.
 class MockDownloadManagerWithCallback : public MockDownloadManager {
  public:
   void DownloadUrl(std::unique_ptr<DownloadUrlParameters> params) override {
     DownloadUrlMock(params.get());
-    params->callback().Run(&download_item_, DOWNLOAD_INTERRUPT_REASON_NONE);
+    std::string guid = base::GenerateGUID();
+    download_items_.emplace_back(
+        base::MakeUnique<MockDownloadItemWithValues>(guid));
+    params->callback().Run(download_items_.back().get(),
+                           DOWNLOAD_INTERRUPT_REASON_NONE);

[Peter Beverloo, 2017/03/10 00:55:14]

The `.back()` is a bit odd. What about:

    std::unique_ptr<MockDownloadItemWithValues> item =
        base::MakeUnique<MockDownloadItemWithValues>(base::GenerateGUID());

    params->callback().Run(item.get(), DOWNLOAD_INTERRUPT_REASON_NONE);
    download_items_.push_back(std::move(item));

[harkness, 2017/03/10 13:50:57]

Done.

   }
 
+  // This is very inefficient, but is only called during shutdown.
   DownloadItem* GetDownloadByGuid(const std::string& guid) override {
-    return &download_item_;
+    for (const auto& item : download_items_) {
+      if (item->GetGuid() == guid)
+        return item.get();
+    }
+    return nullptr;
   }
 
-  MockDownloadItemWithValues* download_item() { return &download_item_; }
+  std::vector<std::unique_ptr<MockDownloadItemWithValues>>& download_items() {

[Peter Beverloo, 2017/03/10 00:55:14]

Two thoughts:

  (1) This is a test, so it'd be OK to expose |download_items_| as a protected
property.
  (2) If we decide to keep this, it should return a const reference and the
method itself should be const too.

[harkness, 2017/03/10 13:50:57]

If the property was on BackgroundFetchJObControllerTest, I'd go with protected,
but I decided to go with const instead.

+    return download_items_;
+  }
 
  private:
-  MockDownloadItemWithValues download_item_;
+  std::vector<std::unique_ptr<MockDownloadItemWithValues>> download_items_;
 };
 
 class BackgroundFetchJobControllerTest : public ::testing::Test {
  public:
   BackgroundFetchJobControllerTest()
       : download_manager_(new MockDownloadManagerWithCallback()) {}
   ~BackgroundFetchJobControllerTest() override = default;
 
   void SetUp() override {
     // The download_manager_ ownership is given to the BrowserContext, and the
@@ skipping 34 matching lines @@
 
   DownloadItem::Observer* ItemObserver() const { return job_controller_.get(); }
 
  private:
   TestBrowserThreadBundle thread_bundle_;
   TestBrowserContext browser_context_;
   std::unique_ptr<BackgroundFetchJobController> job_controller_;
   MockDownloadManagerWithCallback* download_manager_;
 };
 
-TEST_F(BackgroundFetchJobControllerTest, StartDownload) {
+TEST_F(BackgroundFetchJobControllerTest, SingleRequestJob) {
   BackgroundFetchJobInfo job_info(kTag, url::Origin(GURL(kOrigin)),
                                   kServiceWorkerRegistrationId);
   BackgroundFetchRequestInfo request_info(GURL(kTestUrl), kJobGuid);
   std::vector<BackgroundFetchRequestInfo> request_infos{request_info};
 
-  // Create a MockDownloadItem that the test can manipulate.
-  MockDownloadItemWithValues* item = download_manager()->download_item();
-  item->SetGuid("foo");
-
   // Get a JobData to give to the JobController. The JobController then gets
   // the BackgroundFetchRequestInfos from the JobData. The JobController will
   // take ownership of the JobData.
   BackgroundFetchJobData* job_data = new BackgroundFetchJobData(request_infos);
   InitializeJobController(job_data);
 
   EXPECT_CALL(*(download_manager()),
               DownloadUrlMock(::testing::Pointee(::testing::Property(
                   &DownloadUrlParameters::url, GURL(kTestUrl)))))
       .Times(1);
 
   StartProcessing();
+
+  // Get one of the pending downloads from the download manager.
+  auto& download_items = download_manager()->download_items();
+  EXPECT_EQ(1U, download_items.size());
+  MockDownloadItemWithValues* item = download_items[0].get();

[Peter Beverloo, 2017/03/10 00:55:14]

Since we'd be accessing an invalid entry here if |download_items.size()| is not
one (or more), line 157 should be an ASSERT_EQ. Same on line 215.

[harkness, 2017/03/10 13:50:57]

Done.

+
+  // Update the observer with no actual change.
+  ItemObserver()->OnDownloadUpdated(item);
+  EXPECT_FALSE(job_data->IsComplete());
+
+  // Update the item to be completed then update the observer. The JobController
+  // should update the JobData that the request is complete.
+  item->SetState(DownloadItem::DownloadState::COMPLETE);
+  ItemObserver()->OnDownloadUpdated(item);
+  EXPECT_TRUE(job_data->IsComplete());
+}
+
+TEST_F(BackgroundFetchJobControllerTest, MultipleRequestJob) {
+  BackgroundFetchJobInfo job_info(kTag, url::Origin(GURL(kOrigin)),
+                                  kServiceWorkerRegistrationId);
+  std::vector<BackgroundFetchRequestInfo> request_infos;
+  for (int i = 0; i < 10; i++) {
+    request_infos.emplace_back(
+        BackgroundFetchRequestInfo(GURL(kTestUrl), base::IntToString(i)));
+  }
+
+  // Get a JobData to give to the JobController. The JobController then gets
+  // the BackgroundFetchRequestInfos from the JobData. The JobController will
+  // take ownership of the JobData.
+  BackgroundFetchJobData* job_data = new BackgroundFetchJobData(request_infos);
+  InitializeJobController(job_data);
+
+  EXPECT_CALL(*(download_manager()),
+              DownloadUrlMock(::testing::Pointee(::testing::Property(
+                  &DownloadUrlParameters::url, GURL(kTestUrl)))))
+      .Times(10);
+
+  StartProcessing();
+
+  // Get one of the pending downloads from the download manager.
+  auto& download_items = download_manager()->download_items();
+  ASSERT_EQ(1U, download_items.size());
+  MockDownloadItemWithValues* item = download_items[0].get();
+
+  // Update the observer with no actual change.
+  ItemObserver()->OnDownloadUpdated(item);
+  EXPECT_FALSE(job_data->IsComplete());
+  EXPECT_EQ(1U, download_items.size());
+
+  for (size_t i = 0; i < 9; i++) {
+    // Update the next item to be completed then update the observer.
+    ASSERT_EQ(i + 1, download_items.size());
+    item = download_items[i].get();
+    item->SetState(DownloadItem::DownloadState::COMPLETE);
+    ItemObserver()->OnDownloadUpdated(item);
+    EXPECT_FALSE(job_data->IsComplete());
+  }
+  EXPECT_FALSE(job_data->HasRequestsRemaining());
+
+  // Finally, update the last request to be complete. The JobController should
+  // see that there are no more requests and mark the job as done.
+  ASSERT_EQ(10U, download_items.size());
+  item = download_items[9].get();
+  item->SetState(DownloadItem::DownloadState::COMPLETE);
+  ItemObserver()->OnDownloadUpdated(item);
+  EXPECT_TRUE(job_data->IsComplete());
 }
 
 }  // namespace content