Skip to content

Commit

Permalink
Add Forcing Flag Support to Fieldtrial Testing Configurations
Browse files Browse the repository at this point in the history
This allows experiments to specify a command line that will override
the default experiment order and immediately select that experiment
instead.

BUG=637095

Review-Url: https://codereview.chromium.org/2420013003
Cr-Commit-Position: refs/heads/master@{#426005}
  • Loading branch information
robliao authored and Commit bot committed Oct 18, 2016
1 parent 2d7b828 commit 16ac6bb
Show file tree
Hide file tree
Showing 9 changed files with 133 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@
"field": "disable_features",
"type": "array",
"contents": { "type": "string"}
},
{
"field": "forcing_flag",
"type": "string"
}
]
}
Expand Down
25 changes: 24 additions & 1 deletion chrome/common/variations/variations_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <string>
#include <vector>

#include "base/command_line.h"
#include "base/feature_list.h"
#include "base/metrics/field_trial.h"
#include "base/strings/string_split.h"
Expand Down Expand Up @@ -61,6 +62,26 @@ void AssociateParamsFromExperiment(
}
}

// Chooses an experiment taking into account the command line. Defaults to
// picking the first experiment.
const FieldTrialTestingExperiment& ChooseExperiment(
const FieldTrialTestingExperiment experiments[],
size_t experiment_size) {
DCHECK_GT(experiment_size, 0ul);
const auto& command_line = *base::CommandLine::ForCurrentProcess();
size_t chosen_index = 0;
for (size_t i = 1; i < experiment_size && chosen_index == 0; ++i) {
const auto& experiment = experiments[i];
if (experiment.forcing_flag &&
command_line.HasSwitch(experiment.forcing_flag)) {
chosen_index = i;
break;
}
}
DCHECK_GT(experiment_size, chosen_index);
return experiments[chosen_index];
}

} // namespace

