From e88d16809c5f7a0d5e8d59caa451dc4359fa7b06 Mon Sep 17 00:00:00 2001 From: sfiera Date: Tue, 30 Aug 2016 11:16:13 -0700 Subject: [PATCH] Overhaul ntp_snippets_service_unittest.cc. A partial list of things done here: * Move tests that verified NTPSnippet::best_source() behavior into ntp_snippet_test.cc; they don't belong here. * Switch it to use the chromecontentsuggestions-pa. Testing new features like server-provided categories is not possible without this. * Replace MockContentSuggestionsProviderObserver with a fake. It shouldn't have been a mock, since we weren't using it to test behavior, only state. * In one case, test using what the observer sees, instead of peeking in through debugging hooks. * Get rid of special test base class and Mock::VerifyAndClear calls; I consider these anti-patterns. * Initialize the service better; give it an empty list of snippets for its first fetch, and wait for it to complete the fetch before returning (I think this is what was most strongly implicated by the failures with the other CL). * Create the service and expect the scheduling directly from each case, so they are more readable individually. Review-Url: https://codereview.chromium.org/2285133004 Cr-Commit-Position: refs/heads/master@{#415358} --- .../ntp_snippets/ntp_snippet_unittest.cc | 226 ++++- .../ntp_snippets_service_unittest.cc | 930 ++++++++---------- .../ntp_snippets_status_service_unittest.cc | 38 +- .../ntp_snippets/ntp_snippets_test_utils.cc | 27 +- .../ntp_snippets/ntp_snippets_test_utils.h | 22 +- 5 files changed, 672 insertions(+), 571 deletions(-) diff --git a/components/ntp_snippets/ntp_snippet_unittest.cc b/components/ntp_snippets/ntp_snippet_unittest.cc index 93304b63f9649c..825a6f29b1619e 100644 --- a/components/ntp_snippets/ntp_snippet_unittest.cc +++ b/components/ntp_snippets/ntp_snippet_unittest.cc @@ -12,6 +12,16 @@ namespace ntp_snippets { namespace { +std::unique_ptr SnippetFromContentSuggestionJSON( + const std::string& json) { + auto json_value = base::JSONReader::Read(json); + base::DictionaryValue* json_dict; + if (!json_value->GetAsDictionary(&json_dict)) { + return nullptr; + } + return NTPSnippet::CreateFromContentSuggestionsDictionary(*json_dict); +} + TEST(NTPSnippetTest, FromChromeContentSuggestionsDictionary) { const std::string kJsonStr = "{" @@ -26,11 +36,7 @@ TEST(NTPSnippetTest, FromChromeContentSuggestionsDictionary) { " \"ampUrl\" : \"http://localhost/amp\"," " \"faviconUrl\" : \"http://localhost/favicon.ico\" " "}"; - auto json_value = base::JSONReader::Read(kJsonStr); - base::DictionaryValue* json_dict; - ASSERT_TRUE(json_value->GetAsDictionary(&json_dict)); - - auto snippet = NTPSnippet::CreateFromContentSuggestionsDictionary(*json_dict); + auto snippet = SnippetFromContentSuggestionJSON(kJsonStr); ASSERT_THAT(snippet, testing::NotNull()); EXPECT_EQ(snippet->id(), "http://localhost/foobar"); @@ -47,5 +53,215 @@ TEST(NTPSnippetTest, FromChromeContentSuggestionsDictionary) { EXPECT_EQ(snippet->best_source().amp_url, GURL("http://localhost/amp")); } +std::unique_ptr SnippetFromChromeReaderDict( + std::unique_ptr dict) { + if (!dict) { + return nullptr; + } + return NTPSnippet::CreateFromChromeReaderDictionary(*dict); +} + +std::unique_ptr SnippetWithTwoSources() { + const std::string kJsonStr = + "{\n" + " \"contentInfo\": {\n" + " \"url\": \"http://url.com\",\n" + " \"title\": \"Source 1 Title\",\n" + " \"snippet\": \"Source 1 Snippet\",\n" + " \"thumbnailUrl\": \"http://url.com/thumbnail\",\n" + " \"creationTimestampSec\": 1234567890,\n" + " \"expiryTimestampSec\": 2345678901,\n" + " \"sourceCorpusInfo\": [{\n" + " \"corpusId\": \"http://source1.com\",\n" + " \"publisherData\": {\n" + " \"sourceName\": \"Source 1\"\n" + " },\n" + " \"ampUrl\": \"http://source1.amp.com\"\n" + " }, {\n" + " \"corpusId\": \"http://source2.com\",\n" + " \"publisherData\": {\n" + " \"sourceName\": \"Source 2\"\n" + " },\n" + " \"ampUrl\": \"http://source2.amp.com\"\n" + " }]\n" + " },\n" + " \"score\": 5.0\n" + "}\n"; + + auto json_value = base::JSONReader::Read(kJsonStr); + base::DictionaryValue* json_dict; + if (!json_value->GetAsDictionary(&json_dict)) { + return nullptr; + } + return json_dict->CreateDeepCopy(); +} + +TEST(NTPSnippetTest, TestMultipleSources) { + auto snippet = SnippetFromChromeReaderDict(SnippetWithTwoSources()); + ASSERT_THAT(snippet, testing::NotNull()); + + // Expect the first source to be chosen. + EXPECT_EQ(snippet->sources().size(), 2u); + EXPECT_EQ(snippet->id(), "http://url.com"); + EXPECT_EQ(snippet->best_source().url, GURL("http://source1.com")); + EXPECT_EQ(snippet->best_source().publisher_name, std::string("Source 1")); + EXPECT_EQ(snippet->best_source().amp_url, GURL("http://source1.amp.com")); +} + +TEST(NTPSnippetTest, TestMultipleIncompleteSources1) { + // Set Source 2 to have no AMP url, and Source 1 to have no publisher name + // Source 2 should win since we favor publisher name over amp url + auto dict = SnippetWithTwoSources(); + base::ListValue* sources; + ASSERT_TRUE(dict->GetList("contentInfo.sourceCorpusInfo", &sources)); + base::DictionaryValue* source; + ASSERT_TRUE(sources->GetDictionary(0, &source)); + source->Remove("publisherData.sourceName", nullptr); + ASSERT_TRUE(sources->GetDictionary(1, &source)); + source->Remove("ampUrl", nullptr); + + auto snippet = SnippetFromChromeReaderDict(std::move(dict)); + ASSERT_THAT(snippet, testing::NotNull()); + + EXPECT_EQ(snippet->sources().size(), 2u); + EXPECT_EQ(snippet->id(), "http://url.com"); + EXPECT_EQ(snippet->best_source().url, GURL("http://source2.com")); + EXPECT_EQ(snippet->best_source().publisher_name, std::string("Source 2")); + EXPECT_EQ(snippet->best_source().amp_url, GURL()); +} + +TEST(NTPSnippetTest, TestMultipleIncompleteSources2) { + // Set Source 1 to have no AMP url, and Source 2 to have no publisher name + // Source 1 should win in this case since we prefer publisher name to AMP url + auto dict = SnippetWithTwoSources(); + base::ListValue* sources; + ASSERT_TRUE(dict->GetList("contentInfo.sourceCorpusInfo", &sources)); + base::DictionaryValue* source; + ASSERT_TRUE(sources->GetDictionary(0, &source)); + source->Remove("ampUrl", nullptr); + ASSERT_TRUE(sources->GetDictionary(1, &source)); + source->Remove("publisherData.sourceName", nullptr); + + auto snippet = SnippetFromChromeReaderDict(std::move(dict)); + ASSERT_THAT(snippet, testing::NotNull()); + + EXPECT_EQ(snippet->sources().size(), 2u); + EXPECT_EQ(snippet->id(), "http://url.com"); + EXPECT_EQ(snippet->best_source().url, GURL("http://source1.com")); + EXPECT_EQ(snippet->best_source().publisher_name, std::string("Source 1")); + EXPECT_EQ(snippet->best_source().amp_url, GURL()); +} + +TEST(NTPSnippetTest, TestMultipleIncompleteSources3) { + // Set source 1 to have no AMP url and no source, and source 2 to only have + // amp url. There should be no snippets since we only add sources we consider + // complete + auto dict = SnippetWithTwoSources(); + base::ListValue* sources; + ASSERT_TRUE(dict->GetList("contentInfo.sourceCorpusInfo", &sources)); + base::DictionaryValue* source; + ASSERT_TRUE(sources->GetDictionary(0, &source)); + source->Remove("publisherData.sourceName", nullptr); + source->Remove("ampUrl", nullptr); + ASSERT_TRUE(sources->GetDictionary(1, &source)); + source->Remove("publisherData.sourceName", nullptr); + + auto snippet = SnippetFromChromeReaderDict(std::move(dict)); + ASSERT_THAT(snippet, testing::NotNull()); + ASSERT_FALSE(snippet->is_complete()); +} + +std::unique_ptr SnippetWithThreeSources() { + const std::string kJsonStr = + "{\n" + " \"contentInfo\": {\n" + " \"url\": \"http://url.com\",\n" + " \"title\": \"Source 1 Title\",\n" + " \"snippet\": \"Source 1 Snippet\",\n" + " \"thumbnailUrl\": \"http://url.com/thumbnail\",\n" + " \"creationTimestampSec\": 1234567890,\n" + " \"expiryTimestampSec\": 2345678901,\n" + " \"sourceCorpusInfo\": [{\n" + " \"corpusId\": \"http://source1.com\",\n" + " \"publisherData\": {\n" + " \"sourceName\": \"Source 1\"\n" + " },\n" + " \"ampUrl\": \"http://source1.amp.com\"\n" + " }, {\n" + " \"corpusId\": \"http://source2.com\",\n" + " \"publisherData\": {\n" + " \"sourceName\": \"Source 2\"\n" + " },\n" + " \"ampUrl\": \"http://source2.amp.com\"\n" + " }, {\n" + " \"corpusId\": \"http://source3.com\",\n" + " \"publisherData\": {\n" + " \"sourceName\": \"Source 3\"\n" + " },\n" + " \"ampUrl\": \"http://source3.amp.com\"\n" + " }]\n" + " },\n" + " \"score\": 5.0\n" + "}\n"; + + auto json_value = base::JSONReader::Read(kJsonStr); + base::DictionaryValue* json_dict; + if (!json_value->GetAsDictionary(&json_dict)) { + return nullptr; + } + return json_dict->CreateDeepCopy(); +} + +TEST(NTPSnippetTest, TestMultipleCompleteSources1) { + // Test 2 complete sources, we should choose the first complete source + auto dict = SnippetWithThreeSources(); + base::ListValue* sources; + ASSERT_TRUE(dict->GetList("contentInfo.sourceCorpusInfo", &sources)); + base::DictionaryValue* source; + ASSERT_TRUE(sources->GetDictionary(1, &source)); + source->Remove("publisherData.sourceName", nullptr); + + auto snippet = SnippetFromChromeReaderDict(std::move(dict)); + ASSERT_THAT(snippet, testing::NotNull()); + + EXPECT_EQ(snippet->sources().size(), 3u); + EXPECT_EQ(snippet->id(), "http://url.com"); + EXPECT_EQ(snippet->best_source().url, GURL("http://source1.com")); + EXPECT_EQ(snippet->best_source().publisher_name, std::string("Source 1")); + EXPECT_EQ(snippet->best_source().amp_url, GURL("http://source1.amp.com")); +} + +TEST(NTPSnippetTest, TestMultipleCompleteSources2) { + // Test 2 complete sources, we should choose the first complete source + auto dict = SnippetWithThreeSources(); + base::ListValue* sources; + ASSERT_TRUE(dict->GetList("contentInfo.sourceCorpusInfo", &sources)); + base::DictionaryValue* source; + ASSERT_TRUE(sources->GetDictionary(0, &source)); + source->Remove("publisherData.sourceName", nullptr); + + auto snippet = SnippetFromChromeReaderDict(std::move(dict)); + ASSERT_THAT(snippet, testing::NotNull()); + + EXPECT_EQ(snippet->sources().size(), 3u); + EXPECT_EQ(snippet->id(), "http://url.com"); + EXPECT_EQ(snippet->best_source().url, GURL("http://source2.com")); + EXPECT_EQ(snippet->best_source().publisher_name, std::string("Source 2")); + EXPECT_EQ(snippet->best_source().amp_url, GURL("http://source2.amp.com")); +} + +TEST(NTPSnippetTest, TestMultipleCompleteSources3) { + // Test 3 complete sources, we should choose the first complete source + auto dict = SnippetWithThreeSources(); + auto snippet = SnippetFromChromeReaderDict(std::move(dict)); + ASSERT_THAT(snippet, testing::NotNull()); + + EXPECT_EQ(snippet->sources().size(), 3u); + EXPECT_EQ(snippet->id(), "http://url.com"); + EXPECT_EQ(snippet->best_source().url, GURL("http://source1.com")); + EXPECT_EQ(snippet->best_source().publisher_name, std::string("Source 1")); + EXPECT_EQ(snippet->best_source().amp_url, GURL("http://source1.amp.com")); +} + } // namespace } // namespace ntp_snippets diff --git a/components/ntp_snippets/ntp_snippets_service_unittest.cc b/components/ntp_snippets/ntp_snippets_service_unittest.cc index 840b94483e972e..fd7814d955ff09 100644 --- a/components/ntp_snippets/ntp_snippets_service_unittest.cc +++ b/components/ntp_snippets/ntp_snippets_service_unittest.cc @@ -26,8 +26,8 @@ #include "components/image_fetcher/image_fetcher.h" #include "components/image_fetcher/image_fetcher_delegate.h" #include "components/ntp_snippets/category_factory.h" -#include "components/ntp_snippets/mock_content_suggestions_provider_observer.h" #include "components/ntp_snippets/ntp_snippet.h" +#include "components/ntp_snippets/ntp_snippets_constants.h" #include "components/ntp_snippets/ntp_snippets_database.h" #include "components/ntp_snippets/ntp_snippets_fetcher.h" #include "components/ntp_snippets/ntp_snippets_scheduler.h" @@ -36,6 +36,7 @@ #include "components/prefs/testing_pref_service.h" #include "components/signin/core/browser/fake_profile_oauth2_token_service.h" #include "components/signin/core/browser/fake_signin_manager.h" +#include "components/variations/variations_associated_data.h" #include "google_apis/google_api_keys.h" #include "net/url_request/test_url_fetcher_factory.h" #include "net/url_request/url_request_test_util.h" @@ -48,12 +49,17 @@ using image_fetcher::ImageFetcher; using image_fetcher::ImageFetcherDelegate; using testing::ElementsAre; using testing::Eq; +using testing::InSequence; using testing::Invoke; using testing::IsEmpty; using testing::Mock; +using testing::MockFunction; +using testing::NiceMock; using testing::Return; +using testing::SaveArg; using testing::SizeIs; using testing::StartsWith; +using testing::WithArgs; using testing::_; namespace ntp_snippets { @@ -65,8 +71,12 @@ MATCHER_P(IdEq, value, "") { } const base::Time::Exploded kDefaultCreationTime = {2015, 11, 4, 25, 13, 46, 45}; -const char kTestContentSnippetsServerFormat[] = - "https://chromereader-pa.googleapis.com/v1/fetch?key=%s"; +const char kTestContentSuggestionsServerEndpoint[] = + "https://localunittest-chromecontentsuggestions-pa.googleapis.com/v1/" + "suggestions/fetch"; +const char kTestContentSuggestionsServerFormat[] = + "https://localunittest-chromecontentsuggestions-pa.googleapis.com/v1/" + "suggestions/fetch?key=%s"; const char kSnippetUrl[] = "http://localhost/foobar"; const char kSnippetTitle[] = "Title"; @@ -74,7 +84,6 @@ const char kSnippetText[] = "Snippet"; const char kSnippetSalientImage[] = "http://localhost/salient_image"; const char kSnippetPublisherName[] = "Foo News"; const char kSnippetAmpUrl[] = "http://localhost/amp"; -const float kSnippetScore = 5.0; const char kSnippetUrl2[] = "http://foo.com/bar"; @@ -89,96 +98,85 @@ base::Time GetDefaultExpirationTime() { } std::string GetTestJson(const std::vector& snippets) { - return base::StringPrintf("{\"recos\":[%s]}", - base::JoinString(snippets, ", ").c_str()); -} - -std::string GetSnippetWithUrlAndTimesAndSources( - const std::string& url, - const std::string& content_creation_time_str, - const std::string& expiry_time_str, - const std::vector& source_urls, - const std::vector& publishers, - const std::vector& amp_urls) { - char json_str_format[] = - "{ \"contentInfo\": {" - "\"url\" : \"%s\"," - "\"title\" : \"%s\"," - "\"snippet\" : \"%s\"," - "\"thumbnailUrl\" : \"%s\"," - "\"creationTimestampSec\" : \"%s\"," - "\"expiryTimestampSec\" : \"%s\"," - "\"sourceCorpusInfo\" : [%s]" - "}, " - "\"score\" : %f}"; - - char source_corpus_info_format[] = - "{\"corpusId\": \"%s\"," - "\"publisherData\": {" - "\"sourceName\": \"%s\"" - "}," - "\"ampUrl\": \"%s\"}"; - - std::string source_corpus_info_list_str; - for (size_t i = 0; i < source_urls.size(); ++i) { - std::string source_corpus_info_str = - base::StringPrintf(source_corpus_info_format, - source_urls[i].empty() ? "" : source_urls[i].c_str(), - publishers[i].empty() ? "" : publishers[i].c_str(), - amp_urls[i].empty() ? "" : amp_urls[i].c_str()); - source_corpus_info_list_str.append(source_corpus_info_str); - source_corpus_info_list_str.append(","); - } - // Remove the last comma - source_corpus_info_list_str.erase(source_corpus_info_list_str.size()-1, 1); - return base::StringPrintf(json_str_format, url.c_str(), kSnippetTitle, - kSnippetText, kSnippetSalientImage, - content_creation_time_str.c_str(), - expiry_time_str.c_str(), - source_corpus_info_list_str.c_str(), kSnippetScore); -} - -std::string GetSnippetWithSources(const std::vector& source_urls, - const std::vector& publishers, - const std::vector& amp_urls) { - return GetSnippetWithUrlAndTimesAndSources( - kSnippetUrl, NTPSnippet::TimeToJsonString(GetDefaultCreationTime()), - NTPSnippet::TimeToJsonString(GetDefaultExpirationTime()), source_urls, - publishers, amp_urls); -} - -std::string GetSnippetWithUrlAndTimes( + return base::StringPrintf( + "{\n" + " \"categories\": [{\n" + " \"id\": 1,\n" + " \"localizedTitle\": \"Articles for You\",\n" + " \"suggestions\": [%s]\n" + " }]\n" + "}\n", + base::JoinString(snippets, ", ").c_str()); +} + +std::string FormatTime(const base::Time& t) { + base::Time::Exploded x; + t.UTCExplode(&x); + return base::StringPrintf("%04d-%02d-%02dT%02d:%02d:%02dZ", x.year, x.month, + x.day_of_month, x.hour, x.minute, x.second); +} + +std::string GetSnippetWithUrlAndTimesAndSource( + const std::vector& ids, const std::string& url, - const std::string& content_creation_time_str, - const std::string& expiry_time_str) { - return GetSnippetWithUrlAndTimesAndSources( - url, content_creation_time_str, expiry_time_str, - {std::string(url)}, {std::string(kSnippetPublisherName)}, - {std::string(kSnippetAmpUrl)}); -} - -std::string GetSnippetWithTimes(const std::string& content_creation_time_str, - const std::string& expiry_time_str) { - return GetSnippetWithUrlAndTimes(kSnippetUrl, content_creation_time_str, - expiry_time_str); + const base::Time& creation_time, + const base::Time& expiry_time, + const std::string& publisher, + const std::string& amp_url) { + const std::string ids_string = base::JoinString(ids, "\",\n \""); + return base::StringPrintf( + "{\n" + " \"ids\": [\n" + " \"%s\"\n" + " ],\n" + " \"title\": \"%s\",\n" + " \"snippet\": \"%s\",\n" + " \"fullPageUrl\": \"%s\",\n" + " \"creationTime\": \"%s\",\n" + " \"expirationTime\": \"%s\",\n" + " \"attribution\": \"%s\",\n" + " \"imageUrl\": \"%s\",\n" + " \"ampUrl\": \"%s\"\n" + " }", + ids_string.c_str(), kSnippetTitle, kSnippetText, url.c_str(), + FormatTime(creation_time).c_str(), FormatTime(expiry_time).c_str(), + publisher.c_str(), kSnippetSalientImage, amp_url.c_str()); +} + +std::string GetSnippetWithSources(const std::string& source_url, + const std::string& publisher, + const std::string& amp_url) { + return GetSnippetWithUrlAndTimesAndSource( + {kSnippetUrl}, source_url, GetDefaultCreationTime(), + GetDefaultExpirationTime(), publisher, amp_url); +} + +std::string GetSnippetWithUrlAndTimes(const std::string& url, + const base::Time& content_creation_time, + const base::Time& expiry_time) { + return GetSnippetWithUrlAndTimesAndSource({url}, url, content_creation_time, + expiry_time, kSnippetPublisherName, + kSnippetAmpUrl); +} + +std::string GetSnippetWithTimes(const base::Time& content_creation_time, + const base::Time& expiry_time) { + return GetSnippetWithUrlAndTimes(kSnippetUrl, content_creation_time, + expiry_time); } std::string GetSnippetWithUrl(const std::string& url) { - return GetSnippetWithUrlAndTimes( - url, NTPSnippet::TimeToJsonString(GetDefaultCreationTime()), - NTPSnippet::TimeToJsonString(GetDefaultExpirationTime())); + return GetSnippetWithUrlAndTimes(url, GetDefaultCreationTime(), + GetDefaultExpirationTime()); } std::string GetSnippet() { - return GetSnippetWithUrlAndTimes( - kSnippetUrl, NTPSnippet::TimeToJsonString(GetDefaultCreationTime()), - NTPSnippet::TimeToJsonString(GetDefaultExpirationTime())); + return GetSnippetWithUrlAndTimes(kSnippetUrl, GetDefaultCreationTime(), + GetDefaultExpirationTime()); } std::string GetExpiredSnippet() { - return GetSnippetWithTimes( - NTPSnippet::TimeToJsonString(GetDefaultCreationTime()), - NTPSnippet::TimeToJsonString(base::Time::Now())); + return GetSnippetWithTimes(GetDefaultCreationTime(), base::Time::Now()); } std::string GetInvalidSnippet() { @@ -191,7 +189,7 @@ std::string GetIncompleteSnippet() { std::string json_str = GetSnippet(); // Rename the "url" entry. The result is syntactically valid json that will // fail to parse as snippets. - size_t pos = json_str.find("\"url\""); + size_t pos = json_str.find("\"fullPageUrl\""); if (pos == std::string::npos) { NOTREACHED(); return std::string(); @@ -242,33 +240,6 @@ class MockScheduler : public NTPSnippetsScheduler { MOCK_METHOD0(Unschedule, bool()); }; -class WaitForDBLoad { - public: - WaitForDBLoad(MockContentSuggestionsProviderObserver* observer, - NTPSnippetsService* service) - : observer_(observer) { - EXPECT_CALL(*observer_, OnCategoryStatusChanged(_, _, _)) - .WillOnce(Invoke(this, &WaitForDBLoad::OnCategoryStatusChanged)); - if (!service->ready()) - run_loop_.Run(); - } - - ~WaitForDBLoad() { Mock::VerifyAndClearExpectations(observer_); } - - private: - void OnCategoryStatusChanged(ContentSuggestionsProvider* provider, - Category category, - CategoryStatus new_status) { - EXPECT_EQ(new_status, CategoryStatus::AVAILABLE_LOADING); - run_loop_.Quit(); - } - - MockContentSuggestionsProviderObserver* observer_; - base::RunLoop run_loop_; - - DISALLOW_COPY_AND_ASSIGN(WaitForDBLoad); -}; - class MockImageFetcher : public ImageFetcher { public: MOCK_METHOD1(SetImageFetcherDelegate, void(ImageFetcherDelegate*)); @@ -280,29 +251,76 @@ class MockImageFetcher : public ImageFetcher { base::Callback)); }; -class MockDismissedSuggestionsCallback - : public ContentSuggestionsProvider::DismissedSuggestionsCallback { +class FakeContentSuggestionsProviderObserver + : public ContentSuggestionsProvider::Observer { public: - MOCK_METHOD2(MockRun, - void(Category category, - std::vector* dismissed_suggestions)); - void Run(Category category, - std::vector dismissed_suggestions) { - MockRun(category, &dismissed_suggestions); + FakeContentSuggestionsProviderObserver() + : loaded_(base::WaitableEvent::ResetPolicy::MANUAL, + base::WaitableEvent::InitialState::NOT_SIGNALED) {} + + void OnNewSuggestions(ContentSuggestionsProvider* provider, + Category category, + std::vector suggestions) override { + suggestions_[category] = std::move(suggestions); + } + + void OnCategoryStatusChanged(ContentSuggestionsProvider* provider, + Category category, + CategoryStatus new_status) override { + loaded_.Signal(); + statuses_[category] = new_status; + } + + void OnSuggestionInvalidated(ContentSuggestionsProvider* provider, + Category category, + const std::string& suggestion_id) override {} + + CategoryStatus StatusForCategory(Category category) const { + auto it = statuses_.find(category); + if (it == statuses_.end()) { + return CategoryStatus::NOT_PROVIDED; + } + return it->second; } + + const std::vector& SuggestionsForCategory( + Category category) { + return suggestions_[category]; + } + + void WaitForLoad() { loaded_.Wait(); } + bool Loaded() { return loaded_.IsSignaled(); } + + void Reset() { + loaded_.Reset(); + statuses_.clear(); + } + + private: + base::WaitableEvent loaded_; + std::map statuses_; + std::map, Category::CompareByID> + suggestions_; + + DISALLOW_COPY_AND_ASSIGN(FakeContentSuggestionsProviderObserver); }; } // namespace -class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase { +class NTPSnippetsServiceTest : public ::testing::Test { public: NTPSnippetsServiceTest() - : fake_url_fetcher_factory_( + : params_manager_(ntp_snippets::kStudyName, + {{"content_suggestions_backend", + kTestContentSuggestionsServerEndpoint}}), + fake_url_fetcher_factory_( /*default_factory=*/&failing_url_fetcher_factory_), - test_url_(base::StringPrintf(kTestContentSnippetsServerFormat, - google_apis::GetAPIKey().c_str())) { - NTPSnippetsService::RegisterProfilePrefs(pref_service()->registry()); - RequestThrottler::RegisterProfilePrefs(pref_service()->registry()); + test_url_(base::StringPrintf(kTestContentSuggestionsServerFormat, + google_apis::GetAPIKey().c_str())), + + observer_(base::MakeUnique()) { + NTPSnippetsService::RegisterProfilePrefs(utils_.pref_service()->registry()); + RequestThrottler::RegisterProfilePrefs(utils_.pref_service()->registry()); // Since no SuggestionsService is injected in tests, we need to force the // service to fetch from all hosts. @@ -315,67 +333,59 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase { // We need to run the message loop after deleting the database, because // ProtoDatabaseImpl deletes the actual LevelDB asynchronously on the task // runner. Without this, we'd get reports of memory leaks. - Mock::VerifyAndClear(&mock_scheduler()); - service_.reset(); base::RunLoop().RunUntilIdle(); } - void SetUp() override { - test::NTPSnippetsTestBase::SetUp(); - EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1); - CreateSnippetsService(); - } - - void RecreateSnippetsService() { - Mock::VerifyAndClear(&mock_scheduler()); - service_.reset(); - base::RunLoop().RunUntilIdle(); - EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1); - CreateSnippetsService(); - } - - void CreateSnippetsService() { - DCHECK(!service_); + std::unique_ptr MakeSnippetsService() { + CHECK(!observer_->Loaded()); scoped_refptr task_runner( base::ThreadTaskRunnerHandle::Get()); scoped_refptr request_context_getter = new net::TestURLRequestContextGetter(task_runner.get()); - ResetSigninManager(); + utils_.ResetSigninManager(); std::unique_ptr snippets_fetcher = base::MakeUnique( - fake_signin_manager(), fake_token_service_.get(), - std::move(request_context_getter), pref_service(), + utils_.fake_signin_manager(), fake_token_service_.get(), + std::move(request_context_getter), utils_.pref_service(), &category_factory_, base::Bind(&ParseJson), /*is_stable_channel=*/true); - fake_signin_manager()->SignIn("foo@bar.com"); + utils_.fake_signin_manager()->SignIn("foo@bar.com"); snippets_fetcher->SetPersonalizationForTesting( NTPSnippetsFetcher::Personalization::kNonPersonal); - auto image_fetcher = - base::MakeUnique>(); + auto image_fetcher = base::MakeUnique>(); image_fetcher_ = image_fetcher.get(); // Add an initial fetch response, as the service tries to fetch when there // is nothing in the DB. - SetUpFetchResponse(GetTestJson({GetSnippet()})); + SetUpFetchResponse(GetTestJson(std::vector())); - service_.reset(new NTPSnippetsService( - &observer_, &category_factory_, pref_service(), nullptr, nullptr, "fr", - &scheduler_, std::move(snippets_fetcher), + auto service = base::MakeUnique( + observer_.get(), &category_factory_, utils_.pref_service(), nullptr, + nullptr, "fr", &scheduler_, std::move(snippets_fetcher), std::move(image_fetcher), /*image_decoder=*/nullptr, base::MakeUnique(database_dir_.path(), task_runner), - base::MakeUnique(fake_signin_manager(), - pref_service()))); + base::MakeUnique(utils_.fake_signin_manager(), + utils_.pref_service())); - WaitForDBLoad(&observer_, service_.get()); + base::RunLoop().RunUntilIdle(); + observer_->WaitForLoad(); + return service; } - std::string MakeUniqueID(const std::string& within_category_id) { - return service()->MakeUniqueID(articles_category(), within_category_id); + void ResetSnippetsService(std::unique_ptr* service) { + service->reset(); + observer_ = base::MakeUnique(); + *service = MakeSnippetsService(); + } + + std::string MakeUniqueID(const NTPSnippetsService& service, + const std::string& within_category_id) { + return service.MakeUniqueID(articles_category(), within_category_id); } Category articles_category() { @@ -384,12 +394,9 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase { protected: const GURL& test_url() { return test_url_; } - NTPSnippetsService* service() { return service_.get(); } - MockContentSuggestionsProviderObserver& observer() { return observer_; } + FakeContentSuggestionsProviderObserver& observer() { return *observer_; } MockScheduler& mock_scheduler() { return scheduler_; } - testing::NiceMock* image_fetcher() { - return image_fetcher_; - } + NiceMock* image_fetcher() { return image_fetcher_; } // Provide the json to be returned by the fake fetcher. void SetUpFetchResponse(const std::string& json) { @@ -397,25 +404,26 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase { net::URLRequestStatus::SUCCESS); } - void LoadFromJSONString(const std::string& json) { + void LoadFromJSONString(NTPSnippetsService* service, + const std::string& json) { SetUpFetchResponse(json); - service()->FetchSnippets(true); + service->FetchSnippets(true); base::RunLoop().RunUntilIdle(); } private: + variations::testing::VariationParamsManager params_manager_; + test::NTPSnippetsTestUtils utils_; base::MessageLoop message_loop_; FailingFakeURLFetcherFactory failing_url_fetcher_factory_; // Instantiation of factory automatically sets itself as URLFetcher's factory. net::FakeURLFetcherFactory fake_url_fetcher_factory_; const GURL test_url_; std::unique_ptr fake_token_service_; - MockScheduler scheduler_; - MockContentSuggestionsProviderObserver observer_; + NiceMock scheduler_; + std::unique_ptr observer_; CategoryFactory category_factory_; - testing::NiceMock* image_fetcher_; - // Last so that the dependencies are deleted after the service. - std::unique_ptr service_; + NiceMock* image_fetcher_; base::ScopedTempDir database_dir_; @@ -423,52 +431,68 @@ class NTPSnippetsServiceTest : public test::NTPSnippetsTestBase { }; TEST_F(NTPSnippetsServiceTest, ScheduleOnStart) { - // SetUp() checks that Schedule is called. + EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)); + auto service = MakeSnippetsService(); // When we have no snippets are all, loading the service initiates a fetch. base::RunLoop().RunUntilIdle(); - ASSERT_EQ("OK", service()->snippets_fetcher()->last_status()); + ASSERT_EQ("OK", service->snippets_fetcher()->last_status()); } TEST_F(NTPSnippetsServiceTest, Full) { std::string json_str(GetTestJson({GetSnippet()})); - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); + EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)); + auto service = MakeSnippetsService(); - EXPECT_EQ(snippet.id(), kSnippetUrl); - EXPECT_EQ(snippet.title(), kSnippetTitle); - EXPECT_EQ(snippet.snippet(), kSnippetText); - EXPECT_EQ(snippet.salient_image_url(), GURL(kSnippetSalientImage)); - EXPECT_EQ(GetDefaultCreationTime(), snippet.publish_date()); - EXPECT_EQ(snippet.best_source().publisher_name, kSnippetPublisherName); - EXPECT_EQ(snippet.best_source().amp_url, GURL(kSnippetAmpUrl)); + LoadFromJSONString(service.get(), json_str); + ASSERT_THAT(observer().SuggestionsForCategory(articles_category()), + SizeIs(1)); + ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); + const ContentSuggestion& suggestion = + observer().SuggestionsForCategory(articles_category()).front(); + + EXPECT_EQ(MakeUniqueID(*service, kSnippetUrl), suggestion.id()); + EXPECT_EQ(kSnippetTitle, base::UTF16ToUTF8(suggestion.title())); + EXPECT_EQ(kSnippetText, base::UTF16ToUTF8(suggestion.snippet_text())); + // EXPECT_EQ(GURL(kSnippetSalientImage), suggestion.salient_image_url()); + EXPECT_EQ(GetDefaultCreationTime(), suggestion.publish_date()); + EXPECT_EQ(kSnippetPublisherName, + base::UTF16ToUTF8(suggestion.publisher_name())); + EXPECT_EQ(GURL(kSnippetAmpUrl), suggestion.amp_url()); } TEST_F(NTPSnippetsServiceTest, Clear) { + EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)); + auto service = MakeSnippetsService(); + std::string json_str(GetTestJson({GetSnippet()})); - LoadFromJSONString(json_str); - EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); + LoadFromJSONString(service.get(), json_str); + EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); - service()->ClearCachedSuggestions(articles_category()); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + service->ClearCachedSuggestions(articles_category()); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); } TEST_F(NTPSnippetsServiceTest, InsertAtFront) { + EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)); + auto service = MakeSnippetsService(); + std::string first("http://first"); - LoadFromJSONString(GetTestJson({GetSnippetWithUrl(first)})); - EXPECT_THAT(service()->GetSnippetsForTesting(), ElementsAre(IdEq(first))); + LoadFromJSONString(service.get(), GetTestJson({GetSnippetWithUrl(first)})); + EXPECT_THAT(service->GetSnippetsForTesting(), ElementsAre(IdEq(first))); std::string second("http://second"); - LoadFromJSONString(GetTestJson({GetSnippetWithUrl(second)})); + LoadFromJSONString(service.get(), GetTestJson({GetSnippetWithUrl(second)})); // The snippet loaded last should be at the first position in the list now. - EXPECT_THAT(service()->GetSnippetsForTesting(), + EXPECT_THAT(service->GetSnippetsForTesting(), ElementsAre(IdEq(second), IdEq(first))); } TEST_F(NTPSnippetsServiceTest, LimitNumSnippets) { + auto service = MakeSnippetsService(); + int max_snippet_count = NTPSnippetsService::GetMaxSnippetCountForTesting(); int snippets_per_load = max_snippet_count / 2 + 1; char url_format[] = "http://localhost/%i"; @@ -481,154 +505,158 @@ TEST_F(NTPSnippetsServiceTest, LimitNumSnippets) { base::StringPrintf(url_format, snippets_per_load + i))); } - LoadFromJSONString(GetTestJson(snippets1)); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(snippets1.size())); + LoadFromJSONString(service.get(), GetTestJson(snippets1)); + ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(snippets1.size())); - LoadFromJSONString(GetTestJson(snippets2)); - EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(max_snippet_count)); + LoadFromJSONString(service.get(), GetTestJson(snippets2)); + EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(max_snippet_count)); } TEST_F(NTPSnippetsServiceTest, LoadInvalidJson) { - LoadFromJSONString(GetTestJson({GetInvalidSnippet()})); - EXPECT_THAT(service()->snippets_fetcher()->last_status(), + auto service = MakeSnippetsService(); + + LoadFromJSONString(service.get(), GetTestJson({GetInvalidSnippet()})); + EXPECT_THAT(service->snippets_fetcher()->last_status(), StartsWith("Received invalid JSON")); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); } TEST_F(NTPSnippetsServiceTest, LoadInvalidJsonWithExistingSnippets) { - LoadFromJSONString(GetTestJson({GetSnippet()})); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - ASSERT_EQ("OK", service()->snippets_fetcher()->last_status()); + auto service = MakeSnippetsService(); - LoadFromJSONString(GetTestJson({GetInvalidSnippet()})); - EXPECT_THAT(service()->snippets_fetcher()->last_status(), + LoadFromJSONString(service.get(), GetTestJson({GetSnippet()})); + ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); + ASSERT_EQ("OK", service->snippets_fetcher()->last_status()); + + LoadFromJSONString(service.get(), GetTestJson({GetInvalidSnippet()})); + EXPECT_THAT(service->snippets_fetcher()->last_status(), StartsWith("Received invalid JSON")); // This should not have changed the existing snippets. - EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); + EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); } TEST_F(NTPSnippetsServiceTest, LoadIncompleteJson) { - LoadFromJSONString(GetTestJson({GetIncompleteSnippet()})); + auto service = MakeSnippetsService(); + + LoadFromJSONString(service.get(), GetTestJson({GetIncompleteSnippet()})); EXPECT_EQ("Invalid / empty list.", - service()->snippets_fetcher()->last_status()); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + service->snippets_fetcher()->last_status()); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); } TEST_F(NTPSnippetsServiceTest, LoadIncompleteJsonWithExistingSnippets) { - LoadFromJSONString(GetTestJson({GetSnippet()})); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); + auto service = MakeSnippetsService(); - LoadFromJSONString(GetTestJson({GetIncompleteSnippet()})); + LoadFromJSONString(service.get(), GetTestJson({GetSnippet()})); + ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); + + LoadFromJSONString(service.get(), GetTestJson({GetIncompleteSnippet()})); EXPECT_EQ("Invalid / empty list.", - service()->snippets_fetcher()->last_status()); + service->snippets_fetcher()->last_status()); // This should not have changed the existing snippets. - EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); + EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); } TEST_F(NTPSnippetsServiceTest, Dismiss) { - std::vector source_urls, publishers, amp_urls; - source_urls.push_back(std::string("http://site.com")); - publishers.push_back(std::string("Source 1")); - amp_urls.push_back(std::string()); + EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(2); + auto service = MakeSnippetsService(); + std::string json_str( - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)})); + GetTestJson({GetSnippetWithSources("http://site.com", "Source 1", "")})); - LoadFromJSONString(json_str); + LoadFromJSONString(service.get(), json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); + ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); // Dismissing a non-existent snippet shouldn't do anything. - service()->DismissSuggestion(MakeUniqueID("http://othersite.com")); - EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); + service->DismissSuggestion(MakeUniqueID(*service, "http://othersite.com")); + EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); // Dismiss the snippet. - service()->DismissSuggestion(MakeUniqueID(kSnippetUrl)); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + service->DismissSuggestion(MakeUniqueID(*service, kSnippetUrl)); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); // Make sure that fetching the same snippet again does not re-add it. - LoadFromJSONString(json_str); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + LoadFromJSONString(service.get(), json_str); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); // The snippet should stay dismissed even after re-creating the service. - RecreateSnippetsService(); - LoadFromJSONString(json_str); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + ResetSnippetsService(&service); + LoadFromJSONString(service.get(), json_str); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); // The snippet can be added again after clearing dismissed snippets. - service()->ClearDismissedSuggestionsForDebugging(articles_category()); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); - LoadFromJSONString(json_str); - EXPECT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); + service->ClearDismissedSuggestionsForDebugging(articles_category()); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); + LoadFromJSONString(service.get(), json_str); + EXPECT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); } TEST_F(NTPSnippetsServiceTest, GetDismissed) { - LoadFromJSONString(GetTestJson({GetSnippet()})); + auto service = MakeSnippetsService(); - service()->DismissSuggestion(MakeUniqueID(kSnippetUrl)); + LoadFromJSONString(service.get(), GetTestJson({GetSnippet()})); - MockDismissedSuggestionsCallback callback; + service->DismissSuggestion(MakeUniqueID(*service, kSnippetUrl)); - EXPECT_CALL(callback, MockRun(_, _)) - .WillOnce( - Invoke([this](Category category, - std::vector* dismissed_suggestions) { - EXPECT_EQ(1u, dismissed_suggestions->size()); - for (auto& suggestion : *dismissed_suggestions) { - EXPECT_EQ(MakeUniqueID(kSnippetUrl), suggestion.id()); - } - })); - service()->GetDismissedSuggestionsForDebugging( + service->GetDismissedSuggestionsForDebugging( articles_category(), - base::Bind(&MockDismissedSuggestionsCallback::Run, - base::Unretained(&callback), articles_category())); - Mock::VerifyAndClearExpectations(&callback); + base::Bind( + [](NTPSnippetsService* service, NTPSnippetsServiceTest* test, + std::vector dismissed_suggestions) { + EXPECT_EQ(1u, dismissed_suggestions.size()); + for (auto& suggestion : dismissed_suggestions) { + EXPECT_EQ(test->MakeUniqueID(*service, kSnippetUrl), + suggestion.id()); + } + }, + service.get(), this)); + base::RunLoop().RunUntilIdle(); // There should be no dismissed snippet after clearing the list. - EXPECT_CALL(callback, MockRun(_, _)) - .WillOnce( - Invoke([this](Category category, - std::vector* dismissed_suggestions) { - EXPECT_EQ(0u, dismissed_suggestions->size()); - })); - service()->ClearDismissedSuggestionsForDebugging(articles_category()); - service()->GetDismissedSuggestionsForDebugging( + service->ClearDismissedSuggestionsForDebugging(articles_category()); + service->GetDismissedSuggestionsForDebugging( articles_category(), - base::Bind(&MockDismissedSuggestionsCallback::Run, - base::Unretained(&callback), articles_category())); + base::Bind( + [](NTPSnippetsService* service, NTPSnippetsServiceTest* test, + std::vector dismissed_suggestions) { + EXPECT_EQ(0u, dismissed_suggestions.size()); + }, + service.get(), this)); + base::RunLoop().RunUntilIdle(); } TEST_F(NTPSnippetsServiceTest, CreationTimestampParseFail) { - std::string json_str(GetTestJson({GetSnippetWithTimes( - "aaa1448459205", - NTPSnippet::TimeToJsonString(GetDefaultExpirationTime()))})); + auto service = MakeSnippetsService(); - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); - EXPECT_EQ(snippet.id(), kSnippetUrl); - EXPECT_EQ(snippet.title(), kSnippetTitle); - EXPECT_EQ(snippet.snippet(), kSnippetText); - EXPECT_EQ(base::Time::UnixEpoch(), snippet.publish_date()); + std::string json = + GetSnippetWithTimes(GetDefaultCreationTime(), GetDefaultExpirationTime()); + base::ReplaceFirstSubstringAfterOffset( + &json, 0, FormatTime(GetDefaultCreationTime()), "aaa1448459205"); + std::string json_str(GetTestJson({json})); + + LoadFromJSONString(service.get(), json_str); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); } TEST_F(NTPSnippetsServiceTest, RemoveExpiredContent) { + auto service = MakeSnippetsService(); + std::string json_str(GetTestJson({GetExpiredSnippet()})); - LoadFromJSONString(json_str); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + LoadFromJSONString(service.get(), json_str); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); } TEST_F(NTPSnippetsServiceTest, TestSingleSource) { - std::vector source_urls, publishers, amp_urls; - source_urls.push_back(std::string("http://source1.com")); - publishers.push_back(std::string("Source 1")); - amp_urls.push_back(std::string("http://source1.amp.com")); - std::string json_str( - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)})); + auto service = MakeSnippetsService(); + + std::string json_str(GetTestJson({GetSnippetWithSources( + "http://source1.com", "Source 1", "http://source1.amp.com")})); - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); + LoadFromJSONString(service.get(), json_str); + ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); + const NTPSnippet& snippet = *service->GetSnippetsForTesting().front(); EXPECT_EQ(snippet.sources().size(), 1u); EXPECT_EQ(snippet.id(), kSnippetUrl); EXPECT_EQ(snippet.best_source().url, GURL("http://source1.com")); @@ -637,229 +665,56 @@ TEST_F(NTPSnippetsServiceTest, TestSingleSource) { } TEST_F(NTPSnippetsServiceTest, TestSingleSourceWithMalformedUrl) { - std::vector source_urls, publishers, amp_urls; - source_urls.push_back(std::string("aaaa")); - publishers.push_back(std::string("Source 1")); - amp_urls.push_back(std::string("http://source1.amp.com")); - std::string json_str( - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)})); + auto service = MakeSnippetsService(); - LoadFromJSONString(json_str); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); -} + std::string json_str(GetTestJson({GetSnippetWithSources( + "ceci n'est pas un url", "Source 1", "http://source1.amp.com")})); -TEST_F(NTPSnippetsServiceTest, TestSingleSourceWithMissingData) { - std::vector source_urls, publishers, amp_urls; - source_urls.push_back(std::string("http://source1.com")); - publishers.push_back(std::string()); - amp_urls.push_back(std::string()); - std::string json_str( - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)})); - - LoadFromJSONString(json_str); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + LoadFromJSONString(service.get(), json_str); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); } -TEST_F(NTPSnippetsServiceTest, TestMultipleSources) { - std::vector source_urls, publishers, amp_urls; - source_urls.push_back(std::string("http://source1.com")); - source_urls.push_back(std::string("http://source2.com")); - publishers.push_back(std::string("Source 1")); - publishers.push_back(std::string("Source 2")); - amp_urls.push_back(std::string("http://source1.amp.com")); - amp_urls.push_back(std::string("http://source2.amp.com")); - std::string json_str( - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)})); - - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); - // Expect the first source to be chosen - EXPECT_EQ(snippet.sources().size(), 2u); - EXPECT_EQ(snippet.id(), kSnippetUrl); - EXPECT_EQ(snippet.best_source().url, GURL("http://source1.com")); - EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 1")); - EXPECT_EQ(snippet.best_source().amp_url, GURL("http://source1.amp.com")); -} - -TEST_F(NTPSnippetsServiceTest, TestMultipleIncompleteSources) { - // Set Source 2 to have no AMP url, and Source 1 to have no publisher name - // Source 2 should win since we favor publisher name over amp url - std::vector source_urls, publishers, amp_urls; - source_urls.push_back(std::string("http://source1.com")); - source_urls.push_back(std::string("http://source2.com")); - publishers.push_back(std::string()); - publishers.push_back(std::string("Source 2")); - amp_urls.push_back(std::string("http://source1.amp.com")); - amp_urls.push_back(std::string()); - std::string json_str( - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)})); - - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - { - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); - EXPECT_EQ(snippet.sources().size(), 2u); - EXPECT_EQ(snippet.id(), kSnippetUrl); - EXPECT_EQ(snippet.best_source().url, GURL("http://source2.com")); - EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 2")); - EXPECT_EQ(snippet.best_source().amp_url, GURL()); - } - - service()->ClearCachedSuggestions(articles_category()); - // Set Source 1 to have no AMP url, and Source 2 to have no publisher name - // Source 1 should win in this case since we prefer publisher name to AMP url - source_urls.clear(); - source_urls.push_back(std::string("http://source1.com")); - source_urls.push_back(std::string("http://source2.com")); - publishers.clear(); - publishers.push_back(std::string("Source 1")); - publishers.push_back(std::string()); - amp_urls.clear(); - amp_urls.push_back(std::string()); - amp_urls.push_back(std::string("http://source2.amp.com")); - json_str = - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}); - - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - { - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); - EXPECT_EQ(snippet.sources().size(), 2u); - EXPECT_EQ(snippet.id(), kSnippetUrl); - EXPECT_EQ(snippet.best_source().url, GURL("http://source1.com")); - EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 1")); - EXPECT_EQ(snippet.best_source().amp_url, GURL()); - } +TEST_F(NTPSnippetsServiceTest, TestSingleSourceWithMissingData) { + auto service = MakeSnippetsService(); - service()->ClearCachedSuggestions(articles_category()); - // Set source 1 to have no AMP url and no source, and source 2 to only have - // amp url. There should be no snippets since we only add sources we consider - // complete - source_urls.clear(); - source_urls.push_back(std::string("http://source1.com")); - source_urls.push_back(std::string("http://source2.com")); - publishers.clear(); - publishers.push_back(std::string()); - publishers.push_back(std::string()); - amp_urls.clear(); - amp_urls.push_back(std::string()); - amp_urls.push_back(std::string("http://source2.amp.com")); - json_str = - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}); - - LoadFromJSONString(json_str); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); -} - -TEST_F(NTPSnippetsServiceTest, TestMultipleCompleteSources) { - // Test 2 complete sources, we should choose the first complete source - std::vector source_urls, publishers, amp_urls; - source_urls.push_back(std::string("http://source1.com")); - source_urls.push_back(std::string("http://source2.com")); - source_urls.push_back(std::string("http://source3.com")); - publishers.push_back(std::string("Source 1")); - publishers.push_back(std::string()); - publishers.push_back(std::string("Source 3")); - amp_urls.push_back(std::string("http://source1.amp.com")); - amp_urls.push_back(std::string("http://source2.amp.com")); - amp_urls.push_back(std::string("http://source3.amp.com")); std::string json_str( - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)})); + GetTestJson({GetSnippetWithSources("http://source1.com", "", "")})); - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - { - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); - EXPECT_EQ(snippet.sources().size(), 3u); - EXPECT_EQ(snippet.id(), kSnippetUrl); - EXPECT_EQ(snippet.best_source().url, GURL("http://source1.com")); - EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 1")); - EXPECT_EQ(snippet.best_source().amp_url, GURL("http://source1.amp.com")); - } - - // Test 2 complete sources, we should choose the first complete source - service()->ClearCachedSuggestions(articles_category()); - source_urls.clear(); - source_urls.push_back(std::string("http://source1.com")); - source_urls.push_back(std::string("http://source2.com")); - source_urls.push_back(std::string("http://source3.com")); - publishers.clear(); - publishers.push_back(std::string()); - publishers.push_back(std::string("Source 2")); - publishers.push_back(std::string("Source 3")); - amp_urls.clear(); - amp_urls.push_back(std::string("http://source1.amp.com")); - amp_urls.push_back(std::string("http://source2.amp.com")); - amp_urls.push_back(std::string("http://source3.amp.com")); - json_str = - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}); - - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - { - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); - EXPECT_EQ(snippet.sources().size(), 3u); - EXPECT_EQ(snippet.id(), kSnippetUrl); - EXPECT_EQ(snippet.best_source().url, GURL("http://source2.com")); - EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 2")); - EXPECT_EQ(snippet.best_source().amp_url, GURL("http://source2.amp.com")); - } - - // Test 3 complete sources, we should choose the first complete source - service()->ClearCachedSuggestions(articles_category()); - source_urls.clear(); - source_urls.push_back(std::string("http://source1.com")); - source_urls.push_back(std::string("http://source2.com")); - source_urls.push_back(std::string("http://source3.com")); - publishers.clear(); - publishers.push_back(std::string("Source 1")); - publishers.push_back(std::string("Source 2")); - publishers.push_back(std::string("Source 3")); - amp_urls.clear(); - amp_urls.push_back(std::string()); - amp_urls.push_back(std::string("http://source2.amp.com")); - amp_urls.push_back(std::string("http://source3.amp.com")); - json_str = - GetTestJson({GetSnippetWithSources(source_urls, publishers, amp_urls)}); - - LoadFromJSONString(json_str); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); - { - const NTPSnippet& snippet = *service()->GetSnippetsForTesting().front(); - EXPECT_EQ(snippet.sources().size(), 3u); - EXPECT_EQ(snippet.id(), kSnippetUrl); - EXPECT_EQ(snippet.best_source().url, GURL("http://source2.com")); - EXPECT_EQ(snippet.best_source().publisher_name, std::string("Source 2")); - EXPECT_EQ(snippet.best_source().amp_url, GURL("http://source2.amp.com")); - } + LoadFromJSONString(service.get(), json_str); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); } TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) { + auto service = MakeSnippetsService(); + base::HistogramTester tester; - LoadFromJSONString(GetTestJson({GetInvalidSnippet()})); + LoadFromJSONString(service.get(), GetTestJson({GetInvalidSnippet()})); EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"), ElementsAre(base::Bucket(/*min=*/0, /*count=*/1))); + // Invalid JSON shouldn't contribute to NumArticlesFetched. EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticlesFetched"), IsEmpty()); + // Valid JSON with empty list. - LoadFromJSONString(GetTestJson(std::vector())); + LoadFromJSONString(service.get(), GetTestJson(std::vector())); EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"), ElementsAre(base::Bucket(/*min=*/0, /*count=*/2))); EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticlesFetched"), ElementsAre(base::Bucket(/*min=*/0, /*count=*/1))); + // Snippet list should be populated with size 1. - LoadFromJSONString(GetTestJson({GetSnippet()})); + LoadFromJSONString(service.get(), GetTestJson({GetSnippet()})); EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"), ElementsAre(base::Bucket(/*min=*/0, /*count=*/2), base::Bucket(/*min=*/1, /*count=*/1))); EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticlesFetched"), ElementsAre(base::Bucket(/*min=*/0, /*count=*/1), base::Bucket(/*min=*/1, /*count=*/1))); + // Duplicate snippet shouldn't increase the list size. - LoadFromJSONString(GetTestJson({GetSnippet()})); + LoadFromJSONString(service.get(), GetTestJson({GetSnippet()})); EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"), ElementsAre(base::Bucket(/*min=*/0, /*count=*/2), base::Bucket(/*min=*/1, /*count=*/2))); @@ -869,10 +724,11 @@ TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) { EXPECT_THAT( tester.GetAllSamples("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded"), IsEmpty()); + // Dismissing a snippet should decrease the list size. This will only be // logged after the next fetch. - service()->DismissSuggestion(MakeUniqueID(kSnippetUrl)); - LoadFromJSONString(GetTestJson({GetSnippet()})); + service->DismissSuggestion(MakeUniqueID(*service, kSnippetUrl)); + LoadFromJSONString(service.get(), GetTestJson({GetSnippet()})); EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"), ElementsAre(base::Bucket(/*min=*/0, /*count=*/3), base::Bucket(/*min=*/1, /*count=*/2))); @@ -883,77 +739,109 @@ TEST_F(NTPSnippetsServiceTest, LogNumArticlesHistogram) { EXPECT_THAT( tester.GetAllSamples("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded"), ElementsAre(base::Bucket(/*min=*/1, /*count=*/1))); - // Recreating the service and loading from prefs shouldn't count as fetched - // articles. - RecreateSnippetsService(); + + // There is only a single, dismissed snippet in the database, so recreating + // the service will require us to re-fetch. tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 4); + ResetSnippetsService(&service); + EXPECT_EQ(observer().StatusForCategory(articles_category()), + CategoryStatus::AVAILABLE); + tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 5); + EXPECT_THAT( + tester.GetAllSamples("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded"), + ElementsAre(base::Bucket(/*min=*/1, /*count=*/2))); + + // But if there's a non-dismissed snippet in the database, recreating it + // shouldn't trigger a fetch. + LoadFromJSONString( + service.get(), + GetTestJson({GetSnippetWithUrl("http://not-dismissed.com")})); + tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 6); + ResetSnippetsService(&service); + tester.ExpectTotalCount("NewTabPage.Snippets.NumArticlesFetched", 6); } TEST_F(NTPSnippetsServiceTest, DismissShouldRespectAllKnownUrls) { - const std::string creation = - NTPSnippet::TimeToJsonString(GetDefaultCreationTime()); - const std::string expiry = - NTPSnippet::TimeToJsonString(GetDefaultExpirationTime()); + auto service = MakeSnippetsService(); + + const base::Time creation = GetDefaultCreationTime(); + const base::Time expiry = GetDefaultExpirationTime(); const std::vector source_urls = { "http://mashable.com/2016/05/11/stolen", - "http://www.aol.com/article/2016/05/stolen-doggie", - "http://mashable.com/2016/05/11/stolen?utm_cid=1"}; - const std::vector publishers = {"Mashable", "AOL", "Mashable"}; + "http://www.aol.com/article/2016/05/stolen-doggie"}; + const std::vector publishers = {"Mashable", "AOL"}; const std::vector amp_urls = { "http://mashable-amphtml.googleusercontent.com/1", - "http://t2.gstatic.com/images?q=tbn:3", "http://t2.gstatic.com/images?q=tbn:3"}; // Add the snippet from the mashable domain. - LoadFromJSONString(GetTestJson({GetSnippetWithUrlAndTimesAndSources( - source_urls[0], creation, expiry, source_urls, publishers, amp_urls)})); - ASSERT_THAT(service()->GetSnippetsForTesting(), SizeIs(1)); + LoadFromJSONString(service.get(), + GetTestJson({GetSnippetWithUrlAndTimesAndSource( + source_urls, source_urls[0], creation, expiry, + publishers[0], amp_urls[0])})); + ASSERT_THAT(service->GetSnippetsForTesting(), SizeIs(1)); // Dismiss the snippet via the mashable source corpus ID. - service()->DismissSuggestion(MakeUniqueID(source_urls[0])); - EXPECT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + service->DismissSuggestion(MakeUniqueID(*service, source_urls[0])); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); // The same article from the AOL domain should now be detected as dismissed. - LoadFromJSONString(GetTestJson({GetSnippetWithUrlAndTimesAndSources( - source_urls[1], creation, expiry, source_urls, publishers, amp_urls)})); - ASSERT_THAT(service()->GetSnippetsForTesting(), IsEmpty()); + LoadFromJSONString(service.get(), + GetTestJson({GetSnippetWithUrlAndTimesAndSource( + source_urls, source_urls[1], creation, expiry, + publishers[1], amp_urls[1])})); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); } TEST_F(NTPSnippetsServiceTest, StatusChanges) { + { + InSequence s; + EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)); + EXPECT_CALL(mock_scheduler(), Unschedule()); + EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)); + } + auto service = MakeSnippetsService(); + // Simulate user signed out SetUpFetchResponse(GetTestJson({GetSnippet()})); - EXPECT_CALL(observer(), - OnCategoryStatusChanged(_, _, CategoryStatus::SIGNED_OUT)); - service()->OnDisabledReasonChanged(DisabledReason::SIGNED_OUT); + service->OnDisabledReasonChanged(DisabledReason::SIGNED_OUT); + base::RunLoop().RunUntilIdle(); - EXPECT_EQ(NTPSnippetsService::State::DISABLED, service()->state_); - EXPECT_THAT(service()->GetSnippetsForTesting(), + EXPECT_THAT(observer().StatusForCategory(articles_category()), + Eq(CategoryStatus::SIGNED_OUT)); + EXPECT_THAT(NTPSnippetsService::State::DISABLED, Eq(service->state_)); + EXPECT_THAT(service->GetSnippetsForTesting(), IsEmpty()); // No fetch should be made. // Simulate user sign in. The service should be ready again and load snippets. SetUpFetchResponse(GetTestJson({GetSnippet()})); - EXPECT_CALL(observer(), - OnCategoryStatusChanged(_, _, CategoryStatus::AVAILABLE_LOADING)); - EXPECT_CALL(mock_scheduler(), Schedule(_, _, _, _)).Times(1); - service()->OnDisabledReasonChanged(DisabledReason::NONE); - EXPECT_CALL(observer(), - OnCategoryStatusChanged(_, _, CategoryStatus::AVAILABLE)); + service->OnDisabledReasonChanged(DisabledReason::NONE); + EXPECT_THAT(observer().StatusForCategory(articles_category()), + Eq(CategoryStatus::AVAILABLE_LOADING)); + base::RunLoop().RunUntilIdle(); - EXPECT_EQ(NTPSnippetsService::State::READY, service()->state_); - EXPECT_FALSE(service()->GetSnippetsForTesting().empty()); + EXPECT_THAT(observer().StatusForCategory(articles_category()), + Eq(CategoryStatus::AVAILABLE)); + EXPECT_THAT(NTPSnippetsService::State::READY, Eq(service->state_)); + EXPECT_FALSE(service->GetSnippetsForTesting().empty()); } TEST_F(NTPSnippetsServiceTest, ImageReturnedWithTheSameId) { - LoadFromJSONString(GetTestJson({GetSnippet()})); + auto service = MakeSnippetsService(); + + LoadFromJSONString(service.get(), GetTestJson({GetSnippet()})); gfx::Image image; - EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _)) - .WillOnce(testing::WithArgs<0, 2>(Invoke(ServeOneByOneImage))); - testing::MockFunction image_fetched; - EXPECT_CALL(image_fetched, Call(_)).WillOnce(testing::SaveArg<0>(&image)); - - service()->FetchSuggestionImage( - MakeUniqueID(kSnippetUrl), - base::Bind(&testing::MockFunction::Call, + MockFunction image_fetched; + { + InSequence s; + EXPECT_CALL(*image_fetcher(), StartOrQueueNetworkRequest(_, _, _)) + .WillOnce(WithArgs<0, 2>(Invoke(ServeOneByOneImage))); + EXPECT_CALL(image_fetched, Call(_)).WillOnce(SaveArg<0>(&image)); + } + + service->FetchSuggestionImage( + MakeUniqueID(*service, kSnippetUrl), + base::Bind(&MockFunction::Call, base::Unretained(&image_fetched))); base::RunLoop().RunUntilIdle(); // Check that the image by ServeOneByOneImage is really served. @@ -961,14 +849,16 @@ TEST_F(NTPSnippetsServiceTest, ImageReturnedWithTheSameId) { } TEST_F(NTPSnippetsServiceTest, EmptyImageReturnedForNonExistentId) { + auto service = MakeSnippetsService(); + // Create a non-empty image so that we can test the image gets updated. gfx::Image image = gfx::test::CreateImage(1, 1); - testing::MockFunction image_fetched; - EXPECT_CALL(image_fetched, Call(_)).WillOnce(testing::SaveArg<0>(&image)); + MockFunction image_fetched; + EXPECT_CALL(image_fetched, Call(_)).WillOnce(SaveArg<0>(&image)); - service()->FetchSuggestionImage( - MakeUniqueID(kSnippetUrl2), - base::Bind(&testing::MockFunction::Call, + service->FetchSuggestionImage( + MakeUniqueID(*service, kSnippetUrl2), + base::Bind(&MockFunction::Call, base::Unretained(&image_fetched))); base::RunLoop().RunUntilIdle(); diff --git a/components/ntp_snippets/ntp_snippets_status_service_unittest.cc b/components/ntp_snippets/ntp_snippets_status_service_unittest.cc index 6b3c8cfa7605ff..f3bf30b1fb0bdf 100644 --- a/components/ntp_snippets/ntp_snippets_status_service_unittest.cc +++ b/components/ntp_snippets/ntp_snippets_status_service_unittest.cc @@ -21,48 +21,48 @@ using testing::Return; namespace ntp_snippets { -class NTPSnippetsStatusServiceTest : public test::NTPSnippetsTestBase { +class NTPSnippetsStatusServiceTest : public ::testing::Test { public: NTPSnippetsStatusServiceTest() { - NTPSnippetsStatusService::RegisterProfilePrefs(pref_service()->registry()); + NTPSnippetsStatusService::RegisterProfilePrefs( + utils_.pref_service()->registry()); } - void SetUp() override { - test::NTPSnippetsTestBase::SetUp(); - - service_.reset( - new NTPSnippetsStatusService(fake_signin_manager(), pref_service())); + std::unique_ptr MakeService() { + return base::MakeUnique( + utils_.fake_signin_manager(), utils_.pref_service()); } protected: - NTPSnippetsStatusService* service() { return service_.get(); } - - private: - std::unique_ptr service_; + test::NTPSnippetsTestUtils utils_; }; TEST_F(NTPSnippetsStatusServiceTest, SigninStateCompatibility) { + auto service = MakeService(); + // The default test setup is signed out. - EXPECT_EQ(DisabledReason::SIGNED_OUT, service()->GetDisabledReasonFromDeps()); + EXPECT_EQ(DisabledReason::SIGNED_OUT, service->GetDisabledReasonFromDeps()); // Once signed in, we should be in a compatible state. - fake_signin_manager()->SignIn("foo@bar.com"); - EXPECT_EQ(DisabledReason::NONE, service()->GetDisabledReasonFromDeps()); + utils_.fake_signin_manager()->SignIn("foo@bar.com"); + EXPECT_EQ(DisabledReason::NONE, service->GetDisabledReasonFromDeps()); } TEST_F(NTPSnippetsStatusServiceTest, DisabledViaPref) { + auto service = MakeService(); + // The default test setup is signed out. - ASSERT_EQ(DisabledReason::SIGNED_OUT, service()->GetDisabledReasonFromDeps()); + ASSERT_EQ(DisabledReason::SIGNED_OUT, service->GetDisabledReasonFromDeps()); // Once the enabled pref is set to false, we should be disabled. - pref_service()->SetBoolean(prefs::kEnableSnippets, false); + utils_.pref_service()->SetBoolean(prefs::kEnableSnippets, false); EXPECT_EQ(DisabledReason::EXPLICITLY_DISABLED, - service()->GetDisabledReasonFromDeps()); + service->GetDisabledReasonFromDeps()); // The other dependencies shouldn't matter anymore. - fake_signin_manager()->SignIn("foo@bar.com"); + utils_.fake_signin_manager()->SignIn("foo@bar.com"); EXPECT_EQ(DisabledReason::EXPLICITLY_DISABLED, - service()->GetDisabledReasonFromDeps()); + service->GetDisabledReasonFromDeps()); } } // namespace ntp_snippets diff --git a/components/ntp_snippets/ntp_snippets_test_utils.cc b/components/ntp_snippets/ntp_snippets_test_utils.cc index d20b7afb341d37..c26ce93028e762 100644 --- a/components/ntp_snippets/ntp_snippets_test_utils.cc +++ b/components/ntp_snippets/ntp_snippets_test_utils.cc @@ -17,36 +17,36 @@ namespace ntp_snippets { namespace test { -MockSyncService::MockSyncService() +FakeSyncService::FakeSyncService() : can_sync_start_(true), is_sync_active_(true), configuration_done_(true), is_encrypt_everything_enabled_(false), active_data_types_(syncer::HISTORY_DELETE_DIRECTIVES) {} -MockSyncService::~MockSyncService() {} +FakeSyncService::~FakeSyncService() {} -bool MockSyncService::CanSyncStart() const { +bool FakeSyncService::CanSyncStart() const { return can_sync_start_; } -bool MockSyncService::IsSyncActive() const { +bool FakeSyncService::IsSyncActive() const { return is_sync_active_; } -bool MockSyncService::ConfigurationDone() const { +bool FakeSyncService::ConfigurationDone() const { return configuration_done_; } -bool MockSyncService::IsEncryptEverythingEnabled() const { +bool FakeSyncService::IsEncryptEverythingEnabled() const { return is_encrypt_everything_enabled_; } -syncer::ModelTypeSet MockSyncService::GetActiveDataTypes() const { +syncer::ModelTypeSet FakeSyncService::GetActiveDataTypes() const { return active_data_types_; } -NTPSnippetsTestBase::NTPSnippetsTestBase() +NTPSnippetsTestUtils::NTPSnippetsTestUtils() : pref_service_(new TestingPrefServiceSimple()) { pref_service_->registry()->RegisterStringPref(prefs::kGoogleServicesAccountId, std::string()); @@ -54,18 +54,15 @@ NTPSnippetsTestBase::NTPSnippetsTestBase() prefs::kGoogleServicesLastAccountId, std::string()); pref_service_->registry()->RegisterStringPref( prefs::kGoogleServicesLastUsername, std::string()); -} - -NTPSnippetsTestBase::~NTPSnippetsTestBase() {} - -void NTPSnippetsTestBase::SetUp() { signin_client_.reset(new TestSigninClient(pref_service_.get())); account_tracker_.reset(new AccountTrackerService()); - mock_sync_service_.reset(new MockSyncService()); + fake_sync_service_.reset(new FakeSyncService()); ResetSigninManager(); } -void NTPSnippetsTestBase::ResetSigninManager() { +NTPSnippetsTestUtils::~NTPSnippetsTestUtils() = default; + +void NTPSnippetsTestUtils::ResetSigninManager() { fake_signin_manager_.reset( new FakeSigninManagerBase(signin_client_.get(), account_tracker_.get())); } diff --git a/components/ntp_snippets/ntp_snippets_test_utils.h b/components/ntp_snippets/ntp_snippets_test_utils.h index 430515aea07cbc..272b692e315f91 100644 --- a/components/ntp_snippets/ntp_snippets_test_utils.h +++ b/components/ntp_snippets/ntp_snippets_test_utils.h @@ -19,10 +19,10 @@ class TestSigninClient; namespace ntp_snippets { namespace test { -class MockSyncService : public sync_driver::FakeSyncService { +class FakeSyncService : public sync_driver::FakeSyncService { public: - MockSyncService(); - ~MockSyncService() override; + FakeSyncService(); + ~FakeSyncService() override; bool CanSyncStart() const override; bool IsSyncActive() const override; @@ -37,18 +37,16 @@ class MockSyncService : public sync_driver::FakeSyncService { syncer::ModelTypeSet active_data_types_; }; -// Common base for snippet tests, handles initializing mocks for sync and -// signin. |SetUp()| should be called if a subclass overrides it. -class NTPSnippetsTestBase : public testing::Test { +// Common utilities for snippet tests, handles initializing fakes for sync and +// signin. +class NTPSnippetsTestUtils { public: - void SetUp() override; + NTPSnippetsTestUtils(); + ~NTPSnippetsTestUtils(); - protected: - NTPSnippetsTestBase(); - ~NTPSnippetsTestBase() override; void ResetSigninManager(); - MockSyncService* mock_sync_service() { return mock_sync_service_.get(); } + FakeSyncService* fake_sync_service() { return fake_sync_service_.get(); } FakeSigninManagerBase* fake_signin_manager() { return fake_signin_manager_.get(); } @@ -56,7 +54,7 @@ class NTPSnippetsTestBase : public testing::Test { private: std::unique_ptr fake_signin_manager_; - std::unique_ptr mock_sync_service_; + std::unique_ptr fake_sync_service_; std::unique_ptr pref_service_; std::unique_ptr signin_client_; std::unique_ptr account_tracker_;