[NTP::PhysicalWeb] Implement Fetch for "More" button.

components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc

Index: components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
diff --git a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
index a1dc94ddb58ccf6e40b420f116844919c14da42a..96a4718e0c0074d222223ca7adc06de6b5bf5598 100644
--- a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
+++ b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
@@ -8,12 +8,16 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "components/ntp_snippets/category.h"
 #include "components/ntp_snippets/category_factory.h"
 #include "components/ntp_snippets/content_suggestions_provider.h"
 #include "components/ntp_snippets/mock_content_suggestions_provider_observer.h"
+#include "components/ntp_snippets/status.h"
 #include "components/physical_web/data_source/fake_physical_web_data_source.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -26,8 +30,10 @@ using physical_web::FakePhysicalWebDataSource;
 using testing::_;
 using testing::AnyNumber;
 using testing::ElementsAre;
+using testing::Pointwise;
 using testing::StrictMock;
 using testing::UnorderedElementsAre;
+using ::testing::get;

[Marc Treib, 2016/12/05 15:55:19]

Why does this have the leading "::", but none of the other "using"s do?

[vitaliii, 2016/12/06 07:30:21]

Done.

 
 namespace ntp_snippets {
 
@@ -35,16 +41,23 @@ namespace {
 
 MATCHER_P(HasUrl, url, "") {
   *result_listener << "expected URL: " << url
-                   << "has URL: " << arg.url().spec();
+                   << ", but has URL: " << arg.url().spec();
   return arg.url().spec() == url;
 }
 
+MATCHER(HasUrlPointwise, "") {

[Marc Treib, 2016/12/05 15:55:19]

I have no idea what "pointwise" means :)

[vitaliii, 2016/12/06 07:30:21]

This a matcher for Pointwise.

[Marc Treib, 2016/12/06 10:01:34]

And now I even looked up what Pointwise does :)
It's a bit ugly that this needs a different matcher which does basically the
exact same thing. Could this be written in terms of ElementsAre (or
ElementsAreArray)? (But not a big deal)

[vitaliii, 2016/12/06 12:45:52]

Done.

+  *result_listener << "expected URL: " << get<1>(arg)
+                   << ", but has URL: " << get<0>(arg).url().spec();

[Marc Treib, 2016/12/05 15:55:19]

nit: For EXPECT_EQ etc, the expected value comes first. Would be nice to stay
consistent.

[vitaliii, 2016/12/06 07:30:21]

I am not sure what you mean, but I changed order of parameters in return
statement below.

[Marc Treib, 2016/12/06 10:01:34]

I meant, when using EXPECT_EQ, you write
EXPECT_EQ(expected_value, actual_value);
But here, the first argument is "actual" and the second is "expected".
But apparently that's just how Pointwise works :(

[vitaliii, 2016/12/06 12:45:52]

Done.
Removed the matcher, rewrote without Pairwise.

+  return get<0>(arg).url().spec() == get<1>(arg);
+}
+
 }  // namespace
 
 class PhysicalWebPageSuggestionsProviderTest : public testing::Test {
  public:
   PhysicalWebPageSuggestionsProviderTest()
-      : pref_service_(base::MakeUnique<TestingPrefServiceSimple>()) {}
+      : pref_service_(base::MakeUnique<TestingPrefServiceSimple>()),
+        run_loop_(base::MakeUnique<base::RunLoop>()) {}
 
   void IgnoreOnCategoryStatusChangedToAvailable() {
     EXPECT_CALL(observer_, OnCategoryStatusChanged(_, provided_category(),
@@ -86,6 +99,17 @@ class PhysicalWebPageSuggestionsProviderTest : public testing::Test {
     physical_web_data_source_.NotifyOnDistanceChanged(url, new_distance);
   }
 
+  void CompareFetchMoreResult(
+      Status expected_status_code,
+      std::vector<std::string> expected_suggestions_urls,
+      Status actual_status_code,
+      std::vector<ContentSuggestion> actual_suggestions) {
+    EXPECT_EQ(actual_status_code.status, expected_status_code.status);
+    EXPECT_THAT(actual_suggestions,
+                Pointwise(HasUrlPointwise(), expected_suggestions_urls));
+    run_loop()->Quit();
+  }
+
   ContentSuggestionsProvider* provider() {
     DCHECK(provider_);
     return provider_.get();
@@ -96,12 +120,17 @@ class PhysicalWebPageSuggestionsProviderTest : public testing::Test {
   }
   MockContentSuggestionsProviderObserver* observer() { return &observer_; }
   TestingPrefServiceSimple* pref_service() { return pref_service_.get(); }
+  base::RunLoop* run_loop() { return run_loop_.get(); }
 
  private:
   FakePhysicalWebDataSource physical_web_data_source_;
   StrictMock<MockContentSuggestionsProviderObserver> observer_;
   CategoryFactory category_factory_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;
+  // Added in order to test provider's |Fetch| method.
+  base::MessageLoop message_loop_;
+  // Added to wait for |Fetch| callback to be called.
+  std::unique_ptr<base::RunLoop> run_loop_;

[Marc Treib, 2016/12/05 15:55:19]

This shouldn't be a member. Instantiate it locally where you need it, and pass
it around as necessary (probably you only need to pass around its QuitClosure(),
as a "done_callback").

[vitaliii, 2016/12/06 07:30:21]

Done.

   // Last so that the dependencies are deleted after the provider.
   std::unique_ptr<PhysicalWebPageSuggestionsProvider> provider_;
 
@@ -139,4 +168,49 @@ TEST_F(PhysicalWebPageSuggestionsProviderTest, ShouldSortByDistance) {
   CreateProvider();
 }
 
+TEST_F(PhysicalWebPageSuggestionsProviderTest,
+       FetchMoreShouldFilterAndReturnSortedSuggestions) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+  std::vector<int> ids;
+  const int number_of_suggestions_in_section = 10;

[Marc Treib, 2016/12/05 15:55:19]

kNumSuggestionsInSection?

[vitaliii, 2016/12/06 07:30:21]

Done.

+  const int number_of_suggestions_in_source =
+      3 * number_of_suggestions_in_section;
+  for (int i = 1; i <= number_of_suggestions_in_source; ++i) {
+    ids.push_back(i);
+  }
+  // |CreateDummyPhysicalWebPages| builds pages with distances 1, 2, 3, ... ,
+  // so we know the order of suggestions in the provider.
+  physical_web_data_source()->SetMetadata(CreateDummyPhysicalWebPages(ids));
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, provided_category(), _));
+  CreateProvider();
+
+  const std::string dummy_resolved_url = "https://resolved_url.com/";
+  std::set<std::string> known_ids;
+  for (int i = 1; i <= number_of_suggestions_in_section; ++i) {
+    known_ids.insert(dummy_resolved_url + base::IntToString(i));
+  }
+  known_ids.insert(dummy_resolved_url + base::IntToString(12));
+  known_ids.insert(dummy_resolved_url + base::IntToString(17));
+  std::vector<std::string> expected_suggestion_urls;
+  for (int i = 11;
+       i <= number_of_suggestions_in_source &&
+       expected_suggestion_urls.size() < number_of_suggestions_in_section;
+       ++i) {
+    if (i == 12 || i == 17) {
+      continue;
+    }
+    expected_suggestion_urls.push_back(dummy_resolved_url +
+                                       base::IntToString(i));

[Marc Treib, 2016/12/05 15:55:19]

TBH, I'd find it easier to read if this just spelled out the 10 IDs that are
expected.

[vitaliii, 2016/12/06 07:30:21]

Rewrote |for| loop to make it easier.

[Marc Treib, 2016/12/06 10:01:34]

Alright, fair enough.

[vitaliii, 2016/12/06 12:45:52]

Acknowledged.

+  }
+  provider()->Fetch(
+      provided_category(), known_ids,
+      base::Bind(
+          &PhysicalWebPageSuggestionsProviderTest::CompareFetchMoreResult,
+          base::Unretained(this), Status(StatusCode::SUCCESS),
+          expected_suggestion_urls));
+  // Wait for the callback to be called.
+  run_loop()->Run();
+}
+
 }  // namespace ntp_snippets