Add unit test for Mixer::Publish.

chrome/browser/ui/app_list/search/mixer_unittest.cc

Index: chrome/browser/ui/app_list/search/mixer_unittest.cc
diff --git a/chrome/browser/ui/app_list/search/mixer_unittest.cc b/chrome/browser/ui/app_list/search/mixer_unittest.cc
index ddcf3a9f31cd40eac20ee2e79bc78c5d4f1173b3..7eb52f6ed6d3c6f5e222b880ba79da3ad3e20df5 100644
--- a/chrome/browser/ui/app_list/search/mixer_unittest.cc
+++ b/chrome/browser/ui/app_list/search/mixer_unittest.cc
@@ -182,5 +182,44 @@ TEST_F(MixerTest, RemoveDuplicates) {
   EXPECT_EQ("dup0,dup1,dup2", GetResults());
 }
 
+TEST_F(MixerTest, PublishReusesExisting) {

[Matt Giuca, 2014/07/08 01:48:23]

I think should just call this "Publish" (as it is the only test of Publish and
exposes many other aspects of its behaviour than just that it reuses existing).

[calamity, 2014/07/08 04:23:00]

Done.

+  ScopedVector<TestSearchResult> test_results;

[Matt Giuca, 2014/07/08 01:48:24]

I don't think you should use a vector here. It implies that you will pass these
four results to an API at some point, but you're just using it as a holding cell
for various bits of test data.

Just do
scoped_ptr<TestSearchResult> result1(new TestSearchResult("app1", 0));
...

[calamity, 2014/07/08 04:23:00]

Done.

+  test_results.push_back(new TestSearchResult("app1", 0));
+  test_results.push_back(new TestSearchResult("app2", 0));
+  test_results.push_back(new TestSearchResult("app3", 0));
+  test_results.push_back(new TestSearchResult("app4", 0));
+
+  AppListModel::SearchResults ui_results;

[Matt Giuca, 2014/07/08 01:48:23]

// Publish the first three results to |ui_results|.

[calamity, 2014/07/08 04:23:01]

Done.

+  std::vector<Mixer::SortData> new_results;
+
+  new_results.push_back(Mixer::SortData(test_results[0], 1.0f));
+  new_results.push_back(Mixer::SortData(test_results[1], 1.0f));
+  new_results.push_back(Mixer::SortData(test_results[2], 1.0f));
+
+  Mixer::Publish(new_results, &ui_results);
+  EXPECT_EQ(3u, ui_results.item_count());

[Matt Giuca, 2014/07/08 01:48:23]

EXPECT_EQ(new_results[0], ui_results.GetItemAt(0));
EXPECT_EQ(new_results[1], ui_results.GetItemAt(1));
EXPECT_EQ(new_results[2], ui_results.GetItemAt(2));

[calamity, 2014/07/08 04:23:00]

As discussed, this isn't true since new copies get made.

+
+  // Ensure old results are reused if the ID is the same.

[Matt Giuca, 2014/07/08 01:48:23]

Save the "ensures" for the EXPECT section. Here just explain what you are doing.

// Save the current |ui_results| for comparison later.

[calamity, 2014/07/08 04:23:00]

Done.

+  std::vector<const SearchResult*> old_ui_results;
+  for (size_t i = 0; i < ui_results.item_count(); ++i)
+    old_ui_results.push_back(ui_results.GetItemAt(i));
+
+  // The first result will be different as the ID has changed.

[Matt Giuca, 2014/07/08 01:48:23]

// Change the first result to a totally new object (with a new ID).

[calamity, 2014/07/08 04:23:00]

Done.

+  new_results[0] = Mixer::SortData(test_results[3], 1.0f);

[Matt Giuca, 2014/07/08 01:48:23]

Shouldn't you have:
new_results[1] = Mixer::SortData(new TestSearchResult("app2"), 1.0f);
new_results[2] = Mixer::SortData(new TestSearchResult("app3"), 1.0f);

I thought the whole point of this was to ensure that if new_results had a
*distinct* object (different memory address) but the *same* ID, then ui_results
would keep the *original* object (same memory address). You aren't really
testing that unless your second Publish includes some results with different
memory address but the same ID.

[calamity, 2014/07/08 04:23:00]

As discussed, copies are always made because the input SearchResults are owned.
Changed the third result's object to test that the same ui result is used
despite a different input result.

[Matt Giuca, 2014/07/08 05:31:45]

As discussed, do the duplication on 2, not 3 (to test the case of "different
object, same id, different title"). Keep 3 as the same object.

+
+  // The second result will have a different title but still use the original
+  // object.

[Matt Giuca, 2014/07/08 01:48:23]

This is VERY CONFUSING since without reading TestSearchResult, you would think
that you are changing "app2" to "app2altname", but actually you are keeping the
ID "app2" but changing the title. Can you use "New App 2 Title" instead of
"app2altname" so it's obvious this is a different kind of thing to the id
"app2", and add comment:

// Change the second result's title, but keep the same id. (The result will keep
the id "app2" but change its title to "New App 2 Title".)

[calamity, 2014/07/08 04:23:00]

Done.

+  const base::string16 kApp2AltName = base::UTF8ToUTF16("app2altname");
+  new_results[1].result->set_title(kApp2AltName);
+
+  Mixer::Publish(new_results, &ui_results);
+  EXPECT_EQ(3u, ui_results.item_count());
+
+  EXPECT_NE(old_ui_results[0], ui_results.GetItemAt(0));

[Matt Giuca, 2014/07/08 01:48:23]

// The first result will be a new object, as the ID has changed.
EXPECT_NE(...)
EXPECT_EQ(new_results[0], ui_results.GetItemAt(0));

[calamity, 2014/07/08 04:23:00]

Done.

Second check does not apply.

+  EXPECT_EQ(old_ui_results[1], ui_results.GetItemAt(1));

[Matt Giuca, 2014/07/08 01:48:23]

// The second result will still use the original object, but have a different
title, since the ID did not change.

[calamity, 2014/07/08 04:23:00]

Done.

+  EXPECT_EQ(kApp2AltName, new_results[1].result->title());

[Matt Giuca, 2014/07/08 01:48:23]

This isn't testing anything. Should be:
EXPECT_EQ(kApp2AltName, ui_results.GetItemAt(1).result->title());

[calamity, 2014/07/08 04:23:00]

Done.

+  EXPECT_EQ(old_ui_results[2], ui_results.GetItemAt(2));
+}
+
 }  // namespace test
 }  // namespace app_list