Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
372843003:
Add unit test for Mixer::Publish. (Closed)
|
Created:
6 years, 5 months ago by calamity Modified:
6 years, 5 months ago Reviewers:
Matt Giuca CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd unit test for Mixer::Publish.
This CL adds a unit test for the app list search result mixer. This
involves making Publish a static method on Mixer and moving the SortData
struct into the header file.
This is a precursor CL for https://codereview.chromium.org/369693004/.
BUG=349727
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281986
Patch Set 1 : #
Total comments: 26
Patch Set 2 : address comments #Patch Set 3 : use testing macros for friendliness #
Total comments: 2
Patch Set 4 : make things static #Patch Set 5 : add instance ids to TestSearchResult #
Total comments: 2
Patch Set 6 : address comment, fix an address comparison that was missed #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/mixer.h (right): https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer.h:60: static void Publish(const std::vector<SortData>& results, I think the standard practice here would be to make SortData and Publish private and add MixerTest as a friend. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/mixer_unittest.cc (right): https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:185: TEST_F(MixerTest, PublishReusesExisting) { 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). https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:186: ScopedVector<TestSearchResult> test_results; 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)); ... https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:192: AppListModel::SearchResults ui_results; // Publish the first three results to |ui_results|. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:200: EXPECT_EQ(3u, ui_results.item_count()); 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)); https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:202: // Ensure old results are reused if the ID is the same. Save the "ensures" for the EXPECT section. Here just explain what you are doing. // Save the current |ui_results| for comparison later. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:207: // The first result will be different as the ID has changed. // Change the first result to a totally new object (with a new ID). https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:208: new_results[0] = Mixer::SortData(test_results[3], 1.0f); 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. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:211: // object. 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".) https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:218: EXPECT_NE(old_ui_results[0], ui_results.GetItemAt(0)); // The first result will be a new object, as the ID has changed. EXPECT_NE(...) EXPECT_EQ(new_results[0], ui_results.GetItemAt(0)); https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:219: EXPECT_EQ(old_ui_results[1], ui_results.GetItemAt(1)); // The second result will still use the original object, but have a different title, since the ID did not change. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:220: EXPECT_EQ(kApp2AltName, new_results[1].result->title()); This isn't testing anything. Should be: EXPECT_EQ(kApp2AltName, ui_results.GetItemAt(1).result->title());
https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/mixer.h (right): https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer.h:60: static void Publish(const std::vector<SortData>& results, On 2014/07/08 01:48:23, Matt Giuca wrote: > I think the standard practice here would be to make SortData and Publish private > and add MixerTest as a friend. Done-ish. SortData still needs to be public to be constructible by the test. Friendliness doesn't extend to subclasses. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/mixer_unittest.cc (right): https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:185: TEST_F(MixerTest, PublishReusesExisting) { On 2014/07/08 01:48:23, Matt Giuca wrote: > 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). Done. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:186: ScopedVector<TestSearchResult> test_results; On 2014/07/08 01:48:24, Matt Giuca wrote: > 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)); > ... Done. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:192: AppListModel::SearchResults ui_results; On 2014/07/08 01:48:23, Matt Giuca wrote: > // Publish the first three results to |ui_results|. Done. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:200: EXPECT_EQ(3u, ui_results.item_count()); On 2014/07/08 01:48:23, Matt Giuca wrote: > 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)); As discussed, this isn't true since new copies get made. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:202: // Ensure old results are reused if the ID is the same. On 2014/07/08 01:48:23, Matt Giuca wrote: > Save the "ensures" for the EXPECT section. Here just explain what you are doing. > > // Save the current |ui_results| for comparison later. Done. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:207: // The first result will be different as the ID has changed. On 2014/07/08 01:48:23, Matt Giuca wrote: > // Change the first result to a totally new object (with a new ID). Done. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:208: new_results[0] = Mixer::SortData(test_results[3], 1.0f); On 2014/07/08 01:48:23, Matt Giuca wrote: > 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. 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. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:211: // object. On 2014/07/08 01:48:23, Matt Giuca wrote: > 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".) Done. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:218: EXPECT_NE(old_ui_results[0], ui_results.GetItemAt(0)); On 2014/07/08 01:48:23, Matt Giuca wrote: > // The first result will be a new object, as the ID has changed. > EXPECT_NE(...) > EXPECT_EQ(new_results[0], ui_results.GetItemAt(0)); Done. Second check does not apply. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:219: EXPECT_EQ(old_ui_results[1], ui_results.GetItemAt(1)); On 2014/07/08 01:48:23, Matt Giuca wrote: > // The second result will still use the original object, but have a different > title, since the ID did not change. Done. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:220: EXPECT_EQ(kApp2AltName, new_results[1].result->title()); On 2014/07/08 01:48:23, Matt Giuca wrote: > This isn't testing anything. Should be: > EXPECT_EQ(kApp2AltName, ui_results.GetItemAt(1).result->title()); Done.
https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/mixer.h (right): https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer.h:60: static void Publish(const std::vector<SortData>& results, Use FRIEND_TEST_ALL_PREFIXES to actually friend the test class, not the base class. https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/mixer_unittest.cc (right): https://codereview.chromium.org/372843003/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/mixer_unittest.cc:208: new_results[0] = Mixer::SortData(test_results[3], 1.0f); 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.
LGTM if you make things static. https://codereview.chromium.org/372843003/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/mixer.h (right): https://codereview.chromium.org/372843003/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/mixer.h:72: void Publish(const SortedResults& results, Please make this (and RemoveDuplicates) static. (They don't use fields.)
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/372843003/140001
https://codereview.chromium.org/372843003/diff/100001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/mixer.h (right): https://codereview.chromium.org/372843003/diff/100001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/mixer.h:72: void Publish(const SortedResults& results, On 2014/07/08 06:09:08, Matt Giuca wrote: > Please make this (and RemoveDuplicates) static. (They don't use fields.) Done.
The CQ bit was checked by calamity@chromium.org
The CQ bit was unchecked by calamity@chromium.org
lgtm, you earned that gold star, son! https://codereview.chromium.org/372843003/diff/200001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/mixer_unittest.cc (right): https://codereview.chromium.org/372843003/diff/200001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/mixer_unittest.cc:42: static int GetInstanceId(SearchResult* result) { // For reference equality testing. (Addresses cannot be used to test reference equality because it is possible that an object will be allocated at the same address as a previously deleted one.)
https://codereview.chromium.org/372843003/diff/200001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/mixer_unittest.cc (right): https://codereview.chromium.org/372843003/diff/200001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/mixer_unittest.cc:42: static int GetInstanceId(SearchResult* result) { On 2014/07/09 01:53:22, Matt Giuca wrote: > // For reference equality testing. (Addresses cannot be used to test reference > equality because it is possible that an object will be allocated at the same > address as a previously deleted one.) Done.
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/372843003/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 281986 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
