Shared test helpers for setting cmdline switches for field trial params.

This CL introduces the following helpers for setting field trial params
from test code:

content/public/test/test_util.h:

  // Appends command line switches to |command_line| to enable the
  // |feature| and to set field trial params associated with the feature
  // as specified by |param_name| and |param_value|.
  void EnableFeatureWithParam(const base::Feature& feature,
                              const std::string& param_name,
                              const std::string& param_value,
                              base::CommandLine* command_line);

components/variations/variations_params_manager.h:

    // Appends command line switches to |command_line| in a way that
    // mimics SetVariationParams.
    static void AppendVariationParams(
        const std::string& trial_name,
        const std::string& trial_group_name,
        const std::map<std::string, std::string>& param_values,
        base::CommandLine* command_line);


Change-Id: Ibdcd7931c2894b9d5d77334d286bf3142034591a
Bug: 740701
Tbr: bauerb@chromium.org, bartfab@chromium.org, nparker@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/565224
Commit-Queue: Lukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Reviewed-by: Charlie Reis (OOO until July 19) <creis@chromium.org>
Reviewed-by: Alexei Svitkine (slow) <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486814}

chrome/browser/plugins/flash_permission_browsertest.cc

@@ -2,21 +2,21 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/permissions/permissions_browsertest.h"
 #include "chrome/browser/ui/permission_bubble/mock_permission_prompt_factory.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/ui_test_utils.h"
-#include "components/variations/variations_switches.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/ppapi_test_utils.h"
+#include "content/public/test/test_utils.h"
 #include "third_party/WebKit/public/platform/WebInputEvent.h"
 #include "url/gurl.h"
 
