Refactor history menu tests for clearer setup

chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm

Index: chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm
diff --git a/chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm b/chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm
index 73e8b54f2db13787903a2d660a5f63c67f4a6155..9242dd7fb508605ab98b881064c0d63f844f266a 100644
--- a/chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm
+++ b/chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm
@@ -50,6 +50,107 @@ class MockBridge : public HistoryMenuBridge {
   base::scoped_nsobject<NSMenu> menu_;
 };
 
+class TestEntries {

[Sidney San Martín, 2016/07/29 20:21:33]

This class is longer/nastier than I wanted — if anyone sees ways to
shorten/simplify it, let me know.

+ public:
+  struct Entry {
+    SessionID::id_type id;
+    virtual std::unique_ptr<MockTRS::Entry> toTRS() const = 0;

[Robert Sesek, 2016/07/29 21:37:25]

naming: always use CamelCase for method names, unless it is a simple accessor,
in which case you use under_scores. This site and throughout.

https://google.github.io/styleguide/cppguide.html#Function_Names

+    virtual void test(const MockBridge::HistoryItem&, NSMenuItem*) const = 0;
+
+    void test(const MockBridge::HistoryItem& historyItem) const {
+      EXPECT_EQ(id, historyItem.session_id);
+    }
+
+    Entry(SessionID::id_type id_) : id{id_} {}
+  };
+
+  struct Tab : public Entry {
+    std::string url, title;

[Robert Sesek, 2016/07/29 21:37:25]

Don't declare member variables using comma notation.

+
+    Tab(SessionID::id_type id_, std::string url_, std::string title_)

[Robert Sesek, 2016/07/29 21:37:25]

Take std::string arguments as const&, and the argument names shouldn't end in _
(trailing underscores are reserved for member variables).

[Sidney San Martín, 2016/07/29 22:02:33]

WRT underscores, it's to avoid shadowing the members — what do you folks usually
do in a constructor like this?

[Robert Sesek, 2016/08/02 14:39:25]

I use a_var or an_var, indirect through the this pointer, or make the members
private and put a trailing underscore (i.e. convert a struct to a class).

+        : Entry{id_}, url{url_}, title{title_} {}
+
+    std::unique_ptr<MockTRS::Tab> toTRSTab() const {
+      auto tab = base::MakeUnique<MockTRS::Tab>();
+      tab->id = id;
+      tab->current_navigation_index = 0;
+      tab->navigations.push_back(
+          sessions::SerializedNavigationEntryTestHelper::CreateNavigation(
+              url, title));
+      return tab;
+    }
+
+    std::unique_ptr<MockTRS::Entry> toTRS() const override {
+      return std::unique_ptr<MockTRS::Entry>(toTRSTab());
+    }
+
+    void test(const MockBridge::HistoryItem& historyItem,

[Robert Sesek, 2016/07/29 21:37:25]

naming: unless this is an ObjC method, variables always use under_scores

+              NSMenuItem* menuItem) const override {
+      Entry::test(historyItem);
+      EXPECT_NSEQ(base::SysUTF8ToNSString(title), menuItem.title);
+    }
+  };
+
+  struct Window : public Entry {
+    std::vector<Tab> tabs;
+
+    Window(SessionID::id_type id_, std::vector<Tab> tabs_)
+        : Entry{id_}, tabs{tabs_} {}
+
+    std::unique_ptr<MockTRS::Entry> toTRS() const override {
+      auto window = new MockTRS::Window;
+      window->id = id;
+      window->tabs.reserve(tabs.size());
+      for (const auto& tab : tabs) {
+        window->tabs.push_back(*tab.toTRSTab());
+      }
+      return std::unique_ptr<MockTRS::Entry>(window);
+    }
+
+    void test(const MockBridge::HistoryItem& historyItem,
+              NSMenuItem* menuItem) const override {
+      Entry::test(historyItem);
+      NSMenu* submenu = menuItem.submenu;
+      EXPECT_EQ(tabs.size() + 2, (size_t)submenu.numberOfItems);

[Robert Sesek, 2016/07/29 21:37:25]

numberOfItems should be NSUInteger and so should be testable without needing a
cast.

+      EXPECT_TRUE([submenu itemAtIndex:1].isSeparatorItem);
+      for (size_t i = 0; i < tabs.size(); i++) {
+        tabs[i].test(*historyItem.tabs[i], [submenu itemAtIndex:i + 2]);
+      }
+    }
+  };
+
+ private:

[Robert Sesek, 2016/07/29 21:37:25]

Style does not mix public and private. private always comes at the end.

[Sidney San Martín, 2016/07/29 22:02:33]

I like that. When I was writing this I thought entries_ would depend on those
structs, but it turns out it doesn't.

+  std::vector<std::unique_ptr<Entry>> entries_;
+
+ public:
+  TestEntries(std::initializer_list<Entry*> l) {
+    entries_.reserve(l.size());
+    for (const auto& entry : l) {
+      entries_.emplace_back(entry);
+    }
+  }
+
+  void test(MockBridge* bridge) const {
+    NSMenu* menu = bridge->HistoryMenu();
+    ASSERT_EQ(entries_.size(), (size_t)[[menu itemArray] count]);
+    for (size_t i = 0; i < entries_.size(); i++) {
+      NSMenuItem* menuItem = [menu itemAtIndex:i];
+      MockBridge::HistoryItem* historyItem =
+          bridge->HistoryItemForMenuItem(menuItem);
+      EXPECT_TRUE(historyItem);
+      entries_[i]->test(*historyItem, [menu itemAtIndex:i]);
+    }
+  }
+
+  MockTRS::Entries toTRS() const {
+    MockTRS::Entries ret;
+    for (const auto& entry : entries_) {
+      ret.push_back(entry->toTRS().release());
+    }
+    return ret;
+  }
+};
+
 class HistoryMenuBridgeTest : public CocoaProfileTest {
  public:
   void SetUp() override {
@@ -96,16 +197,6 @@ class HistoryMenuBridgeTest : public CocoaProfileTest {
     return item;
   }
 
-  MockTRS::Tab CreateSessionTab(const std::string& url,
-                                const std::string& title) {
-    MockTRS::Tab tab;
-    tab.current_navigation_index = 0;
-    tab.navigations.push_back(
-        sessions::SerializedNavigationEntryTestHelper::CreateNavigation(
-            url, title));
-    return tab;
-  }
-
   void GetFaviconForHistoryItem(HistoryMenuBridge::HistoryItem* item) {
     bridge_->GetFaviconForHistoryItem(item);
   }
@@ -213,123 +304,43 @@ TEST_F(HistoryMenuBridgeTest, AddItemToMenu) {
 // Test that the menu is created for a set of simple tabs.
 TEST_F(HistoryMenuBridgeTest, RecentlyClosedTabs) {
   std::unique_ptr<MockTRS> trs(new MockTRS(profile()));
-  MockTRS::Entries entries;
-
-  MockTRS::Tab tab1 = CreateSessionTab("http://google.com", "Google");
-  tab1.id = 24;
-  entries.push_back(&tab1);
 
-  MockTRS::Tab tab2 = CreateSessionTab("http://apple.com", "Apple");
-  tab2.id = 42;
-  entries.push_back(&tab2);
+  const TestEntries testEntries{
+    new TestEntries::Tab{24, "http://google.com", "Google"},
+    new TestEntries::Tab{42, "http://apple.com", "Apple"},
+  };
 
+  auto entries = testEntries.toTRS();
   using ::testing::ReturnRef;
   EXPECT_CALL(*trs.get(), entries()).WillOnce(ReturnRef(entries));
 
   bridge_->TabRestoreServiceChanged(trs.get());
-
-  NSMenu* menu = bridge_->HistoryMenu();
-  ASSERT_EQ(2U, [[menu itemArray] count]);
-
-  NSMenuItem* item1 = [menu itemAtIndex:0];
-  MockBridge::HistoryItem* hist1 = bridge_->HistoryItemForMenuItem(item1);
-  EXPECT_TRUE(hist1);
-  EXPECT_EQ(24, hist1->session_id);
-  EXPECT_NSEQ(@"Google", [item1 title]);
-
-  NSMenuItem* item2 = [menu itemAtIndex:1];
-  MockBridge::HistoryItem* hist2 = bridge_->HistoryItemForMenuItem(item2);
-  EXPECT_TRUE(hist2);
-  EXPECT_EQ(42, hist2->session_id);
-  EXPECT_NSEQ(@"Apple", [item2 title]);
+  testEntries.test(bridge_.get());
 }
 
 // Test that the menu is created for a mix of windows and tabs.
 TEST_F(HistoryMenuBridgeTest, RecentlyClosedTabsAndWindows) {
   std::unique_ptr<MockTRS> trs(new MockTRS(profile()));
-  MockTRS::Entries entries;
-
-  MockTRS::Tab tab1 = CreateSessionTab("http://google.com", "Google");
-  tab1.id = 24;
-  entries.push_back(&tab1);
-
-  // TODO(sdy): The tab ids below *must* be set after all of them have been
-  // pushed onto window.tabs. Otherwise, the ids will change when push_back
-  // reallocates the vector's storage and calls each tabs' copy ctor. Ugh.
-
-  MockTRS::Window win1;
-  win1.id = 30;
-  win1.tabs.push_back(CreateSessionTab("http://foo.com", "foo"));
-  win1.tabs.push_back(CreateSessionTab("http://bar.com", "bar"));
-  win1.tabs[0].id = 31;
-  win1.tabs[1].id = 32;
-  entries.push_back(&win1);
-
-  MockTRS::Tab tab2 = CreateSessionTab("http://apple.com", "Apple");
-  tab2.id = 42;
-  entries.push_back(&tab2);
-
-  MockTRS::Window win2;
-  win2.id = 50;
-  win2.tabs.push_back(CreateSessionTab("http://magic.com", "magic"));
-  win2.tabs.push_back(CreateSessionTab("http://goats.com", "goats"));
-  win2.tabs.push_back(CreateSessionTab("http://teleporter.com", "teleporter"));
-  win2.tabs[0].id = 51;
-  win2.tabs[1].id = 52;
-  win2.tabs[2].id = 53;
-  entries.push_back(&win2);
 
+  const TestEntries testEntries{
+    new TestEntries::Window{30, {
+      {31, "http://foo.com", "foo"},
+      {32, "http://bar.com", "bar"},
+    }},
+    new TestEntries::Tab{42, "http://apple.com", "Apple"},
+    new TestEntries::Window{50, {
+      {51, "http://magic.com", "magic"},
+      {52, "http://goats.com", "goats"},
+      {53, "http://teleporter.com", "teleporter"},
+    }},
+  };
+
+  auto entries = testEntries.toTRS();
   using ::testing::ReturnRef;
   EXPECT_CALL(*trs.get(), entries()).WillOnce(ReturnRef(entries));
 
   bridge_->TabRestoreServiceChanged(trs.get());
-
-  NSMenu* menu = bridge_->HistoryMenu();
-  ASSERT_EQ(4U, [[menu itemArray] count]);
-
-  NSMenuItem* item1 = [menu itemAtIndex:0];
-  MockBridge::HistoryItem* hist1 = bridge_->HistoryItemForMenuItem(item1);
-  EXPECT_TRUE(hist1);
-  EXPECT_EQ(24, hist1->session_id);
-  EXPECT_NSEQ(@"Google", [item1 title]);
-
-  NSMenuItem* item2 = [menu itemAtIndex:1];
-  MockBridge::HistoryItem* hist2 = bridge_->HistoryItemForMenuItem(item2);
-  EXPECT_TRUE(hist2);
-  EXPECT_EQ(30, hist2->session_id);
-  EXPECT_EQ(2U, hist2->tabs.size());
-  // Do not test menu item title because it is localized.
-  NSMenu* submenu1 = [item2 submenu];
-  EXPECT_EQ(4U, [[submenu1 itemArray] count]);
-  // Do not test Restore All Tabs because it is localiced.
-  EXPECT_TRUE([[submenu1 itemAtIndex:1] isSeparatorItem]);
-  EXPECT_NSEQ(@"foo", [[submenu1 itemAtIndex:2] title]);
-  EXPECT_NSEQ(@"bar", [[submenu1 itemAtIndex:3] title]);
-  EXPECT_EQ(31, hist2->tabs[0]->session_id);
-  EXPECT_EQ(32, hist2->tabs[1]->session_id);
-
-  NSMenuItem* item3 = [menu itemAtIndex:2];
-  MockBridge::HistoryItem* hist3 = bridge_->HistoryItemForMenuItem(item3);
-  EXPECT_TRUE(hist3);
-  EXPECT_EQ(42, hist3->session_id);
-  EXPECT_NSEQ(@"Apple", [item3 title]);
-
-  NSMenuItem* item4 = [menu itemAtIndex:3];
-  MockBridge::HistoryItem* hist4 = bridge_->HistoryItemForMenuItem(item4);
-  EXPECT_TRUE(hist4);
-  EXPECT_EQ(50, hist4->session_id);
-  EXPECT_EQ(3U, hist4->tabs.size());
-  // Do not test menu item title because it is localized.
-  NSMenu* submenu2 = [item4 submenu];
-  EXPECT_EQ(5U, [[submenu2 itemArray] count]);
-  // Do not test Restore All Tabs because it is localiced.
-  EXPECT_TRUE([[submenu2 itemAtIndex:1] isSeparatorItem]);
-  EXPECT_NSEQ(@"magic", [[submenu2 itemAtIndex:2] title]);
-  EXPECT_NSEQ(@"goats", [[submenu2 itemAtIndex:3] title]);
-  EXPECT_NSEQ(@"teleporter", [[submenu2 itemAtIndex:4] title]);
-  EXPECT_EQ(51, hist4->tabs[0]->session_id);
-  EXPECT_EQ(52, hist4->tabs[1]->session_id);
-  EXPECT_EQ(53, hist4->tabs[2]->session_id);
+  testEntries.test(bridge_.get());
 }
 
 // Tests that we properly request an icon from the FaviconService.