a provider of Physical Web pages.

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
new file mode 100644
index 0000000000000000000000000000000000000000..423c8b6ab39a46dd318073462a2259829be66fe8
--- /dev/null
+++ b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
@@ -0,0 +1,78 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h"
+
+#include <string>
+#include <vector>
+
+#include "components/ntp_snippets/category.h"
+#include "components/ntp_snippets/category_factory.h"
+#include "components/ntp_snippets/content_suggestions_provider.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ntp_snippets {
+
+namespace {
+
+UrlInfo CreateUrlInfo(const std::string& site_url) {
+  UrlInfo url_info;
+  url_info.site_url = GURL(site_url);
+  return url_info;
+}
+
+std::vector<UrlInfo> CreateUrlInfos(const std::vector<std::string>& site_urls) {
+  std::vector<UrlInfo> url_infos;
+  for (const std::string& site_url : site_urls) {
+    url_infos.emplace_back(CreateUrlInfo(site_url));
+  }
+  return url_infos;
+}
+
+class MockProviderObserver : public ContentSuggestionsProvider::Observer {

[tschumann, 2016/08/10 17:37:57]

Marc, do we already have such an observer for other tests? Might make sense to
move this into a mock_content_suggestions_observer.h library or the like.

[vitaliii, 2016/08/11 12:15:25]

Done.

+ public:
+  void OnNewSuggestions(ContentSuggestionsProvider* provider, Category category,
+                        std::vector<ContentSuggestion> suggestions) override {
+    std::vector<std::string> urls;
+    for (const auto& suggestion : suggestions) {
+      urls.push_back(suggestion.url().spec());
+    }
+    OnNewSuggestionsUrls(urls);

[Marc Treib, 2016/08/10 16:22:03]

You could make the mock method take a "const std::vector<ContentSuggestion>&",
then you don't have to copy stuff.

[tschumann, 2016/08/10 17:37:57]

This seems odd -- you're adding a mock function (OnNewSuggestionsUrls()) that
isn't on the base class?

Unless I'm mistaken something that's sort-of abusing the mock system and might
actually break in the future (actually I thought MOCK_METHOD would also add an
override to the signature, but apparently not). Anyways, what you really want is
a better matcher that can verify the suggestions you got have a certain URL. You
can do this with something like this:

MATCHER_P(HasUrl, url, "") {
  *result_listener << "expected URL: " << url
                   << "has URL: " << arg.url().spec();
  return arg.url().spec() == url;
}

EXPECT_CALL(observer,
OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"),
HasUrl("http://test2.com/"));

(module formatting ;-)).

(for more details see the gMock docs,
https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList)

[Marc Treib, 2016/08/11 12:15:24]

This is a workaround for gMock's lack of support for movable types. Ideally,
we'd just like to mock the OnNewSuggestions method directly and then use proper
matchers in the test, but that doesn't compile. The common workaround is to
override the method manually, and pass the argument as a const ref to a
different (non-overriding) mock method. Hardly optimal, but this is what we're
working with.

[vitaliii, 2016/08/11 12:15:25]

Since ContentSuggestion cannot be copied and assigned,
MOCK_METHOD3(OnNewSuggestions, (omited)) does not compile.

[tschumann, 2016/08/11 12:52:24]

yuck... I start wondering if it's really worth having ContentSuggestion not
being copy-able -- that's actually the reason why we're not passing this value
by const reference.

As it is, I'm opposed as tests get super confusing:inside the TEST() you set up
expectations about calling methods that don't exist...

However, what should work and might hide this ugliness inside the Mock would be
to add an overload
that behaves the same way, e.g.:
void OnNewSuggestions(ContentSuggestionsProvider* provider, Category category,
                      std::vector<ContentSuggestion> suggestions) override {
  std::list<ContentSuggestion> suggestions_list;
  for (auto& suggestion: suggestion) {
    suggestions_list.emplace_back(std::move(suggestion));
  }
  OnNewSuggestions(provider, category, suggestions_list);
}

MOCK_METHOD3(OnNewSuggestions, void(ContentSuggestionsProvider* provider,
Category category,
                                    const std::list<ContentSuggestion>&
suggestions));


We should also add some comments explaining this hack and why it's needed.

[Marc Treib, 2016/08/11 13:09:14]

No, that's not the only reason. This way, the ContentSuggestionsService doesn't
have to copy all the suggestions it gets; it can just std::move them out of the
vector it gets passed.
Of course, you could argue that that's not worth the hassle, since the copying
shouldn't be *that* expensive.
Wait, std::list so that it gets disambiguated from std::vector? Not sure I
prefer that...
+1 (said the same thing :) )

[tschumann, 2016/08/11 13:14:05]

yes, we need to somehow allow the compiler to distinguish the methods, simply
having an overload with const& doesn't work out. The nice property of the
std::list is that from a test perspective, you don't see a difference (except
maybe in the error message). If gmock get's fixed one day, we just need to clean
up the mock itself and are done.
My primary concern is that I can understand the test by just looking at the
TEST() and the observer interface.
wdyt?

