New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 740701 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Provide a common helper and deduplicate test code that manually builds field trial cmdline args

Project Member Reported by lukasza@chromium.org, Jul 10 2017

Issue description

This bug is a follow-up for https://codereview.chromium.org/2946113002/diff/200001/content/public/test/test_utils.cc#newcode213

Let's use this bug to
1) discuss how a common helper should look like
2) land associated CLs
 
Status: Started (was: Untriaged)
I propose the following API:

class VariationParamsManager {
...
  // Appends command line switches to |command_line| in a way that mimics
  // SetVariationParams.
  //
  // This 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(
      base::CommandLine* command_line,
      const std::string& trial_name,
      const std::map<std::string, std::string>& param_values);

  // Appends command line switches to |command_line| in a way that mimics
  // SetVariationParamsWithFeatureAssociations.
  //
  // This 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 AppendVariationParamsWithFeatureAssociations(
      base::CommandLine* command_line,
      const std::string& trial_name,
      const std::map<std::string, std::string>& param_values,
      const std::set<std::string>& associated_features);

I think a new, separate API is needed.  I don't think VariationParamsManager::set_use_command_line_flags() helps.  This is because the usage patterns of non-cmdline VS cmdline usage are different.  In particular:

- cmdline usage typically requires overriding InProcessBrowserTest::SetUpCommandLine

- non-cmdline usage / constructing of VariationParamsManager typically happens from an override of ContentBrowserTest::SetUp
Hmmm... implementing the above requires access to ::switches::kEnableFeatures from content/public/common/content_switches.h, but currently the dependency from //components/variations -> //content/public/common/content_switches.h is forbidden (perhaps for a good reason - this dependency would seem kind of circular).
Cc: lukasza@chromium.org
Owner: ----
Status: Available (was: Started)
FWIW, I have a WIP CL @ https://chromium-review.googlesource.com/565224, but it has 2 open issues:
- it violates the dependency rule from #c2 above
- it gives up on removing raw reference to kForceFieldTrialParams in chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc (all other tests passed, but one of the tests here started hanging after my changes).
Components: Internals>Metrics
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d46ecdc00b7d228d987e20616f2001a987c1f65

commit 1d46ecdc00b7d228d987e20616f2001a987c1f65
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Jul 14 18:21:08 2017

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}
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/chrome/browser/plugins/flash_permission_browsertest.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/chrome/browser/tracing/chrome_tracing_delegate_browsertest.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/chrome/test/BUILD.gn
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/components/variations/BUILD.gn
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/components/variations/field_trial_config/field_trial_util.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/components/variations/field_trial_config/field_trial_util.h
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/components/variations/variations_params_manager.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/components/variations/variations_params_manager.h
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/DEPS
[delete] https://crrev.com/b6c97cd8784b662059cb5b04de50544298dd5130/content/browser/memory/DEPS
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/browser/memory/memory_condition_observer.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/browser/renderer_host/DEPS
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/ppapi_plugin/DEPS
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/public/test/test_utils.cc
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/public/test/test_utils.h
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/renderer/DEPS
[modify] https://crrev.com/1d46ecdc00b7d228d987e20616f2001a987c1f65/content/test/BUILD.gn

Project Member

Comment 6 by sheriffbot@chromium.org, Jul 16

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: -asvitk...@chromium.org
Owner: asvitk...@chromium.org
Status: Assigned (was: Untriaged)
Assigning to Alexei, as Gayane is leaving soon.  Is there something else to be done for this bug now that Gayane's change to build a command line in about:version has landed?
Owner: lukasza@chromium.org
Status: Fixed (was: Assigned)
I think this was fixed as part of comment 5? If there's still follow-up work we want to do, we can file new bugs.

Sign in to add a comment