@@ -55,13 +55,9 @@ class FlashPermissionBrowserTest : public PermissionsBrowserTest {
 
     // Set a high engagement threshhold so it doesn't interfere with testing the
     // permission.
-    command_line->AppendSwitchASCII(switches::kEnableFeatures,
-                                    "PreferHtmlOverPlugins<Study1");
-    command_line->AppendSwitchASCII(switches::kForceFieldTrials,
-                                    "Study1/Enabled/");
-    command_line->AppendSwitchASCII(
-        variations::switches::kForceFieldTrialParams,
-        "Study1.Enabled:engagement_threshold_for_flash/100");
+    content::EnableFeatureWithParam(features::kPreferHtmlOverPlugins,
+                                    "engagement_threshold_for_flash", "100",
+                                    command_line);
   }
 
   void TriggerPrompt() override {

chrome/browser/policy/policy_browsertest.cc

@@ -12,7 +12,6 @@
 #include <vector>
 
 #include "ash/accelerators/accelerator_controller_delegate_aura.h"
-#include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ -33,7 +32,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/scoped_feature_list.h"
 #include "base/test/test_file_util.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
@@ -132,7 +130,6 @@
 #include "components/update_client/url_request_post_interceptor.h"
 #include "components/user_prefs/user_prefs.h"
 #include "components/variations/service/variations_service.h"
-#include "components/variations/variations_switches.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -4384,21 +4381,9 @@ IN_PROC_BROWSER_TEST_F(ArcPolicyTest, ArcLocationServiceEnabled) {
 class NetworkTimePolicyTest : public PolicyTest {
  public:
   void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitchASCII(
-        switches::kForceFieldTrials,
-        "SSLNetworkTimeBrowserTestFieldTrial/Enabled/");
-    command_line->AppendSwitchASCII(
-        variations::switches::kForceFieldTrialParams,
-        "SSLNetworkTimeBrowserTestFieldTrial.Enabled:FetchBehavior/"
-        "on-demand-only");
-  }
-
-  void SetUpOnMainThread() override {
-    PolicyTest::SetUpOnMainThread();
-    scoped_feature_list_.InitFromCommandLine(
-        std::string(network_time::kNetworkTimeServiceQuerying.name) +
-            "<SSLNetworkTimeBrowserTestFieldTrial",
-        std::string());
+    content::EnableFeatureWithParam(network_time::kNetworkTimeServiceQuerying,
+                                    "FetchBehavior", "on-demand-only",
+                                    command_line);
   }
 
   // A request handler that returns a dummy response and counts the number of
@@ -4421,7 +4406,6 @@ class NetworkTimePolicyTest : public PolicyTest {
 
  private:
   uint32_t num_requests_ = 0;
-  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_F(NetworkTimePolicyTest, NetworkTimeQueriesDisabled) {

chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

@@ -23,6 +23,7 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browsing_data_remover.h"
+#include "content/public/test/test_utils.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/url_util.h"
 #include "net/dns/mock_host_resolver.h"
@@ -321,13 +322,9 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
   using URLRequestSummary = URLRequestSummary;
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitchASCII("force-fieldtrials", "trial/group");
-    std::string parameter = base::StringPrintf(
-        "trial.group:%s/%s", kModeParamName, kExternalPrefetchingMode);
-    command_line->AppendSwitchASCII("force-fieldtrial-params", parameter);
-    std::string enabled_feature = base::StringPrintf(
-        "%s<trial", kSpeculativeResourcePrefetchingFeatureName);
-    command_line->AppendSwitchASCII("enable-features", enabled_feature);
+    content::EnableFeatureWithParam(kSpeculativeResourcePrefetchingFeature,
+                                    kModeParamName, kExternalPrefetchingMode,
+                                    command_line);
   }
 
   void SetUpOnMainThread() override {

chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc

@@ -4,7 +4,6 @@
 
 #include "chrome/browser/safe_browsing/certificate_reporting_service.h"
 
-#include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/test/histogram_tester.h"
@@ -25,7 +24,7 @@
 #include "components/certificate_reporting/error_report.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
-#include "components/variations/variations_switches.h"
+#include "components/variations/variations_params_manager.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
@@ -108,13 +107,10 @@ class CertificateReportingServiceBrowserTest : public InProcessBrowserTest {
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitchASCII(
-        switches::kForceFieldTrials,
-        "ReportCertificateErrors/ShowAndPossiblySend/");
     // Setting the sending threshold to 1.0 ensures reporting is enabled.
-    command_line->AppendSwitchASCII(
-        variations::switches::kForceFieldTrialParams,
-        "ReportCertificateErrors.ShowAndPossiblySend:sendingThreshold/1.0");
+    variations::testing::VariationParamsManager::AppendVariationParams(
+        "ReportCertificateErrors", "ShowAndPossiblySend",
+        {{"sendingThreshold", "1.0"}}, command_line);
   }
 
   CertificateReportingServiceTestHelper* test_helper() { return &test_helper_; }

chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc

@@ -7,7 +7,6 @@
 #include <string>
 #include <utility>
 
-#include "base/base_switches.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
@@ -28,7 +27,7 @@
 #include "components/captive_portal/captive_portal_detector.h"
 #include "components/prefs/pref_service.h"
 #include "components/security_state/core/security_state.h"
-#include "components/variations/variations_switches.h"
+#include "components/variations/variations_params_manager.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
@@ -100,13 +99,10 @@ class CaptivePortalBlockingPageTest : public InProcessBrowserTest {
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitchASCII(
-        switches::kForceFieldTrials,
-        "ReportCertificateErrors/ShowAndPossiblySend/");
     // Setting the sending threshold to 1.0 ensures reporting is enabled.
-    command_line->AppendSwitchASCII(
-        variations::switches::kForceFieldTrialParams,
-        "ReportCertificateErrors.ShowAndPossiblySend:sendingThreshold/1.0");
+    variations::testing::VariationParamsManager::AppendVariationParams(
+        "ReportCertificateErrors", "ShowAndPossiblySend",
+        {{"sendingThreshold", "1.0"}}, command_line);
   }
 
   void TestInterstitial(bool is_wifi_connection,

chrome/browser/tracing/chrome_tracing_delegate_browsertest.cc

@@ -4,7 +4,6 @@
 
 #include <utility>
 
-#include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "build/build_config.h"
@@ -17,7 +16,7 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/metrics/metrics_pref_names.h"
 #include "components/prefs/pref_service.h"
-#include "components/variations/variations_switches.h"
+#include "components/variations/variations_params_manager.h"
 #include "content/public/browser/background_tracing_config.h"
 #include "content/public/browser/background_tracing_manager.h"
 #include "content/public/browser/browser_thread.h"
@@ -251,11 +250,9 @@ class ChromeTracingDelegateBrowserTestOnStartup
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-        switches::kForceFieldTrials, "BackgroundTracing/TestGroup/");
-    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-        variations::switches::kForceFieldTrialParams,
-        "BackgroundTracing.TestGroup:config/default_config_for_testing");
+    variations::testing::VariationParamsManager::AppendVariationParams(
+        "BackgroundTracing", "TestGroup",
+        {{"config", "default_config_for_testing"}}, command_line);
 
     tracing::SetConfigTextFilterForTesting(&FieldTrialConfigTextFilter);
   }

chrome/test/BUILD.gn

@@ -138,6 +138,7 @@ static_library("test_support") {
     "//components/sync_sessions:test_support",
     "//components/toolbar:test_support",
     "//components/update_client:test_support",
+    "//components/variations:test_support",
     "//components/web_resource:test_support",
     "//content/public/app:both",
     "//content/public/child",
@@ -3488,7 +3489,6 @@ test("unit_tests") {
     "//components/sync:test_support_driver",
     "//components/sync:test_support_model",
     "//components/sync_sessions:test_support",
-    "//components/variations:test_support",
     "//components/version_info:generate_version_info",
     "//components/webdata_services:test_support",
     "//content/app/resources",

components/variations/BUILD.gn

@@ -111,6 +111,7 @@ static_library("test_support") {
   ]
 
   deps = [
+    "field_trial_config:field_trial_config",
     "//base/test:test_support",
   ]
 }

components/variations/field_trial_config/field_trial_util.cc

@@ -21,7 +21,7 @@
 namespace variations {
 namespace {
 
-std::string EscapeValue(const std::string& value) {
+std::string UnescapeValue(const std::string& value) {
   return net::UnescapeURLComponent(
       value, net::UnescapeRule::PATH_SEPARATORS |
                  net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS);
@@ -83,6 +83,12 @@ const FieldTrialTestingExperiment& ChooseExperiment(
 
 } // namespace
 
+std::string EscapeValue(const std::string& value) {
+  // This needs to be the inverse of UnescapeValue in the anonymous namespace
+  // above.
+  return net::EscapeQueryParamValue(value, true /* use_plus */);
+}
+
 bool AssociateParamsFromString(const std::string& varations_string) {
   // Format: Trial1.Group1:k1/v1/k2/v2,Trial2.Group2:k1/v1/k2/v2
   std::set<std::pair<std::string, std::string>> trial_groups;
@@ -109,8 +115,8 @@ bool AssociateParamsFromString(const std::string& varations_string) {
       DLOG(ERROR) << "Param name and param value should be separated by '/'";
       return false;
     }
-    std::string trial = EscapeValue(group_parts[0]);
-    std::string group = EscapeValue(group_parts[1]);
+    std::string trial = UnescapeValue(group_parts[0]);
+    std::string group = UnescapeValue(group_parts[1]);
     auto trial_group = std::make_pair(trial, group);
     if (trial_groups.find(trial_group) != trial_groups.end()) {
       DLOG(ERROR) << base::StringPrintf(
@@ -121,8 +127,8 @@ bool AssociateParamsFromString(const std::string& varations_string) {
     trial_groups.insert(trial_group);
     std::map<std::string, std::string> params;
     for (size_t i = 0; i < key_values.size(); i += 2) {
-      std::string key = EscapeValue(key_values[i]);
-      std::string value = EscapeValue(key_values[i + 1]);
+      std::string key = UnescapeValue(key_values[i]);
+      std::string value = UnescapeValue(key_values[i + 1]);
       params[key] = value;
     }
     AssociateVariationParams(trial, group, params);

components/variations/field_trial_config/field_trial_util.h

@@ -15,6 +15,10 @@ namespace variations {
 
 struct FieldTrialTestingConfig;
 
+// Escapes the trial name, or parameter name, or parameter value in a way that
+// makes it usable within variations::switches::kForceFieldTrialParams.
+std::string EscapeValue(const std::string& value);
+
 // Provides a mechanism to associate multiple set of params to multiple groups
 // with a formatted string specified from commandline. See
 // kForceFieldTrialParams in components/variations/variations_switches.cc for

components/variations/variations_params_manager.cc

@@ -6,11 +6,14 @@
 
 #include <utility>
 
+#include "base/base_switches.h"
 #include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/test/scoped_feature_list.h"
+#include "components/variations/field_trial_config/field_trial_util.h"
 #include "components/variations/variations_associated_data.h"
+#include "components/variations/variations_switches.h"
 
 namespace variations {
 namespace testing {
@@ -91,5 +94,35 @@ void VariationParamsManager::ClearAllVariationParams() {
   field_trial_list_ = base::MakeUnique<base::FieldTrialList>(nullptr);
 }
 
+// static
+void VariationParamsManager::AppendVariationParams(
+    const std::string& trial_name,
+    const std::string& trial_group_name,
+    const std::map<std::string, std::string>& param_values,
+    base::CommandLine* command_line) {
+  // Register the trial group.
+  command_line->AppendSwitchASCII(
+      ::switches::kForceFieldTrials,
+      EscapeValue(trial_name) + "/" + EscapeValue(trial_group_name));
+
+  // Associate |param_values| with the trial group.
+  std::string params_arg =
+      EscapeValue(trial_name) + "." + EscapeValue(trial_group_name) + ":";
+  bool first = true;
+  for (const auto& param : param_values) {
+    // Separate each |param|.
+    if (!first)
+      params_arg += "/";
+    first = false;
+
+    // Append each |param|.
+    const std::string& name = param.first;
+    const std::string& value = param.second;
+    params_arg += EscapeValue(name) + "/" + EscapeValue(value);
+  }
+  command_line->AppendSwitchASCII(variations::switches::kForceFieldTrialParams,
+                                  params_arg);
+}
+
 }  // namespace testing
 }  // namespace variations

components/variations/variations_params_manager.h

@@ -14,6 +14,7 @@
 #include "base/metrics/field_trial.h"
 
 namespace base {
+class CommandLine;
 class FieldTrialList;
 
 namespace test {
@@ -70,6 +71,19 @@ class VariationParamsManager {
   // Clears all of the associated params.
   void ClearAllVariationParams();
 
+  // Appends command line switches to |command_line| in a way that mimics
+  // SetVariationParams.
+  //
+  // This static method is useful in situations where using
+  // VariationParamsManager directly would have resulted in initializing
+  // FieldTrialList twice (once from ChromeBrowserMainParts::SetupFieldTrials
+  // and once from VariationParamsManager).
+  static void AppendVariationParams(
+      const std::string& trial_name,
+      const std::string& trial_group_name,
+      const std::map<std::string, std::string>& param_values,
+      base::CommandLine* command_line);
+
  private:
   std::unique_ptr<base::FieldTrialList> field_trial_list_;
   std::unique_ptr<base::test::ScopedFeatureList> scoped_feature_list_;

content/DEPS

@@ -118,10 +118,20 @@ include_rules = [
   "+jni",
 ]
 
-# content -> content/shell dependency is not allowed, except for browser tests.
 specific_include_rules = {
   ".*_browsertest[a-z_]*\.(cc|h)": [
+    # content -> content/shell dependency is disallowed, except browser tests.
     "+content/shell/browser",
     "+content/shell/common",
+
+    # components/variations/variations_params_manager.h is allowed from tests
+    # (it is part of //components/variations:test_support target).
+    "+components/variations/variations_params_manager.h",
+  ],
+
+  "test_utils\.cc": [
+    # components/variations/variations_params_manager.h is allowed from tests
+    # (it is part of //components/variations:test_support target).
+    "+components/variations/variations_params_manager.h",
   ],
 }

content/browser/memory/DEPS

-include_rules = [
-  "+components/variations",
-]

content/browser/memory/memory_condition_observer.cc

@@ -6,7 +6,6 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
-#include "components/variations/variations_associated_data.h"
 #include "content/browser/memory/memory_monitor.h"
 
 namespace content {

content/browser/renderer_host/DEPS

@@ -3,7 +3,6 @@ include_rules = [
   "+components/viz/common",
   "+components/viz/host",
   "+components/viz/service",
-  "+components/variations",
   "+services/ui/public",
   "+third_party/zlib",
   "+ui/latency",

content/browser/renderer_host/render_view_host_impl.cc

@@ -29,7 +29,6 @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "cc/base/switches.h"
-#include "components/variations/variations_associated_data.h"
 #include "content/browser/bad_message.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"

content/ppapi_plugin/DEPS

 include_rules = [
   "+components/discardable_memory/client",
-  "+components/variations",
+  "+components/variations/child_process_field_trial_syncer.h",
   "+content/child",
   "+gin/public/isolate_holder.h",
   "+gin/v8_initializer.h",

content/public/test/test_utils.cc

@@ -19,6 +19,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "components/variations/variations_params_manager.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/url_schemes.h"
 #include "content/public/browser/browser_child_process_host_iterator.h"
@@ -212,6 +213,23 @@ void ResetSchemesAndOriginsWhitelist() {
   url::Initialize();
 }
 
+void EnableFeatureWithParam(const base::Feature& feature,
+                            const std::string& param_name,
+                            const std::string& param_value,
+                            base::CommandLine* command_line) {
+  static const char kFakeTrialName[] = "TrialNameForTesting";
+  static const char kFakeTrialGroupName[] = "TrialGroupForTesting";
+
+  // Enable all the |feature|, associating them with |trial_name|.
+  command_line->AppendSwitchASCII(
+      switches::kEnableFeatures,
+      std::string(feature.name) + "<" + kFakeTrialName);
+
+  std::map<std::string, std::string> param_values = {{param_name, param_value}};
+  variations::testing::VariationParamsManager::AppendVariationParams(
+      kFakeTrialName, kFakeTrialGroupName, param_values, command_line);
+}
+
 #if defined(OS_ANDROID)
 // Registers content/browser and mojo JNI bindings necessary for some types of
 // tests.

content/public/test/test_utils.h

@@ -26,8 +26,9 @@
 #endif
 
 namespace base {
-class Value;
 class CommandLine;
+class Value;
+struct Feature;
 }  // namespace base
 
 // A collection of functions designed for use with unit and browser tests.
@@ -87,6 +88,17 @@ void IsolateAllSitesForTesting(base::CommandLine* command_line);
 // Resets the internal secure schemes/origins whitelist.
 void ResetSchemesAndOriginsWhitelist();
 
+// Appends command line switches to |command_line| to enable the |feature| and
+// to set field trial params associated with the feature as specified by
+// |param_name| and |param_value|.
+//
+// Note that a dummy trial and trial group will be registered behind the scenes.
+// See also variations::testing::VariationsParamsManager class.
+void EnableFeatureWithParam(const base::Feature& feature,
+                            const std::string& param_name,
+                            const std::string& param_value,
+                            base::CommandLine* command_line);
+
 #if defined(OS_ANDROID)
 // Registers content/browser JNI bindings necessary for some types of tests.
 bool RegisterJniForTesting(JNIEnv* env);

content/renderer/DEPS

@@ -7,7 +7,7 @@ include_rules = [
   "+components/payments",
   "+components/scheduler",
   "+components/url_formatter",
-  "+components/variations",
+  "+components/variations/child_process_field_trial_syncer.h",
   "+components/viz/client",
   "+components/viz/common",
 

content/test/BUILD.gn

@@ -234,6 +234,7 @@ static_library("test_support") {
   ]
 
   public_deps = [
+    "//components/variations:test_support",
     "//content/public/app:both",
     "//content/public/browser",
     "//content/public/common",