Omnibox: Create Keyword Verbatim Result in Search Provider

chrome/browser/autocomplete/search_provider_unittest.cc

Index: chrome/browser/autocomplete/search_provider_unittest.cc
diff --git a/chrome/browser/autocomplete/search_provider_unittest.cc b/chrome/browser/autocomplete/search_provider_unittest.cc
index 8b9735c6208c5e0576c7ec2521024f98af74ff48..9923d27e5023713c79d77ccaf974130211f7f987 100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -787,8 +787,9 @@ TEST_F(SearchProviderTest, InlineMixedCaseMatches) {
   EXPECT_EQ(ASCIIToUTF16("FOO"), term_match.fill_into_edit);
 }
 
-// Verifies AutocompleteControllers sets descriptions for results correctly.
-TEST_F(SearchProviderTest, UpdateKeywordDescriptions) {
+// Verifies AutocompleteControllers returns results (including keyword

[Bart N., 2013/01/29 18:50:42]

Nit: returns -> return (unless you meant one AutocompleteController).

[Mark P, 2013/01/30 19:46:21]

Done.

[Mark P, 2013/01/30 19:46:21]

Done.

+// results) in the right order and sets descriptions for them correctly.

[Bart N., 2013/01/29 18:50:42]

sets -> set.

[Bart N., 2013/01/29 18:50:42]

sets -> set

[Mark P, 2013/01/30 19:46:21]

Done.

[Mark P, 2013/01/30 19:46:21]

Duplicate of another comment.

+TEST_F(SearchProviderTest, KeywordOrderingAndDescriptions) {
   // Add an entry that corresponds to a keyword search with 'term2'.
   AddSearchToHistory(keyword_t_url_, ASCIIToUTF16("term2"), 1);
   profile_.BlockUntilHistoryProcessesPendingRequests();
@@ -800,16 +801,29 @@ TEST_F(SearchProviderTest, UpdateKeywordDescriptions) {
       AutocompleteInput::ALL_MATCHES));
   const AutocompleteResult& result = controller.result();
 
-  // There should be two matches, one for the keyword one for what you typed.
-  ASSERT_EQ(2u, result.size());
+  // There should be three matches, one for the keyword history, one for
+  // keyword provider's what-you-typed, and one for the default provider's
+  // what you typed, in that order.  They'll all have keywords.  (The
+  // default provider's result will be listed under the default provider's
+  // keyword.)  Also, the top result will always have a description.  The
+  // third result, coming from a different provider than the first two,
+  // should also.  Whether the second result has one doesn't matter much.
+  // (If it was missing, people would infer that it's the same search
+  // provider as the one above it.)
+  ASSERT_EQ(3u, result.size());
+
+  EXPECT_GT(result.match_at(0).relevance, result.match_at(1).relevance);

[Bart N., 2013/01/29 18:50:42]

Technically speaking, why GT and not GE? 

[Mark P, 2013/01/30 19:46:21]

There shouldn't be ties in this test.  If there are, that's a problem.

+  EXPECT_GT(result.match_at(1).relevance, result.match_at(2).relevance);
 
   EXPECT_FALSE(result.match_at(0).keyword.empty());

[Bart N., 2013/01/29 18:50:42]

Honestly, I'd prefer exact assertion here, i.e. say exactly what keyword we are
expecting. The current set of expectations is very vague. If you do that, you
will not need the extra two EXPECTs you've added.

[Mark P, 2013/01/30 19:46:21]

Mostly done.  Did the exact equal asserts for the keyword provider keyword. 
Still did a EXPECT_NE comparison for the default provider; not sure what keyword
that coming with.  (When running the test it appears to arrive with "dummy"; I'm
not sure where that's coming from so I don't want to test that exact name
explicitly.)

   EXPECT_FALSE(result.match_at(1).keyword.empty());
-  EXPECT_NE(result.match_at(0).keyword, result.match_at(1).keyword);
+  EXPECT_FALSE(result.match_at(2).keyword.empty());
+  EXPECT_EQ(result.match_at(0).keyword, result.match_at(1).keyword);
+  EXPECT_NE(result.match_at(0).keyword, result.match_at(2).keyword);
 
   EXPECT_FALSE(result.match_at(0).description.empty());
-  EXPECT_FALSE(result.match_at(1).description.empty());
-  EXPECT_NE(result.match_at(0).description, result.match_at(1).description);
+  EXPECT_FALSE(result.match_at(2).description.empty());
+  EXPECT_NE(result.match_at(0).description, result.match_at(2).description);
 }

[Bart N., 2013/01/29 18:50:42]

I think it would be worth to add a unit test that actually verifies that keyword
verbatim type is SEARCH_OTHER_ENGINE.

[Mark P, 2013/01/30 19:46:21]

Added to this test (and analogous ones).

 
 // Verifies Navsuggest results don't set a TemplateURL, which instant relies on.