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}

components/ntp_snippets/ntp_snippet_unittest.cc

@@ -12,6 +12,16 @@
 namespace ntp_snippets {
 namespace {
 
+std::unique_ptr<NTPSnippet> 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<NTPSnippet> SnippetFromChromeReaderDict(
+    std::unique_ptr<base::DictionaryValue> dict) {
+  if (!dict) {
+    return nullptr;
+  }
+  return NTPSnippet::CreateFromChromeReaderDictionary(*dict);
+}
+
+std::unique_ptr<base::DictionaryValue> 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<base::DictionaryValue> 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

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<NTPSnippetsStatusService> MakeService() {
+    return base::MakeUnique<NTPSnippetsStatusService>(
+        utils_.fake_signin_manager(), utils_.pref_service());
   }
 
  protected:
-  NTPSnippetsStatusService* service() { return service_.get(); }
-
- private:
-  std::unique_ptr<NTPSnippetsStatusService> 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

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()));
 }

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<FakeSigninManagerBase> fake_signin_manager_;
-  std::unique_ptr<MockSyncService> mock_sync_service_;
+  std::unique_ptr<FakeSyncService> fake_sync_service_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;
   std::unique_ptr<TestSigninClient> signin_client_;
   std::unique_ptr<AccountTrackerService> account_tracker_;