Pyformat and lean up config validation logic.

Minor changes in warning messages, but no other logic changes.

Change-Id: I3641831df502de28caa707aad96f883b84cee50d
Reviewed-on: https://chromium-review.googlesource.com/c/1355979Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615615}

testing/variations/PRESUBMIT.py

@@ -13,21 +13,11 @@ import sys
 
 from collections import OrderedDict
 
-VALID_EXPERIMENT_KEYS = ['name',
-                         'forcing_flag',
-                         'params',
-                         'enable_features',
-                         'disable_features',
-                         '//0',
-                         '//1',
-                         '//2',
-                         '//3',
-                         '//4',
-                         '//5',
-                         '//6',
-                         '//7',
-                         '//8',
-                         '//9']
+VALID_EXPERIMENT_KEYS = [
+    'name', 'forcing_flag', 'params', 'enable_features', 'disable_features',
+    '//0', '//1', '//2', '//3', '//4', '//5', '//6', '//7', '//8', '//9'
+]
+
 
 def PrettyPrint(contents):
   """Pretty prints a fieldtrial configuration.
@@ -70,9 +60,9 @@ def PrettyPrint(contents):
     study = copy.deepcopy(config[key])
     ordered_study = []
     for experiment_config in study:
-      ordered_experiment_config = OrderedDict([
-          ('platforms', experiment_config['platforms']),
-          ('experiments', [])])
+      ordered_experiment_config = OrderedDict([('platforms',
+                                                experiment_config['platforms']),
+                                               ('experiments', [])])
       for experiment in experiment_config['experiments']:
         ordered_experiment = OrderedDict()
         for index in xrange(0, 10):
@@ -94,9 +84,9 @@ def PrettyPrint(contents):
         ordered_experiment_config['experiments'].append(ordered_experiment)
       ordered_study.append(ordered_experiment_config)
     ordered_config[key] = ordered_study
-  return json.dumps(ordered_config,
-                    sort_keys=False, indent=4,
-                    separators=(',', ': ')) + '\n'
+  return json.dumps(
+      ordered_config, sort_keys=False, indent=4, separators=(',', ': ')) + '\n'
+
 
 def ValidateData(json_data, file_path, message_type):
   """Validates the format of a fieldtrial configuration.
@@ -105,79 +95,104 @@ def ValidateData(json_data, file_path, message_type):
     json_data: Parsed JSON object representing the fieldtrial config.
     file_path: String representing the path to the JSON file.
     message_type: Type of message from |output_api| to return in the case of