bool AssociateParamsFromString(const std::string& varations_string) {
Expand Down Expand Up @@ -116,7 +137,9 @@ void AssociateParamsFromFieldTrialConfig(const FieldTrialTestingConfig& config,
const FieldTrialTestingStudy& study = config.studies[i];
if (study.experiments_size > 0) {
AssociateParamsFromExperiment(
study.name, study.experiments[0], feature_list);
study.name,
ChooseExperiment(study.experiments, study.experiments_size),
feature_list);
} else {
DLOG(ERROR) << "Unexpected empty study: " << study.name;
}
Expand Down
51 changes: 44 additions & 7 deletions chrome/common/variations/variations_util_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <memory>
#include <utility>

#include "base/command_line.h"
#include "base/feature_list.h"
#include "base/macros.h"
#include "base/metrics/field_trial.h"
Expand All @@ -18,7 +19,7 @@ namespace chrome_variations {

class VariationsUtilTest : public ::testing::Test {
public:
VariationsUtilTest() : field_trial_list_(NULL) {}
VariationsUtilTest() : field_trial_list_(nullptr) {}

~VariationsUtilTest() override {
// Ensure that the maps are cleared between tests, since they are stored as
Expand Down Expand Up @@ -61,13 +62,16 @@ TEST_F(VariationsUtilTest, AssociateParamsFromFieldTrialConfig) {
const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params_0[] =
{{"x", "1"}, {"y", "2"}};
const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = {
{"TestGroup1", array_kFieldTrialConfig_params_0, 2, NULL, 0, NULL, 0},
{"TestGroup1", array_kFieldTrialConfig_params_0, 2, nullptr, 0,
nullptr, 0, nullptr},
};
const FieldTrialTestingExperimentParams array_kFieldTrialConfig_params_1[] =
{{"x", "3"}, {"y", "4"}};
const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_1[] = {
{"TestGroup2", array_kFieldTrialConfig_params_0, 2, NULL, 0, NULL, 0},
{"TestGroup2-2", array_kFieldTrialConfig_params_1, 2, NULL, 0, NULL, 0},
{"TestGroup2", array_kFieldTrialConfig_params_0, 2, nullptr, 0,
nullptr, 0, nullptr},
{"TestGroup2-2", array_kFieldTrialConfig_params_1, 2, nullptr, 0,
nullptr, 0, nullptr},
};
const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = {
{"TestTrial1", array_kFieldTrialConfig_experiments_0, 1},
Expand Down Expand Up @@ -103,11 +107,11 @@ TEST_F(VariationsUtilTest, AssociateFeaturesFromFieldTrialConfig) {
const char* disable_features[] = {"C", "D"};

const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = {
{"TestGroup1", NULL, 0, enable_features, 2, NULL, 0},
{"TestGroup1", nullptr, 0, enable_features, 2, nullptr, 0, nullptr},
};
const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_1[] = {
{"TestGroup2", NULL, 0, NULL, 0, disable_features, 2},
{"TestGroup2-2", NULL, 0, NULL, 0, NULL, 0},
{"TestGroup2", nullptr, 0, nullptr, 0, disable_features, 2, nullptr},
{"TestGroup2-2", nullptr, 0, nullptr, 0, nullptr, 0, nullptr},
};

const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = {
Expand Down Expand Up @@ -137,4 +141,37 @@ TEST_F(VariationsUtilTest, AssociateFeaturesFromFieldTrialConfig) {
EXPECT_TRUE(base::FieldTrialList::IsTrialActive("TestTrial2"));
}

TEST_F(VariationsUtilTest, AssociateForcingFlagsFromFieldTrialConfig) {
const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = {
{"TestGroup1", nullptr, 0, nullptr, 0, nullptr, 0, nullptr}
};
const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_1[] = {
{"TestGroup2", nullptr, 0, nullptr, 0, nullptr, 0, nullptr},
{"ForcedGroup2", nullptr, 0, nullptr, 0, nullptr, 0, "flag-2"},
};
const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_2[] = {
{"TestGroup3", nullptr, 0, nullptr, 0, nullptr, 0, nullptr},
{"ForcedGroup3", nullptr, 0, nullptr, 0, nullptr, 0, "flag-3"},
{"ForcedGroup3-2", nullptr, 0, nullptr, 0, nullptr, 0, "flag-3-2"},
};
const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = {
{"TestTrial1", array_kFieldTrialConfig_experiments_0, 1},
{"TestTrial2", array_kFieldTrialConfig_experiments_1, 2},
{"TestTrial3", array_kFieldTrialConfig_experiments_2, 3},
};
const FieldTrialTestingConfig kConfig = {
array_kFieldTrialConfig_studies, 3
};

base::CommandLine::ForCurrentProcess()->AppendSwitch("flag-2");
base::CommandLine::ForCurrentProcess()->AppendSwitch("flag-3");

base::FeatureList feature_list;
AssociateParamsFromFieldTrialConfig(kConfig, &feature_list);

EXPECT_EQ("TestGroup1", base::FieldTrialList::FindFullName("TestTrial1"));
EXPECT_EQ("ForcedGroup2", base::FieldTrialList::FindFullName("TestTrial2"));
EXPECT_EQ("ForcedGroup3", base::FieldTrialList::FindFullName("TestTrial3"));
}

} // namespace chrome_variations
4 changes: 4 additions & 0 deletions testing/variations/PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from collections import OrderedDict

VALID_EXPERIMENT_KEYS = ['name',
'forcing_flag',
'params',
'enable_features',
'disable_features',
Expand Down Expand Up @@ -50,6 +51,7 @@ def PrettyPrint(contents):
# 'groups': [
# {
# name: ...
# forcing_flag: "forcing flag string"
# params: {sorted dict}
# enable_features: [sorted features]
# disable_features: [sorted features]
Expand Down Expand Up @@ -78,6 +80,8 @@ def PrettyPrint(contents):
if comment_key in experiment:
ordered_experiment[comment_key] = experiment[comment_key]
ordered_experiment['name'] = experiment['name']
if 'forcing_flag' in experiment:
ordered_experiment['forcing_flag'] = experiment['forcing_flag']
if 'params' in experiment:
ordered_experiment['params'] = OrderedDict(
sorted(experiment['params'].items(), key=lambda t: t[0]))
Expand Down
3 changes: 3 additions & 0 deletions tools/variations/fieldtrial_to_struct.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ def _LoadFieldTrialConfig(filename, platform):

def _CreateExperiment(experiment_data):
experiment = {'name': experiment_data['name']}
forcing_flags_data = experiment_data.get('forcing_flag')
if forcing_flags_data:
experiment['forcing_flag'] = forcing_flags_data
params_data = experiment_data.get('params')
if (params_data):
experiment['params'] = [{'key': param, 'value': params_data[param]}
Expand Down
20 changes: 20 additions & 0 deletions tools/variations/fieldtrial_to_struct_unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,17 @@ def test_FieldTrialToDescription(self):
'platforms': ['win'],
'experiments': [{'name': 'OtherGroup'}]
}
],
'TrialWithForcingFlag': [
{
'platforms': ['win'],
'experiments': [
{
'name': 'ForcedGroup',
'forcing_flag': "my-forcing-flag"
}
]
}
]
}
result = fieldtrial_to_struct._FieldTrialConfigToDescription(config, 'win')
Expand Down Expand Up @@ -76,6 +87,15 @@ def test_FieldTrialToDescription(self):
'name': 'Trial2',
'experiments': [{'name': 'OtherGroup'}]
},
{
'name': 'TrialWithForcingFlag',
'experiments': [
{
'name': 'ForcedGroup',
'forcing_flag': "my-forcing-flag"
}
]
},
]
}
}
Expand Down
23 changes: 22 additions & 1 deletion tools/variations/unittest_data/expected_output.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@
#include "test_output.h"


const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_2[] = {
{
"ForcedGroup",
NULL,
0,
NULL,
0,
NULL,
0,
"my-forcing-flag",
},
};
const char* const array_kFieldTrialConfig_enable_features_1[] = {
"X",
};
Expand All @@ -22,6 +34,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_1[] = {
1,
NULL,
0,
NULL,
},
};
const char* const array_kFieldTrialConfig_disable_features_0[] = {
Expand Down Expand Up @@ -67,6 +80,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = {
2,
array_kFieldTrialConfig_disable_features,
1,
NULL,
},
{
"TestGroup2-2",
Expand All @@ -76,6 +90,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments_0[] = {
2,
array_kFieldTrialConfig_disable_features_0,
1,
NULL,
},
};
const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = {
Expand All @@ -87,6 +102,7 @@ const FieldTrialTestingExperiment array_kFieldTrialConfig_experiments[] = {
0,
NULL,
0,
NULL,
},
};
const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = {
Expand All @@ -105,8 +121,13 @@ const FieldTrialTestingStudy array_kFieldTrialConfig_studies[] = {
array_kFieldTrialConfig_experiments_1,
1,
},
{
"TrialWithForcingFlag",
array_kFieldTrialConfig_experiments_2,
1,
},
};
const FieldTrialTestingConfig kFieldTrialConfig = {
array_kFieldTrialConfig_studies,
3,
4,
};
1 change: 1 addition & 0 deletions tools/variations/unittest_data/expected_output.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ struct FieldTrialTestingExperiment {
const size_t enable_features_size;
const char* const * disable_features;
const size_t disable_features_size;
const char* const forcing_flag;
};

struct FieldTrialTestingStudy {
Expand Down
11 changes: 11 additions & 0 deletions tools/variations/unittest_data/test_config.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,16 @@
"platforms": ["win"],
"experiments": [{"name": "TestGroup3", "enable_features": ["X"]}]
}
],
"TrialWithForcingFlag": [
{
"platforms": ["win"],
"experiments": [
{
"name": "ForcedGroup",
"forcing_flag": "my-forcing-flag"
}
]
}
]
}

0 comments on commit 16ac6bb

Please sign in to comment.