Provide a common helper and deduplicate test code that manually builds field trial cmdline args |
||||||
Issue descriptionThis 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
,
Jul 10 2017
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).
,
Jul 10 2017
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).
,
Jul 11 2017
,
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
,
Jul 16
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
,
Jul 19
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?
,
Sep 25
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 |
||||||
Comment 1 by lukasza@chromium.org
, Jul 10 2017I 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