-        errors/warnings.
+      errors/warnings.
 
   Returns:
     A list of |message_type| messages. In the case of all tests passing with no
     warnings/errors, this will return [].
   """
-  if not isinstance(json_data, dict):
+
+  def _CreateMessage(message_format, *args):
     return _CreateMalformedConfigMessage(message_type, file_path,
-                                         'Expecting dict')
+                                         message_format, *args)
+
+  if not isinstance(json_data, dict):
+    return _CreateMessage('Expecting dict')
   for (study, experiment_configs) in json_data.iteritems():
-    if not isinstance(study, unicode):
-      return _CreateMalformedConfigMessage(message_type, file_path,
-          'Expecting keys to be string, got %s', type(study))
-    if not isinstance(experiment_configs, list):
-      return _CreateMalformedConfigMessage(message_type, file_path,
-          'Expecting list for study %s', study)
-    for experiment_config in experiment_configs:
-      if not isinstance(experiment_config, dict):
-        return _CreateMalformedConfigMessage(message_type, file_path,
-            'Expecting dict for experiment config in Study[%s]', study)
-      if not 'experiments' in experiment_config:
-        return _CreateMalformedConfigMessage(message_type, file_path,
-            'Missing valid experiments for experiment config in Study[%s]',
-            study)
-      if not isinstance(experiment_config['experiments'], list):
-        return _CreateMalformedConfigMessage(message_type, file_path,
-            'Expecting list for experiments in Study[%s]', study)
-      for experiment in experiment_config['experiments']:
-        if not 'name' in experiment or not isinstance(experiment['name'],
-                                                      unicode):
-          return _CreateMalformedConfigMessage(message_type, file_path,
-              'Missing valid name for experiment in Study[%s]', study)
-        if 'params' in experiment:
-          params = experiment['params']
-          if not isinstance(params, dict):
-            return _CreateMalformedConfigMessage(message_type, file_path,
-                'Expected dict for params for Experiment[%s] in Study[%s]',
-                experiment['name'], study)
-          for (key, value) in params.iteritems():
-            if not isinstance(key, unicode) or not isinstance(value, unicode):
-              return _CreateMalformedConfigMessage(message_type, file_path,
-                  'Invalid param (%s: %s) for Experiment[%s] in Study[%s]',
-                  key, value, experiment['name'], study)
-        for key in experiment.keys():
-          if key not in VALID_EXPERIMENT_KEYS:
-            return _CreateMalformedConfigMessage(message_type, file_path,
-                'Key[%s] in Experiment[%s] in Study[%s] is not a valid key.',
-                key, experiment['name'], study)
-      if not 'platforms' in experiment_config:
-        return _CreateMalformedConfigMessage(message_type, file_path,
-            'Missing valid platforms for experiment config in Study[%s]', study)
-      if not isinstance(experiment_config['platforms'], list):
-        return _CreateMalformedConfigMessage(message_type, file_path,
-            'Expecting list for platforms in Study[%s]', study)
-      supported_platforms = ['android', 'android_webview', 'chromeos', 'ios',
-                             'linux', 'mac', 'windows']
-      experiment_platforms = experiment_config['platforms']
-      unsupported_platforms = list(set(experiment_platforms).difference(
-                                       supported_platforms))
-      if unsupported_platforms:
-        return _CreateMalformedConfigMessage(message_type, file_path,
-                              'Unsupported platforms %s in Study[%s]',
-                              unsupported_platforms, study)
+    warnings = _ValidateEntry(study, experiment_configs, _CreateMessage)
+    if warnings:
+      return warnings
 
   return []
 
+
+def _ValidateEntry(study, experiment_configs, create_message_fn):
+  """Validates one entry of the field trial configuration."""
+  if not isinstance(study, unicode):
+    return create_message_fn('Expecting keys to be string, got %s', type(study))
+  if not isinstance(experiment_configs, list):
+    return create_message_fn('Expecting list for study %s', study)
+
+  # Add context to other messages.
+  def _CreateStudyMessage(message_format, *args):
+    suffix = ' in Study[%s]' % study
+    return create_message_fn(message_format + suffix, *args)
+
+  for experiment_config in experiment_configs:
+    warnings = _ValidateExperimentConfig(experiment_config, _CreateStudyMessage)
+    if warnings:
+      return warnings
+  return []
+
+
+def _ValidateExperimentConfig(experiment_config, create_message_fn):
+  """Validates one config in a configuration entry."""
+  if not isinstance(experiment_config, dict):
+    return create_message_fn('Expecting dict for experiment config')
+  if not 'experiments' in experiment_config:
+    return create_message_fn('Missing valid experiments for experiment config')
+  if not isinstance(experiment_config['experiments'], list):
+    return create_message_fn('Expecting list for experiments')
+  for experiment_group in experiment_config['experiments']:
+    warnings = _ValidateExperimentGroup(experiment_group, create_message_fn)
+    if warnings:
+      return warnings
+  if not 'platforms' in experiment_config:
+    return create_message_fn('Missing valid platforms for experiment config')
+  if not isinstance(experiment_config['platforms'], list):
+    return create_message_fn('Expecting list for platforms')
+  supported_platforms = [
+      'android', 'android_webview', 'chromeos', 'ios', 'linux', 'mac', 'windows'
+  ]
+  experiment_platforms = experiment_config['platforms']
+  unsupported_platforms = list(
+      set(experiment_platforms).difference(supported_platforms))
+  if unsupported_platforms:
+    return create_message_fn('Unsupported platforms %s', unsupported_platforms)
+  return []
+
+
+def _ValidateExperimentGroup(experiment_group, create_message_fn):
+  """Validates one group of one config in a configuration entry."""
+  name = experiment_group.get('name', '')
+  if not name or not isinstance(name, unicode):
+    return create_message_fn('Missing valid name for experiment')
+
+  # Add context to other messages.
+  def _CreateGroupMessage(message_format, *args):
+    suffix = ' in Group[%s]' % name
+    return create_message_fn(message_format + suffix, *args)
+
+  if 'params' in experiment_group:
+    params = experiment_group['params']
+    if not isinstance(params, dict):
+      return _CreateGroupMessage('Expected dict for params')
+    for (key, value) in params.iteritems():
+      if not isinstance(key, unicode) or not isinstance(value, unicode):
+        return _CreateGroupMessage('Invalid param (%s: %s)', key, value)
+  for key in experiment_group.keys():
+    if key not in VALID_EXPERIMENT_KEYS:
+      return _CreateGroupMessage('Key[%s] is not a valid key', key)
+  return []
+
+
 def _CreateMalformedConfigMessage(message_type, file_path, message_format,
                                   *args):
   """Returns a list containing one |message_type| with the error message.
 
   Args:
     message_type: Type of message from |output_api| to return in the case of
-        errors/warnings.
+      errors/warnings.
     message_format: The error message format string.
     file_path: The path to the config file.
     *args: The args for message_format.
@@ -190,6 +205,7 @@ def _CreateMalformedConfigMessage(message_type, file_path, message_format,
   format_args = (file_path,) + args
   return [message_type(error_message_format % format_args)]
 
+
 def CheckPretty(contents, file_path, message_type):
   """Validates the pretty printing of fieldtrial configuration.
 
@@ -197,7 +213,7 @@ def CheckPretty(contents, file_path, message_type):
     contents: File contents as a string.
     file_path: String representing the path to the JSON file.
     message_type: Type of message from |output_api| to return in the case of
-        errors/warnings.
+      errors/warnings.
 
   Returns:
     A list of |message_type| messages. In the case of all tests passing with no
@@ -205,11 +221,13 @@ def CheckPretty(contents, file_path, message_type):
   """
   pretty = PrettyPrint(contents)
   if contents != pretty:
-    return [message_type(
-        'Pretty printing error: Run '
-        'python testing/variations/PRESUBMIT.py %s' % file_path)]
+    return [
+        message_type('Pretty printing error: Run '
+                     'python testing/variations/PRESUBMIT.py %s' % file_path)
+    ]
   return []
 
+
 def CommonChecks(input_api, output_api):
   affected_files = input_api.AffectedFiles(
       include_deletes=False,
@@ -225,13 +243,16 @@ def CommonChecks(input_api, output_api):
       if len(result):
         return result
     except ValueError:
-      return [output_api.PresubmitError(
-          'Malformed JSON file: %s' % f.LocalPath())]
+      return [
+          output_api.PresubmitError('Malformed JSON file: %s' % f.LocalPath())
+      ]
   return []
 
+
 def CheckChangeOnUpload(input_api, output_api):
   return CommonChecks(input_api, output_api)
 
+
 def CheckChangeOnCommit(input_api, output_api):
   return CommonChecks(input_api, output_api)
 
@@ -241,5 +262,6 @@ def main(argv):
   pretty = PrettyPrint(content)
   open(argv[1], 'wb').write(pretty)
 
-if __name__ == "__main__":
+
+if __name__ == '__main__':
   sys.exit(main(sys.argv))