Overhaul ntp_snippets_service_unittest.cc.

components/ntp_snippets/ntp_snippet_unittest.cc

Index: components/ntp_snippets/ntp_snippet_unittest.cc
diff --git a/components/ntp_snippets/ntp_snippet_unittest.cc b/components/ntp_snippets/ntp_snippet_unittest.cc
index 93304b63f9649c318717050acf00b0590d7848d8..47c3e754b511de3f3dda7fc064911d10edf8ba27 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<NTPSnippet> SnippetFromContentSuggestionJSON(
+    const std::string& json) {
+  auto json_value = base::JSONReader::Read(json);
+  base::DictionaryValue* json_dict;
+  if (!json_value->GetAsDictionary(&json_dict)) {

[tschumann, 2016/08/30 06:57:21]

as you don't expect this to ever fail, you can also just CHECK(json_value->...);

[IMO, it's fine to CHECK-fail in case of bugs inside the test code itself.]

[Marc Treib, 2016/08/30 09:29:28]

I find it a bit nicer to ASSERT_*, so the test fails "properly" rather than
crashing.

[sfiera, 2016/08/30 13:16:43]

My feeling is that ASSERTing is better but often not worth the effort. Here,
since I've already made the effort, I'd keep it.

[tschumann, 2016/08/30 14:01:39]

Ok. My motivation is that the test verifies the system under test and uses
EXPECT and ASSERT to accomplish that.
But to verify behavior of the test itself -- invariants -- IMO check-failing is
fine as it a)should never happen (-->less branches to follow + simpler
semantics) and b) is easy to spot.

That aside, can you actually ASSERT within a non-void function? ASSERT (at least
in google3) has the really annoying property of including a return statement
which completely screws things up if not called within the TEST(){} block.

maybe we should discuss this in a larger context -- not necessarily in this CL.

+    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) {

[tschumann, 2016/08/30 06:57:20]

i don't think you need to handle this case. None of the callers relies on this.

+    return nullptr;
+  }
+  return NTPSnippet::CreateFromChromeReaderDictionary(*dict);
+}
+
+std::unique_ptr<base::DictionaryValue> SnippetWithTwoSources() {
+  std::string kJsonStr =

[Marc Treib, 2016/08/30 09:29:28]

nit: If you use kConstantNaming, you should also make it constant

[sfiera, 2016/08/30 13:16:43]

Done.

+      "{\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)) {

[tschumann, 2016/08/30 06:57:20]

CHECK()?

+    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

[Marc Treib, 2016/08/30 09:29:29]

Why? AFAICT, they're equivalent. Do we really want to test that the first one
gets chosen in that case?

nit: Comments should end with a ".", also below.

[sfiera, 2016/08/30 13:16:43]

I don't know; this was moved verbatim from the other test. I could change the
comment but keep the asserts as is, or change the asserts to merely expect that
the best_source has values, not what they are.
Done.

[Marc Treib, 2016/08/30 13:43:38]

Hm, so if we can't figure out why we'd expect this exact behavior, then my
preference would be not testing for it. If you're trying to move on with things,
just adding a comment (TODO) is fine.

[sfiera, 2016/08/30 16:33:09]

I've loosened the tests; there are things we care about, even if not the
specific rule about the first source being 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() {
+  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)) {

[tschumann, 2016/08/30 06:57:21]

CHECK()?

+    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