Omnibox: Create Keyword Verbatim Result in Search Provider

chrome/browser/autocomplete/search_provider_unittest.cc

 // Copyright 2012 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 "chrome/browser/autocomplete/search_provider.h"
 
 #include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
 #include "base/string_util.h"
 #include "base/time.h"
@@ skipping 769 matching lines @@
   ASSERT_NO_FATAL_FAILURE(QueryForInputAndSetWYTMatch(ASCIIToUTF16("f"),
                                                       &wyt_match));
   ASSERT_EQ(2u, provider_->matches().size());
   AutocompleteMatch term_match;
   EXPECT_TRUE(FindMatchWithDestination(term_url, &term_match));
   EXPECT_GT(term_match.relevance, wyt_match.relevance);
   EXPECT_EQ(1u, term_match.inline_autocomplete_offset);
   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();
 
   AutocompleteController controller(&profile_, NULL,
       AutocompleteProvider::TYPE_SEARCH);
   controller.Start(AutocompleteInput(
       ASCIIToUTF16("k t"), string16::npos, string16(), false, false, true,
       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.
 // Also verifies that just the *first* navigational result is listed as a match
 // if suggested relevance scores were not sent.
 TEST_F(SearchProviderTest, NavSuggestNoSuggestedRelevanceScores) {
   QueryForInput(ASCIIToUTF16("a.c"), string16(), false);
 
   // Make sure the default providers suggest service was queried.
   net::TestURLFetcher* fetcher = test_factory_.GetFetcherByID(
       SearchProvider::kDefaultProviderURLFetcherID);
@@ skipping 593 matching lines @@
   EXPECT_EQ(AutocompleteMatch::ACMatchClassification::URL,
             match.contents_class[0].style);
   EXPECT_EQ(4U, match.contents_class[1].offset);
   EXPECT_EQ(AutocompleteMatch::ACMatchClassification::URL |
             AutocompleteMatch::ACMatchClassification::MATCH,
             match.contents_class[1].style);
   EXPECT_EQ(5U, match.contents_class[2].offset);
   EXPECT_EQ(AutocompleteMatch::ACMatchClassification::URL,
             match.contents_class[2].style);
 }