Added CategoryFactoryTest.

components/ntp_snippets/category_factory_unittest.cc

Index: components/ntp_snippets/category_factory_unittest.cc
diff --git a/components/ntp_snippets/category_factory_unittest.cc b/components/ntp_snippets/category_factory_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..804ec0a446b9f08ec9dfcabb8471f3bf3fb172d8
--- /dev/null
+++ b/components/ntp_snippets/category_factory_unittest.cc
@@ -0,0 +1,129 @@
+// 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/category_factory.h"
+
+#include <algorithm>
+#include <vector>
+
+#include "components/ntp_snippets/category.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ntp_snippets {
+
+class CategoryFactoryTest : public testing::Test {
+ public:
+  CategoryFactoryTest() {}
+
+  void SetUp() override { unused_remote_category_id_ = 1; }

[Marc Treib, 2016/08/16 13:44:21]

Put this in the constructor and remove SetUp

[vitaliii, 2016/08/17 13:45:07]

Done.

+
+  int GetUnusedRemoteCategoryID() {
+    int id = unused_remote_category_id_;
+    ++unused_remote_category_id_;

[Philipp Keck, 2016/08/16 13:22:47]

Isn't this whole function equivalent to "return unused_remote_category_id++"?

[Marc Treib, 2016/08/17 14:41:34]

+1 - I'd just do
return unused_remote_category_id++;

[Marc Treib, 2016/08/18 08:21:53]

This is still open.

[vitaliii, 2016/08/18 09:00:13]

Done.

+    return id;
+  }
+
+  bool CompareCategories(const Category& left, const Category& right) const {
+    return factory()->CompareCategories(left, right);
+  }
+
+  KnownCategories KnownCategoryByID(int id) {
+    return static_cast<KnownCategories>(id);
+  }
+
+  CategoryFactory* factory() { return &factory_; }
+  const CategoryFactory* factory() const { return &factory_; }

[Marc Treib, 2016/08/16 13:44:21]

I think the const version isn't required.

[vitaliii, 2016/08/17 13:45:07]

Done.

+
+ private:
+  CategoryFactory factory_;
+  int unused_remote_category_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(CategoryFactoryTest);
+};
+
+TEST_F(CategoryFactoryTest,
+       FromKnownCategoryShouldReturnSameIdForSameCategories) {
+  const KnownCategories known_category = KnownCategoryByID(0);

[Marc Treib, 2016/08/16 13:44:21]

Just use any actual known category here and get rid of KnownCategoryByID?

[vitaliii, 2016/08/17 13:45:07]

Done.

+  Category first = factory()->FromKnownCategory(known_category);
+  Category second = factory()->FromKnownCategory(known_category);
+  EXPECT_EQ(first, second);
+}
+
+TEST_F(CategoryFactoryTest,
+       FromRemoteCategoryShouldReturnSameIdForSameCategories) {
+  const int remote_category_id = GetUnusedRemoteCategoryID();
+  Category first = factory()->FromRemoteCategory(remote_category_id);
+  Category second = factory()->FromRemoteCategory(remote_category_id);
+  EXPECT_EQ(first, second);
+}
+
+TEST_F(CategoryFactoryTest, FromRemoteCategoryOrder) {
+  const int first_id = GetUnusedRemoteCategoryID();
+  const int second_id = GetUnusedRemoteCategoryID();
+  Category added_first = factory()->FromRemoteCategory(second_id);
+  Category added_second = factory()->FromRemoteCategory(first_id);
+  EXPECT_TRUE(CompareCategories(added_first, added_second));
+  EXPECT_FALSE(CompareCategories(added_second, added_first));
+}
+
+TEST_F(CategoryFactoryTest, FromIDValueReturnsSameKnownCategory) {
+  const KnownCategories raw_known_category = KnownCategoryByID(0);
+  Category known_category = factory()->FromKnownCategory(raw_known_category);
+  Category known_category_by_id = factory()->FromIDValue(known_category.id());
+  EXPECT_EQ(known_category, known_category_by_id);
+}
+
+TEST_F(CategoryFactoryTest, FromIDValueReturnsSameRemoteCategory) {
+  const int remote_category_id = GetUnusedRemoteCategoryID();
+  Category remote_category = factory()->FromRemoteCategory(remote_category_id);
+  Category remote_category_by_id = factory()->FromIDValue(remote_category.id());
+  EXPECT_EQ(remote_category, remote_category_by_id);
+}
+
+TEST_F(CategoryFactoryTest, CompareCategoriesLocalBeforeRemote) {
+  const int remote_category_id = GetUnusedRemoteCategoryID();
+  Category remote_category = factory()->FromRemoteCategory(remote_category_id);
+  const KnownCategories raw_known_category = KnownCategoryByID(0);
+  Category local_category = factory()->FromKnownCategory(raw_known_category);
+  EXPECT_TRUE(CompareCategories(local_category, remote_category));
+  EXPECT_FALSE(CompareCategories(remote_category, local_category));
+}
+
+TEST_F(CategoryFactoryTest, CompareCategoriesSame) {
+  const int remote_category_id = GetUnusedRemoteCategoryID();
+  Category remote_category = factory()->FromRemoteCategory(remote_category_id);
+  EXPECT_FALSE(CompareCategories(remote_category, remote_category));
+
+  const KnownCategories raw_known_category = KnownCategoryByID(0);
+  Category local_category = factory()->FromKnownCategory(raw_known_category);
+  EXPECT_FALSE(CompareCategories(local_category, local_category));
+}
+
+TEST_F(CategoryFactoryTest, CompareCategoriesSortContract) {

[Marc Treib, 2016/08/16 13:44:21]

This name is not very expressive - it really tests that remote categories are
ordered by order of registering.

[vitaliii, 2016/08/17 13:45:07]

Acknowledged.

[Marc Treib, 2016/08/17 14:41:34]

CL tool usage nit: "Acknowledged" means "I read your comment but didn't do
anything about it". Usually you want "Done" instead :)

+  std::vector<int> remote_category_ids;
+  for (size_t i = 0; i < 4; ++i) {

[Philipp Keck, 2016/08/16 13:22:47]

Include stddef.h because of size_t? Let Marc decide.

[Marc Treib, 2016/08/16 13:44:21]

Eh.. it's technically required, but I think most files in Chrome don't include
it, and I've never seen any problems caused by that.
Since the counter isn't used for anything, might as well just make it an int.

[vitaliii, 2016/08/17 13:45:07]

Acknowledged.

+    remote_category_ids.push_back(GetUnusedRemoteCategoryID());
+  }
+
+  // Categories with higher id are added first.
+  std::reverse(remote_category_ids.begin(), remote_category_ids.end());
+
+  std::vector<Category> categories;
+  for (int id : remote_category_ids) {
+    categories.push_back(factory()->FromRemoteCategory(id));
+  }
+
+  for (size_t first_i = 0; first_i < remote_category_ids.size(); ++first_i) {
+    for (size_t second_i = 0; second_i < remote_category_ids.size();
+         ++second_i) {
+      int first_id = remote_category_ids[first_i];
+      int second_id = remote_category_ids[second_i];
+      EXPECT_EQ(first_id > second_id,
+                CompareCategories(categories[first_i], categories[second_i]));

[Marc Treib, 2016/08/16 13:44:21]

Hm, I find this a bit hard to read - it's not really clear from reading the code
what the expected behavior is.

Would the test lose anything if there were just two categories, and we hardcoded
the expected ordering?

[vitaliii, 2016/08/17 13:45:07]

It is already done just for two categories in FromRemoteCategoryOrder.
I rewrote this test to test two categories when more than two have been added.

+    }
+  }
+}
+
+}  // namespace ntp_snippets