Omnibox: Create Bundled Field Trial; Convert SearchHistory trial to it

chrome/browser/omnibox/omnibox_field_trial_unittest.cc

 // Copyright (c) 2013 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/omnibox/omnibox_field_trial.h"
 
 #include "base/basictypes.h"
 #include "base/metrics/field_trial.h"
 #include "base/strings/string16.h"
 #include "chrome/common/metrics/entropy_provider.h"
+#include "chrome/common/metrics/variations/variations_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class OmniboxFieldTrialTest : public testing::Test {
  public:
   OmniboxFieldTrialTest() {}
 
   static void SetUpTestCase() {
     ResetFieldTrialList();

[Alexei Svitkine (slow), 2013/07/31 20:27:46]

Hmm, I think this keeps the field trial list persistent between all the cases in
your test, whereas if you do it in either a "virtual void SetUp() OVERRIDE"
method or in the ctor, you can have a field trial list per test case, so
different tests don't contaminate each other.

In fact, you can simplify this by just having a base::FieldTrialList
field_trial_list_; instance variable and initialize it in the ctor via
field_trial_list_(new metrics::SHA1EntropyProvider("foo")) instead of having the
more complicated ResetFieldTrialList() logic.

[Mark P, 2013/07/31 21:59:28]

These tests pass as-is.  (This isn't the changelist I discuss with you over
e-mail.)

In the change that adds another test, I needed to (and did) move the
field_trial_list_ to the constructor as you suggest.

I'd like to leave this as-is and fix the test structure in the same change that
requires the correct test structure to function correctly.

   }
 
   static void TearDownTestCase() {
     delete field_trial_list_;
     field_trial_list_ = NULL;
   }
 
   static void ResetFieldTrialList() {
     // It's important to delete the old pointer first which sets
     // FieldTrialList::global_ to NULL.
@@ skipping 93 matching lines @@
 
     ResetFieldTrialList();
     CreateTestTrial("AutocompleteDynamicTrial_2", "EnableZeroSuggest_Queries");
     EXPECT_TRUE(OmniboxFieldTrial::InZeroSuggestFieldTrial());
 
     ResetFieldTrialList();
     CreateTestTrial("AutocompleteDynamicTrial_3", "EnableZeroSuggest_URLs");
     EXPECT_TRUE(OmniboxFieldTrial::InZeroSuggestFieldTrial());
   }
 }
+
+TEST_F(OmniboxFieldTrialTest,
+    GetConsequencesOfRuleInPageClassificationContext) {
+  // Must be the same as kBundledExperimentFieldTrialName
+  // defined in omnibox_field_trial.cc.

[Peter Kasting, 2013/07/31 20:14:09]

Nit: Should we declare this in a .h somewhere, then?

[Mark P, 2013/07/31 21:59:28]

I'd rather not have someone reading the .h have to think about this
implementation detail, even if it's in a private section and listed as a friend.

[Alexei Svitkine (slow), 2013/08/01 15:25:35]

Fair enough, SGTM.

+  const std::string kTrialName = "OmniboxBundledExperimentV1";
+  {
+    std::map<std::string, std::string> params;
+    params["1:rule1"] = "1-rule1-value";
+    params["3:rule1"] = "3-rule1-value";
+    params["*:rule1"] = "*-rule1-value";
+    params["*:rule2"] = "*-rule2-value";
+    params["4:rule3"] = "4-rule3-value";
+    params["unrecognized"] = "unrecognized-value";
+    ASSERT_TRUE(chrome_variations::AssociateVariationParams(
+        kTrialName, "A", params));
+  }
+
+  base::FieldTrialList::CreateFieldTrial(kTrialName, "A");
+  EXPECT_EQ("1-rule1-value",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+          AutocompleteInput::NEW_TAB_PAGE, "rule1"));  // exact match
+  EXPECT_EQ("*-rule1-value",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+      AutocompleteInput::BLANK, "rule1"));  // fallback to global
+  EXPECT_EQ("3-rule1-value",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+      AutocompleteInput::HOMEPAGE, "rule1"));  // exact match
+  EXPECT_EQ("*-rule1-value",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+          AutocompleteInput::OTHER, "rule1"));  // fallback to global
+  EXPECT_EQ("*-rule2-value",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+          AutocompleteInput::HOMEPAGE, "rule2"));  // fallback to global
+  EXPECT_EQ("*-rule2-value",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+          AutocompleteInput::OTHER, "rule2"));  // fallback to global
+  EXPECT_EQ("",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+          AutocompleteInput::BLANK, "rule3"));  // no global
+  EXPECT_EQ("",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+          AutocompleteInput::HOMEPAGE, "rule3"));  // no global
+  EXPECT_EQ("4-rule3-value",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+          AutocompleteInput::OTHER, "rule3"));  // exact match
+  EXPECT_EQ("",
+      OmniboxFieldTrial::GetConsequencesOfRuleInPageClassificationContext(
+          AutocompleteInput::OTHER, "rule4"));  // no rule at all
+}