Connect BackgroundFetch to the OfflineItemCollection

chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc

Index: chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
diff --git a/chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc b/chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..900539319911544faf54fa784fa92f12b36b3023
--- /dev/null
+++ b/chrome/browser/background_fetch/background_fetch_client_impl_unittest.cc
@@ -0,0 +1,94 @@
+// 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 "chrome/browser/background_fetch/background_fetch_client_impl.h"
+
+#include "base/macros.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/background_fetch/background_fetch_client_factory.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/offline_items_collection/core/offline_item.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+const char kRegistrationId[] = "1234:www.example.com:game_data";
+const char kNamespace[] = "BackgroundFetchNamespace";
+
+class FakeBackgroundFetchDelegate
+    : public content::BackgroundFetchClient::Delegate {
+ public:
+  explicit FakeBackgroundFetchDelegate(const std::string& id)
+      : expected_id_(id) {}
+
+  void CancelDownload(const std::string& registration_id) override {
+    ASSERT_EQ(registration_id, expected_id_);
+  }
+
+  void PauseDownload(const std::string& registration_id) override {
+    ASSERT_EQ(registration_id, expected_id_);
+  }
+
+  void ResumeDownload(const std::string& registration_id) override {
+    ASSERT_EQ(registration_id, expected_id_);
+  }
+
+ private:
+  std::string expected_id_;
+};
+
+class BackgroundFetchTestingProfile : public TestingProfile {
+ public:
+  BackgroundFetchTestingProfile() {}
+  ~BackgroundFetchTestingProfile() override {}
+
+  BackgroundFetchClientImpl* GetBackgroundFetchClient() override {
+    return BackgroundFetchClientFactory::GetForProfile(this);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BackgroundFetchTestingProfile);
+};
+
+}  // namespace
+
+class BackgroundFetchClientTest : public ::testing::Test {
+ public:
+  BackgroundFetchClientTest() {}
+  ~BackgroundFetchClientTest() override {}
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+
+ protected:
+  BackgroundFetchTestingProfile profile_;
+};
+
+TEST_F(BackgroundFetchClientTest, CancelDownloadTest) {
+  BackgroundFetchClientImpl* client = profile_.GetBackgroundFetchClient();
+  FakeBackgroundFetchDelegate delegate(kRegistrationId);
+  client->SetDelegate(&delegate);
+
+  offline_items_collection::ContentId id(kNamespace, kRegistrationId);
+  ASSERT_NO_FATAL_FAILURE(client->CancelDownload(id));
+}
+
+TEST_F(BackgroundFetchClientTest, PauseDownloadTest) {
+  BackgroundFetchClientImpl* client = profile_.GetBackgroundFetchClient();
+  FakeBackgroundFetchDelegate delegate(kRegistrationId);
+  client->SetDelegate(&delegate);
+
+  offline_items_collection::ContentId id(kNamespace, kRegistrationId);
+  ASSERT_NO_FATAL_FAILURE(client->PauseDownload(id));
+}
+
+TEST_F(BackgroundFetchClientTest, ResumeDownloadTest) {
+  BackgroundFetchClientImpl* client = profile_.GetBackgroundFetchClient();
+  FakeBackgroundFetchDelegate delegate(kRegistrationId);
+  client->SetDelegate(&delegate);
+
+  offline_items_collection::ContentId id(kNamespace, kRegistrationId);
+  ASSERT_NO_FATAL_FAILURE(client->ResumeDownload(id));

[Peter Beverloo, 2017/03/31 01:32:23]

I've got two worries with these tests:

  (1) Half of the ASSERT_EQ in the faked delegate above is already done by the
DCHECKs in the BackgroundFetchClientImpl.
  (2) The |delegate| will go out of scope after these function's bodies, which
means that the BackgroundFetchClientImpl continues to have an invalid pointer.

If this is all we can reasonably test at this level, let's punt tests for now?

[harkness, 2017/03/31 10:11:43]

I don't really see a problem with #1. I've cleaned up #2.

I've also added an extra call to the methods after the cleanup, to make sure
that the ClientImpl doesn't crash if the delegate is null.

I agree that these tests aren't testing complex functionality, but they are
testing basic plumbing, which I think has some value.

One alternative I considered was having individual members in the
FakeBackgroundFetchDelegate for each method: cancelled_id_, paused_id_, etc, and
then in the tests checking that the correct method had been called. That would
be a sanity check test against copy/paste code, but I decided it was a lot of
bloat for not much value. If you think it has value though, I can certainly add
that in.

[Peter Beverloo, 2017/03/31 11:54:06]

Well, we always need to gauge whether adding tests is worth it. The logic
involved in the plumbing couldn't be simpler. (Which is a good thing!) Do you
think additional state may be added?

[harkness, 2017/04/03 12:47:22]

I think any additional complexity will be on the content -> chrome side. I'll
work on adding that in now and if you still think I should pull this out after
that, I will.

+}