New snippets now replace old snippets and do not merge

components/ntp_snippets/content_suggestions_service_unittest.cc

 // Copyright 2016 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 "components/ntp_snippets/content_suggestions_service.h"
 
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/ntp_snippets/category_info.h"
@@ skipping 13 matching lines @@
 using testing::IsNull;
 using testing::Mock;
 using testing::NotNull;
 using testing::Property;
 using testing::_;
 
 namespace ntp_snippets {
 
 namespace {
 
-// Returns a suggestion instance for testing.
-ContentSuggestion CreateSuggestion(int number) {
-  return ContentSuggestion(
-      base::IntToString(number),
-      GURL("http://testsuggestion/" + base::IntToString(number)));
-}
-
-std::vector<ContentSuggestion> CreateSuggestions(
-    const std::vector<int>& numbers) {
-  std::vector<ContentSuggestion> result;
-  for (int number : numbers) {
-    result.emplace_back(CreateSuggestion(number));
-  }
-  return result;
-}
-
 class MockProvider : public ContentSuggestionsProvider {
  public:
   MockProvider(Observer* observer,
                CategoryFactory* category_factory,
                const std::vector<Category>& provided_categories)
       : ContentSuggestionsProvider(observer, category_factory) {
     SetProvidedCategories(provided_categories);
   }
 
   void SetProvidedCategories(const std::vector<Category>& provided_categories) {
     statuses_.clear();
     provided_categories_ = provided_categories;
     for (Category category : provided_categories) {
       statuses_[category.id()] = CategoryStatus::AVAILABLE;
     }
   }
 
   CategoryStatus GetCategoryStatus(Category category) {
     return statuses_[category.id()];
   }
 
   CategoryInfo GetCategoryInfo(Category category) {
     return CategoryInfo(base::ASCIIToUTF16("Section title"),
                         ContentSuggestionsCardLayout::FULL_CARD, true, true);
   }
 
-  void FireSuggestionsChanged(Category category,
-                              const std::vector<int>& numbers) {
-    observer()->OnNewSuggestions(this, category, CreateSuggestions(numbers));
+  void FireSuggestionsChanged(
+      Category category,
+      std::vector<ContentSuggestion> suggestions) {
+    observer()->OnNewSuggestions(this, category, std::move(suggestions));
   }
 
   void FireCategoryStatusChanged(Category category, CategoryStatus new_status) {
     statuses_[category.id()] = new_status;
     observer()->OnCategoryStatusChanged(this, category, new_status);
   }
 
   void FireCategoryStatusChangedWithCurrentStatus(Category category) {
     observer()->OnCategoryStatusChanged(this, category,
                                         statuses_[category.id()]);
@@ skipping 53 matching lines @@
   // returned by the service for the given |category|.
   void ExpectThatSuggestionsAre(Category category, std::vector<int> numbers) {
     std::vector<Category> categories = service()->GetCategories();
     auto position = std::find(categories.begin(), categories.end(), category);
     if (!numbers.empty()) {
       EXPECT_NE(categories.end(), position);
     }
 
     for (const auto& suggestion :
          service()->GetSuggestionsForCategory(category)) {
+      std::string id_string =

[Marc Treib, 2016/09/23 13:31:29]

within_category_id ?

[jkrcal, 2016/09/23 16:11:48]

Done.

+          service()->category_factory()->GetWithinCategoryIDFromUniqueID(
+              suggestion.id());
       int id;
-      ASSERT_TRUE(base::StringToInt(suggestion.id(), &id));
+      ASSERT_TRUE(base::StringToInt(id_string, &id));
       auto position = std::find(numbers.begin(), numbers.end(), id);
       if (position == numbers.end()) {
         ADD_FAILURE() << "Unexpected suggestion with ID " << id;
       } else {
         numbers.erase(position);
       }
     }
     for (int number : numbers) {
       ADD_FAILURE() << "Suggestion number " << number
                     << " not present, though expected";
@@ skipping 34 matching lines @@
   void CreateContentSuggestionsService(
       ContentSuggestionsService::State enabled) {
     ASSERT_FALSE(service_);
     service_.reset(new ContentSuggestionsService(enabled,
                                                  nullptr /* history_service */,
                                                  nullptr /* pref_service */));
   }
 
   ContentSuggestionsService* service() { return service_.get(); }
 
+  // Returns a suggestion instance for testing.
+  ContentSuggestion CreateSuggestion(Category category, int number) {
+    return ContentSuggestion(
+        service()->category_factory()->MakeUniqueID(category,
+                                                    base::IntToString(number)),
+        GURL("http://testsuggestion/" + base::IntToString(number)));
+  }
+
+  std::vector<ContentSuggestion> CreateSuggestions(
+      Category category,
+      const std::vector<int>& numbers) {
+    std::vector<ContentSuggestion> result;
+    for (int number : numbers) {
+      result.emplace_back(CreateSuggestion(category, number));

[Marc Treib, 2016/09/23 13:31:29]

nit: push_back will do the exact same thing here I think

[jkrcal, 2016/09/23 16:11:48]

Done, thanks!

+    }
+    return result;
+  }
+
  private:
   std::unique_ptr<ContentSuggestionsService> service_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsServiceTest);
 };
 
 class ContentSuggestionsServiceDisabledTest
     : public ContentSuggestionsServiceTest {
  public:
   void SetUp() override {
@@ skipping 57 matching lines @@
               IsEmpty());
 }
 
 TEST_F(ContentSuggestionsServiceTest, ShouldRedirectFetchSuggestionImage) {
   Category articles_category = FromKnownCategory(KnownCategories::ARTICLES);
   Category offline_pages_category =
       FromKnownCategory(KnownCategories::DOWNLOADS);
   MockProvider* provider1 = RegisterProvider(articles_category);
   MockProvider* provider2 = RegisterProvider(offline_pages_category);
 
-  provider1->FireSuggestionsChanged(articles_category, {1});
-  std::string suggestion_id = CreateSuggestion(1).id();
+  provider1->FireSuggestionsChanged(articles_category,
+                                    CreateSuggestions(articles_category, {1}));
+  std::string suggestion_id = CreateSuggestion(articles_category, 1).id();
 
   EXPECT_CALL(*provider1, FetchSuggestionImage(suggestion_id, _));
   EXPECT_CALL(*provider2, FetchSuggestionImage(_, _)).Times(0);
   service()->FetchSuggestionImage(
       suggestion_id, base::Bind(&ContentSuggestionsServiceTest::OnImageFetched,
                                 base::Unretained(this)));
 }
 
 TEST_F(ContentSuggestionsServiceTest,
        ShouldCallbackEmptyImageForUnavailableProvider) {
   // Setup the current thread's MessageLoop.
   base::MessageLoop message_loop;
 
   base::RunLoop run_loop;
-  std::string suggestion_id = "TestID";
+  // Assuming there will never be a category with the randomly-picked id below.

[Marc Treib, 2016/09/23 13:31:29]

https://xkcd.com/221/ :)
(Not an actual complaint)

[jkrcal, 2016/09/23 16:11:48]

:)

+  std::string suggestion_id = "21563|TestID";
   EXPECT_CALL(*this, OnImageFetched(Property(&gfx::Image::IsEmpty, Eq(true))))
       .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
   service()->FetchSuggestionImage(
       suggestion_id, base::Bind(&ContentSuggestionsServiceTest::OnImageFetched,
                                 base::Unretained(this)));
   run_loop.Run();
 }
 
 TEST_F(ContentSuggestionsServiceTest, ShouldRedirectDismissSuggestion) {
   Category articles_category = FromKnownCategory(KnownCategories::ARTICLES);
   Category offline_pages_category =
       FromKnownCategory(KnownCategories::DOWNLOADS);
   MockProvider* provider1 = RegisterProvider(articles_category);
   MockProvider* provider2 = RegisterProvider(offline_pages_category);
 
-  provider2->FireSuggestionsChanged(offline_pages_category, {11});
-  std::string suggestion_id = CreateSuggestion(11).id();
+  provider2->FireSuggestionsChanged(
+      offline_pages_category, CreateSuggestions(offline_pages_category, {11}));
+  std::string suggestion_id = CreateSuggestion(offline_pages_category, 11).id();
 
   EXPECT_CALL(*provider1, DismissSuggestion(_)).Times(0);
   EXPECT_CALL(*provider2, DismissSuggestion(suggestion_id));
   service()->DismissSuggestion(suggestion_id);
 }
 
 TEST_F(ContentSuggestionsServiceTest, ShouldRedirectSuggestionInvalidated) {
   Category articles_category = FromKnownCategory(KnownCategories::ARTICLES);
 
   MockProvider* provider = RegisterProvider(articles_category);
   MockServiceObserver observer;
   service()->AddObserver(&observer);
 
-  provider->FireSuggestionsChanged(articles_category, {11, 12, 13});
+  provider->FireSuggestionsChanged(
+      articles_category, CreateSuggestions(articles_category, {11, 12, 13}));
   ExpectThatSuggestionsAre(articles_category, {11, 12, 13});
 
-  std::string suggestion_id = CreateSuggestion(12).id();
+  std::string suggestion_id = CreateSuggestion(articles_category, 12).id();
   EXPECT_CALL(observer,
               OnSuggestionInvalidated(articles_category, suggestion_id));
   provider->FireSuggestionInvalidated(articles_category, suggestion_id);
   ExpectThatSuggestionsAre(articles_category, {11, 13});
   Mock::VerifyAndClearExpectations(&observer);
 
   // Unknown IDs must be forwarded (though no change happens to the service's
   // internal data structures) because previously opened UIs, which can still
   // show the invalidated suggestion, must be notified.
-  std::string unknown_id = CreateSuggestion(1234).id();
+  std::string unknown_id = CreateSuggestion(articles_category, 1234).id();
   EXPECT_CALL(observer, OnSuggestionInvalidated(articles_category, unknown_id));
   provider->FireSuggestionInvalidated(articles_category, unknown_id);
   ExpectThatSuggestionsAre(articles_category, {11, 13});
   Mock::VerifyAndClearExpectations(&observer);
 
   service()->RemoveObserver(&observer);
 }
 
 TEST_F(ContentSuggestionsServiceTest, ShouldForwardSuggestions) {
   Category articles_category = FromKnownCategory(KnownCategories::ARTICLES);
   Category offline_pages_category =
       FromKnownCategory(KnownCategories::DOWNLOADS);
 
   // Create and register providers
   MockProvider* provider1 = RegisterProvider(articles_category);
   provider1->FireCategoryStatusChangedWithCurrentStatus(articles_category);
   MockProvider* provider2 = RegisterProvider(offline_pages_category);
   provider2->FireCategoryStatusChangedWithCurrentStatus(offline_pages_category);
   ASSERT_THAT(providers().count(articles_category), Eq(1ul));
   EXPECT_THAT(providers().at(articles_category), Eq(provider1));
   ASSERT_THAT(providers().count(offline_pages_category), Eq(1ul));
   EXPECT_THAT(providers().at(offline_pages_category), Eq(provider2));
 
   // Create and register observer
   MockServiceObserver observer;
   service()->AddObserver(&observer);
 
   // Send suggestions 1 and 2
   EXPECT_CALL(observer, OnNewSuggestions(articles_category));
-  provider1->FireSuggestionsChanged(articles_category, {1, 2});
+  provider1->FireSuggestionsChanged(
+      articles_category, CreateSuggestions(articles_category, {1, 2}));
   ExpectThatSuggestionsAre(articles_category, {1, 2});
   Mock::VerifyAndClearExpectations(&observer);
 
   // Send them again, make sure they're not reported twice
   EXPECT_CALL(observer, OnNewSuggestions(articles_category));
-  provider1->FireSuggestionsChanged(articles_category, {1, 2});
+  provider1->FireSuggestionsChanged(
+      articles_category, CreateSuggestions(articles_category, {1, 2}));
   ExpectThatSuggestionsAre(articles_category, {1, 2});
   ExpectThatSuggestionsAre(offline_pages_category, std::vector<int>());
   Mock::VerifyAndClearExpectations(&observer);
 
   // Send suggestions 13 and 14
   EXPECT_CALL(observer, OnNewSuggestions(offline_pages_category));
-  provider2->FireSuggestionsChanged(offline_pages_category, {13, 14});
+  provider2->FireSuggestionsChanged(
+      offline_pages_category, CreateSuggestions(articles_category, {13, 14}));
   ExpectThatSuggestionsAre(articles_category, {1, 2});
   ExpectThatSuggestionsAre(offline_pages_category, {13, 14});
   Mock::VerifyAndClearExpectations(&observer);
 
   // Send suggestion 1 only
   EXPECT_CALL(observer, OnNewSuggestions(articles_category));
-  provider1->FireSuggestionsChanged(articles_category, {1});
+  provider1->FireSuggestionsChanged(articles_category,
+                                    CreateSuggestions(articles_category, {1}));
   ExpectThatSuggestionsAre(articles_category, {1});
   ExpectThatSuggestionsAre(offline_pages_category, {13, 14});
   Mock::VerifyAndClearExpectations(&observer);
 
   // provider2 reports BOOKMARKS as unavailable
   EXPECT_CALL(observer, OnCategoryStatusChanged(
                             offline_pages_category,
                             CategoryStatus::CATEGORY_EXPLICITLY_DISABLED));
   provider2->FireCategoryStatusChanged(
       offline_pages_category, CategoryStatus::CATEGORY_EXPLICITLY_DISABLED);
@@ skipping 43 matching lines @@
   // Provider starts providing |new_category| without calling
   // |OnCategoryStatusChanged|. This is supported for now until further
   // reconsideration.
   Category new_category = FromKnownCategory(KnownCategories::ARTICLES);
   provider->SetProvidedCategories(
       std::vector<Category>({category, new_category}));
 
   EXPECT_CALL(observer, OnNewSuggestions(new_category));
   EXPECT_CALL(observer,
               OnCategoryStatusChanged(new_category, CategoryStatus::AVAILABLE));
-  provider->FireSuggestionsChanged(new_category, {1, 2});
+  provider->FireSuggestionsChanged(new_category,
+                                   CreateSuggestions(new_category, {1, 2}));
 
   ExpectThatSuggestionsAre(new_category, {1, 2});
   ASSERT_THAT(providers().count(category), Eq(1ul));
   EXPECT_THAT(providers().at(category), Eq(provider));
   EXPECT_THAT(service()->GetCategoryStatus(category),
               Eq(CategoryStatus::AVAILABLE));
   ASSERT_THAT(providers().count(new_category), Eq(1ul));
   EXPECT_THAT(providers().at(new_category), Eq(provider));
   EXPECT_THAT(service()->GetCategoryStatus(new_category),
               Eq(CategoryStatus::AVAILABLE));
@@ skipping 29 matching lines @@
 
   service()->RemoveObserver(&observer);
 }
 
 TEST_F(ContentSuggestionsServiceTest, ShouldRemoveCategoryWhenNotProvided) {
   Category category = FromKnownCategory(KnownCategories::DOWNLOADS);
   MockProvider* provider = RegisterProvider(category);
   MockServiceObserver observer;
   service()->AddObserver(&observer);
 
-  provider->FireSuggestionsChanged(category, {1, 2});
+  provider->FireSuggestionsChanged(category,
+                                   CreateSuggestions(category, {1, 2}));
   ExpectThatSuggestionsAre(category, {1, 2});
 
   EXPECT_CALL(observer,
               OnCategoryStatusChanged(category, CategoryStatus::NOT_PROVIDED));
   provider->FireCategoryStatusChanged(category, CategoryStatus::NOT_PROVIDED);
 
   EXPECT_THAT(service()->GetCategoryStatus(category),
               Eq(CategoryStatus::NOT_PROVIDED));
   EXPECT_TRUE(service()->GetCategories().empty());
   ExpectThatSuggestionsAre(category, std::vector<int>());
 
   service()->RemoveObserver(&observer);
 }
 
 // This tests the temporary special-casing of the bookmarks section: If it is
 // empty, it should appear at the end; see crbug.com/640568.
 TEST_F(ContentSuggestionsServiceTest, ShouldPutBookmarksAtEndIfEmpty) {
   // Register a bookmarks provider and an arbitrary remote provider.
   Category bookmarks = FromKnownCategory(KnownCategories::BOOKMARKS);
   MockProvider* bookmarks_provider = RegisterProvider(bookmarks);
   bookmarks_provider->FireCategoryStatusChangedWithCurrentStatus(bookmarks);
   Category remote = FromRemoteCategory(123);
   MockProvider* remote_provider = RegisterProvider(remote);
   remote_provider->FireCategoryStatusChangedWithCurrentStatus(remote);
 
   // By default, the bookmarks category is empty, so it should be at the end.
   EXPECT_THAT(service()->GetCategories(), ElementsAre(remote, bookmarks));
 
   // Add two bookmark suggestions; now bookmarks should be in the front.
-  bookmarks_provider->FireSuggestionsChanged(bookmarks, {1, 2});
+  bookmarks_provider->FireSuggestionsChanged(
+      bookmarks, CreateSuggestions(bookmarks, {1, 2}));
   EXPECT_THAT(service()->GetCategories(), ElementsAre(bookmarks, remote));
   // Dismiss the first suggestion; bookmarks should stay in the front.
-  service()->DismissSuggestion(CreateSuggestion(1).id());
+  service()->DismissSuggestion(CreateSuggestion(bookmarks, 1).id());
   EXPECT_THAT(service()->GetCategories(), ElementsAre(bookmarks, remote));
   // Dismiss the second suggestion; now bookmarks should go back to the end.
-  service()->DismissSuggestion(CreateSuggestion(2).id());
+  service()->DismissSuggestion(CreateSuggestion(bookmarks, 2).id());
   EXPECT_THAT(service()->GetCategories(), ElementsAre(remote, bookmarks));
 
   // Same thing, but invalidate instead of dismissing.
-  bookmarks_provider->FireSuggestionsChanged(bookmarks, {1, 2});
+  bookmarks_provider->FireSuggestionsChanged(
+      bookmarks, CreateSuggestions(bookmarks, {1, 2}));
   EXPECT_THAT(service()->GetCategories(), ElementsAre(bookmarks, remote));
-  bookmarks_provider->FireSuggestionInvalidated(bookmarks,
-                                                CreateSuggestion(1).id());
+  bookmarks_provider->FireSuggestionInvalidated(
+      bookmarks, CreateSuggestion(bookmarks, 1).id());
   EXPECT_THAT(service()->GetCategories(), ElementsAre(bookmarks, remote));
-  bookmarks_provider->FireSuggestionInvalidated(bookmarks,
-                                                CreateSuggestion(2).id());
+  bookmarks_provider->FireSuggestionInvalidated(
+      bookmarks, CreateSuggestion(bookmarks, 2).id());
   EXPECT_THAT(service()->GetCategories(), ElementsAre(remote, bookmarks));
 
   // Same thing, but now the bookmarks category updates "naturally".
-  bookmarks_provider->FireSuggestionsChanged(bookmarks, {1, 2});
+  bookmarks_provider->FireSuggestionsChanged(
+      bookmarks, CreateSuggestions(bookmarks, {1, 2}));
   EXPECT_THAT(service()->GetCategories(), ElementsAre(bookmarks, remote));
-  bookmarks_provider->FireSuggestionsChanged(bookmarks, {1});
+  bookmarks_provider->FireSuggestionsChanged(bookmarks,
+                                             CreateSuggestions(bookmarks, {1}));
   EXPECT_THAT(service()->GetCategories(), ElementsAre(bookmarks, remote));
-  bookmarks_provider->FireSuggestionsChanged(bookmarks, std::vector<int>());
+  bookmarks_provider->FireSuggestionsChanged(
+      bookmarks, CreateSuggestions(bookmarks, std::vector<int>()));
   EXPECT_THAT(service()->GetCategories(), ElementsAre(remote, bookmarks));
 }
 
 TEST_F(ContentSuggestionsServiceTest, ShouldForwardClearHistory) {
   Category category = FromKnownCategory(KnownCategories::DOWNLOADS);
   MockProvider* provider = RegisterProvider(category);
   base::Time begin = base::Time::FromTimeT(123),
              end = base::Time::FromTimeT(456);
   EXPECT_CALL(*provider, ClearHistory(begin, end, _));
   base::Callback<bool(const GURL& url)> filter;
   service()->ClearHistory(begin, end, filter);
 }
 
 }  // namespace ntp_snippets