Fix two bugs in split_variations_cmd.py

1) A bug introduced during addressing review feedbacks so now the split
files can't be split further because they are wrong.

2) The script doesn't work if one if the four switches are missing. This
is problematic because we could easily split into a state where certain
switches no longer have values.

Added unit tests to make sure the above two issues are covered.

BUG=932197
TEST=unittests
R=askitkine@chromium.org

Change-Id: Ic961f609f0a0d00ebf213206a70b4fb76b83aac8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1497797Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Zhenyao Mo <zmo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638262}

tools/variations/split_variations_cmd.py

@@ -173,7 +173,7 @@ def _SplitFieldTrials(trials, trial_params):
   params_second = []
   for trial in trials_second:
     if trial.trial_name in params:
-      params_first.append(params[trial.trial_name])
+      params_second.append(params[trial.trial_name])
 
   return [
     {_FORCE_FIELD_TRIALS_SWITCH_NAME: trials_first,
@@ -266,7 +266,7 @@ def ParseCommandLineSwitchesString(data):
   return switch_data
 
 
-def ParseVariationsCmd(data):
+def ParseVariationsCmdFromString(input_string):
   """Parses commandline switches string into internal representation.
 
   Commandline switches string comes from chrome://version/?show-variations-cmd.
@@ -276,7 +276,7 @@ def ParseVariationsCmd(data):
     --enable-features
     --disable-features
   """
-  switch_data = ParseCommandLineSwitchesString(data)
+  switch_data = ParseCommandLineSwitchesString(input_string)
   results = {}
   for switch_name, switch_value in switch_data.items():
     built_switch_value = None
@@ -310,12 +310,12 @@ def ParseVariationsCmd(data):
 def ParseVariationsCmdFromFile(filename):
   """Parses commandline switches string into internal representation.
 
-  Same as ParseVariationsCmd(), except the commandline switches string
-  comes from a file.
+  Same as ParseVariationsCmdFromString(), except the commandline switches
+  string comes from a file.
   """
   with open(filename, 'r') as f:
     data = f.read().replace('\n', ' ')
-  return ParseVariationsCmd(data)
+  return ParseVariationsCmdFromString(data)
 
 
 def VariationsCmdToStrings(data):
@@ -360,32 +360,15 @@ def VariationsCmdToStrings(data):
   return cmd_list
 
 
-def SaveVariationsCmdToFile(data, output_file):
-  """Saves internal representation into four commandline switches in a file.
-
-  This is the opposite of ParseVariationsCmdFromFile().
-
-  Args:
-      data: Input data dictionary. Keys are four commandline switches:
-        --force-fieldtrials
-        --force-fieldtrial-params
-        --enable-features
-        --disable-features
-      output_file: Output file object.
-  """
-  cmd_list = VariationsCmdToStrings(data)
-  output_file.write(' '.join(cmd_list))
-
-
 def SplitVariationsCmd(results):
   """Splits internal representation of commandline switches into two.
 
   The commandline switches come from chrome://version/?show-variations-cmd.
   """
-  enable_features = results[_ENABLE_FEATURES_SWITCH_NAME]
-  disable_features = results[_DISABLE_FEATURES_SWITCH_NAME]
-  field_trials = results[_FORCE_FIELD_TRIALS_SWITCH_NAME]
-  field_trial_params = results[_FORCE_FIELD_TRIAL_PARAMS_SWITCH_NAME]
+  enable_features = results.get(_ENABLE_FEATURES_SWITCH_NAME, [])
+  disable_features = results.get(_DISABLE_FEATURES_SWITCH_NAME, [])
+  field_trials = results.get(_FORCE_FIELD_TRIALS_SWITCH_NAME, [])
+  field_trial_params = results.get(_FORCE_FIELD_TRIAL_PARAMS_SWITCH_NAME, [])
   enable_features_splits = _SplitFeatures(enable_features)
   disable_features_splits = _SplitFeatures(disable_features)
   field_trials_splits = _SplitFieldTrials(field_trials, field_trial_params)
@@ -399,31 +382,59 @@ def SplitVariationsCmd(results):
   return splits
 
 
+def SplitVariationsCmdFromString(input_string):
+  """Splits commandline switches into two.
+
+  Same as SplitVariationsCmd(), except data comes from a string rather than
+  an internal representation.
+
+  Args:
+      input_string: Variations string to be split.
+
+  Returns:
+      A list of two strings, each is half of the variations cmd.
+  """
+  data = ParseVariationsCmdFromString(input_string)
+  splits = SplitVariationsCmd(data)
+  results = []
+  for split in splits:
+    cmd_list = VariationsCmdToStrings(split)
+    results.append(' '.join(cmd_list))
+  return results
+
+
 def SplitVariationsCmdFromFile(input_filename, output_dir=None):
-  """Splits internal representation of commandline switches into two.
+  """Splits commandline switches into two.
 
   Same as SplitVariationsCmd(), except data comes from a file rather than
   an internal representation.
 
-  If |output_dir| is None, output results to the same dir as |input_filename|.
+  Args:
+      input_filename: Variations file to be split.
+      output_dir: Folder to output the two split variations files. If None,
+          output to the same folder as the input_filename. If the folder
+          doesn't exist, it will be created.
 
-  It will be created if |output_dir| doesn't exist.
+  Returns:
+      A list of two output file names.
   """
-  results = ParseVariationsCmdFromFile(input_filename)
-  if not results:
-    return
-  runs = SplitVariationsCmd(results)
+  with open(input_filename, 'r') as f:
+    input_string = f.read().replace('\n', ' ')
+  splits = SplitVariationsCmdFromString(input_string)
   dirname, filename = os.path.split(input_filename)
   basename, ext = os.path.splitext(filename)
   if output_dir is None:
     output_dir = dirname
   if not os.path.exists(output_dir):
     os.makedirs(output_dir)
-  for index in range(len(runs)):
+  split_filenames = []
+  for index in range(len(splits)):
     output_filename = "%s_%d%s" % (basename, index + 1, ext)
     output_filename = os.path.join(output_dir, output_filename)
     with open(output_filename, 'w') as output_file:
-      SaveVariationsCmdToFile(runs[index], output_file)
+      output_file.write(splits[index])
+    split_filenames.append(output_filename)
+  return split_filenames
 
 
 def main():

tools/variations/split_variations_cmd_unittest.py

@@ -59,6 +59,7 @@ class SplitVariationsCmdUnittest(unittest.TestCase):
     data_lists = [
         split[switch_name] for split in splits if switch_name in split]
     if len(data_lists) == 0:
+      self.assertFalse(ref_switch_data)
       return
     max_size = max(len(data) for data in data_lists)
     min_size = min(len(data) for data in data_lists)
@@ -69,6 +70,7 @@ class SplitVariationsCmdUnittest(unittest.TestCase):
       joined_switch_data.extend(data)
     self.assertEqual(ref_switch_data, joined_switch_data)
 
+
   def testLoadFromFileAndSaveToStrings(self):
     # Verifies we load data from the file and save it to a list of strings,
     # the two data sets contain the same command line switches.
@@ -78,19 +80,46 @@ class SplitVariationsCmdUnittest(unittest.TestCase):
     cmd_list = split_variations_cmd.VariationsCmdToStrings(data)
     self.assertTrue(self._CompareCommandLineSwitches(data_file, cmd_list))
 
-  def testSplitVariationsCmd(self):
+
+  def _testSplitVariationsCmdHelper(self, input_data):
     # Verifies we correctly and (almost) evenly split one set of command line
     # switches into two sets.
-    data_file = os.path.join(self._GetUnittestDataDir(), 'variations_cmd.txt')
-    assert os.path.isfile(data_file)
-    data = split_variations_cmd.ParseVariationsCmdFromFile(data_file)
-    splits = split_variations_cmd.SplitVariationsCmd(data)
+    splits = split_variations_cmd.SplitVariationsCmd(input_data)
     switches = [_ENABLE_FEATURES_SWITCH_NAME,
                 _DISABLE_FEATURES_SWITCH_NAME,
                 _FORCE_FIELD_TRIALS_SWITCH_NAME,
                 _FORCE_FIELD_TRIAL_PARAMS_SWITCH_NAME]
     for switch in switches:
-      self._VerifySplits(switch, splits, data[switch])
+      self._VerifySplits(switch, splits, input_data.get(switch, []))
+    # Verify both split variations are valid.
+    for variations_cmd in splits:
+      cmd_list = split_variations_cmd.VariationsCmdToStrings(variations_cmd)
+      split_variations_cmd.ParseVariationsCmdFromString(' '.join(cmd_list))
+
+
+  def testSplitVariationsCmd(self):
+    input_file = os.path.join(self._GetUnittestDataDir(), 'variations_cmd.txt')
+    assert os.path.isfile(input_file)
+    data = split_variations_cmd.ParseVariationsCmdFromFile(input_file)
+    self._testSplitVariationsCmdHelper(data)
+
+
+  def testSplitVariationsCmdWithMissingEnableDisableFeatures(self):
+    input_string = (
+        '--force-fieldtrials="Tria1/Disabled/*Trial2/Enabled/" '
+        '--force-fieldtrial-params="Trial2.Enabled:age/18/gender/male" '
+        '--disable-features="FeatureA<FeatureA"')
+    data = split_variations_cmd.ParseVariationsCmdFromString(input_string)
+    self._testSplitVariationsCmdHelper(data)
+
+
+  def testSplitVariationsCmdWithMissingForceFieldTrialParams(self):
+    input_string = (
+        '--force-fieldtrials="*Trial2/Enabled/" '
+        '--enable-features="FeatureA<FeatureA,FeatureB<FeatureB" '
+        '--disable-features="FeatureC<FeatureC,FeatureD<FeatureD"')
+    data = split_variations_cmd.ParseVariationsCmdFromString(input_string)
+    self._testSplitVariationsCmdHelper(data)
 
 
 if __name__ == '__main__':