[Marc Treib, 2016/08/11 13:17:53]

Alright, makes sense. No objections :)

[vitaliii, 2016/08/11 14:29:58]

Acknowledged.

+  }
+
+  MOCK_METHOD1(OnNewSuggestionsUrls, void(std::vector<std::string> urls));
+  MOCK_METHOD3(OnCategoryStatusChanged,
+               void(ContentSuggestionsProvider* provider, Category category,
+                    CategoryStatus new_status));
+};
+
+class PhysicalWebPageSuggestionsProviderTest : public testing::Test {
+ public:
+  PhysicalWebPageSuggestionsProviderTest() {}
+
+  void SetUp() override {}
+
+  void TearDown() override {}

[Marc Treib, 2016/08/10 16:22:03]

No need to implement SetUp and TearDown if they don't do anything.

In fact, if the test class doesn't do anything, you can just remove it (and use
TEST() instead of TEST_F()). If more tests are coming soon, it might make sense
to move e.g. the observer into the class though.

[vitaliii, 2016/08/11 12:15:25]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PhysicalWebPageSuggestionsProviderTest);
+};
+
+TEST_F(PhysicalWebPageSuggestionsProviderTest, ShouldCreateSuggestions) {

[tschumann, 2016/08/10 17:37:57]

If you don't use anything of the test fixture (==the class class
PhysicalWebPageSuggestionsProviderTest), then you don't have to use the TEST_F()
macro but can use TEST() instead.

Simply remove the fixture and write:
TEST(PhysicalWebPageSuggestionsProviderTest, ShouldCreateSuggestions) {

[vitaliii, 2016/08/11 12:15:25]

Done.

+  MockProviderObserver observer;
+  CategoryFactory category_factory;
+  ContentSuggestionsProvider::Observer* ob = &observer;
+  PhysicalWebPageSuggestionsProvider provider(ob, &category_factory);

[Marc Treib, 2016/08/10 16:22:03]

Just do &observer here and get rid of the ob variable?

[vitaliii, 2016/08/11 12:15:25]

Done.

+  const std::vector<std::string> urls(
+      {"http://test1.com/", "http://test2.com/"});
+  const std::vector<UrlInfo> url_infos = CreateUrlInfos(urls);
+
+  EXPECT_CALL(observer, OnNewSuggestionsUrls(urls));
+  provider.OnDisplayableUrlsChanged(url_infos);
+}
+
+}  // namespace

[Marc Treib, 2016/08/10 16:22:03]

Only the global functions should be in the anonymous namespace

[tschumann, 2016/08/10 17:37:57]

actually, i think there's a tendency to have all tests in the unnamed namespace
(https://yaqs.googleplex.com/eng/q/5783337608151040). Unless you need to make
use of friend tests.

I'm usually not feeling strong enough to argue, but if we had to settle on
something, I'd be in favor of putting it all into an unnamed namespace.

[Marc Treib, 2016/08/11 12:15:24]

This is not a common tendency in Chrome I think, but alright.
I think it won't work though if we need to make the test a friend of the tested
class? So if we go for "everything in the anon namespace", then there'd always
be exceptions.

[vitaliii, 2016/08/11 12:15:25]

Other files in the folder (e.g.
https://cs.chromium.org/chromium/src/components/ntp_snippets/ntp_snippets_dat...)
do not use an unnamed namespace, so I removed this one too.

[tschumann, 2016/08/11 12:52:24]

Consistency trumps; so let's remove it. [however, i'd be fine with exceptions
for friend tests as they should be exceptional ;-)].

[Marc Treib, 2016/08/11 13:09:14]

They're somewhat common in unit tests. Personally, I prefer friend declarations
to "ForTesting" methods or to "// public for testing".

[vitaliii, 2016/08/11 14:29:58]

Acknowledged.

+
+}  // namespace ntp_snippets