Simplify CommandLine parameterizing J3 Instrumentation Tests

build/android/pylib/instrumentation/instrumentation_test_instance.py

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-import collections
 import copy
 import logging
 import os
 import pickle
 import re
 
 from devil.android import apk_helper
 from devil.android import md5sum
 from pylib import constants
 from pylib.base import base_test_result
@@ skipping 24 matching lines @@
     'org.chromium.test.driver.OnDeviceInstrumentationDriver.TestList')
 _EXTRA_DRIVER_TEST_LIST_FILE = (
     'org.chromium.test.driver.OnDeviceInstrumentationDriver.TestListFile')
 _EXTRA_DRIVER_TARGET_PACKAGE = (
     'org.chromium.test.driver.OnDeviceInstrumentationDriver.TargetPackage')
 _EXTRA_DRIVER_TARGET_CLASS = (
     'org.chromium.test.driver.OnDeviceInstrumentationDriver.TargetClass')
 _EXTRA_TIMEOUT_SCALE = (
     'org.chromium.test.driver.OnDeviceInstrumentationDriver.TimeoutScale')
 
-_PARAMETERIZED_TEST_ANNOTATION = 'ParameterizedTest'
-_PARAMETERIZED_TEST_SET_ANNOTATION = 'ParameterizedTest$Set'
+_SKIP_PARAMETERIZATION = 'SkipParameterization'
+_COMMANDLINE_PARAMETERIZATION = 'JUnit3CommandLineParameter'
 _NATIVE_CRASH_RE = re.compile('(process|native) crash', re.IGNORECASE)
 _CMDLINE_NAME_SEGMENT_RE = re.compile(
     r' with(?:out)? \{[^\}]*\}')
-_PICKLE_FORMAT_VERSION = 11
+_PICKLE_FORMAT_VERSION = 12
 
 
 class MissingSizeAnnotationError(test_exception.TestException):
   def __init__(self, class_name):
     super(MissingSizeAnnotationError, self).__init__(class_name +
         ': Test method is missing required size annotation. Add one of: ' +
         ', '.join('@' + a for a in _VALID_ANNOTATIONS))
 
 
 class TestListPickleException(test_exception.TestException):
@@ skipping 84 matching lines @@
                  and any(_NATIVE_CRASH_RE.search(l)
                          for l in result_bundle.itervalues()))
       if crashed:
         current_result.SetType(base_test_result.ResultType.CRASH)
 
     results.append(current_result)
 
   return results
 
 
-def ParseCommandLineFlagParameters(annotations):
-  """Determines whether the test is parameterized to be run with different
-     command-line flags.
-
-  Args:
-    annotations: The annotations of the test.
-
-  Returns:
-    If the test is parameterized, returns a list of named tuples
-    with lists of flags, e.g.:
-
-      [(add=['--flag-to-add']), (remove=['--flag-to-remove']), ()]
-
-    That means, the test must be run three times, the first time with
-    "--flag-to-add" added to command-line, the second time with
-    "--flag-to-remove" to be removed from command-line, and the third time
-    with default command-line args. If the same flag is listed both for adding
-    and for removing, it is left unchanged.
-
-    If the test is not parametrized, returns None.
-
-  """
-  ParamsTuple = collections.namedtuple('ParamsTuple', ['add', 'remove'])
-  parameterized_tests = []
-  if _PARAMETERIZED_TEST_SET_ANNOTATION in annotations:
-    if annotations[_PARAMETERIZED_TEST_SET_ANNOTATION]:
-      parameterized_tests = annotations[
-        _PARAMETERIZED_TEST_SET_ANNOTATION].get('tests', [])
-  elif _PARAMETERIZED_TEST_ANNOTATION in annotations:
-    parameterized_tests = [annotations[_PARAMETERIZED_TEST_ANNOTATION]]
-  else:
-    return None
-
-  result = []
-  for pt in parameterized_tests:
-    if not pt:
-      continue
-    for p in pt['parameters']:
-      if p['tag'] == _COMMAND_LINE_PARAMETER:
-        to_add = []
-        to_remove = []
-        for a in p.get('arguments', []):
-          if a['name'] == 'add':
-            to_add = ['--%s' % f for f in a['stringArray']]
-          elif a['name'] == 'remove':
-            to_remove = ['--%s' % f for f in a['stringArray']]
-        result.append(ParamsTuple(to_add, to_remove))
-  return result if result else None
-
-
 def FilterTests(tests, test_filter=None, annotations=None,
                 excluded_annotations=None):
   """Filter a list of tests
 
   Args:
     tests: a list of tests. e.g. [
            {'annotations": {}, 'class': 'com.example.TestA', 'methods':[]},
            {'annotations": {}, 'class': 'com.example.TestB', 'methods':[]}]
     test_filter: googletest-style filter string.
     annotations: a dict of wanted annotations for test methods.
@@ skipping 62 matching lines @@
       raise MissingSizeAnnotationError(GetTestName(t))
 
     if (not annotation_filter(t['annotations'])
         or not excluded_annotation_filter(t['annotations'])):
       continue
 
     filtered_tests.append(t)
 
   return filtered_tests
 
-

[jbudorick, 2017/07/13 18:45:53]

nit: please don't pull the second newline from between top-level functions.

[the real yoland, 2017/07/13 23:49:19]

Done

 def GetAllTestsFromJar(test_jar):
   pickle_path = '%s-proguard.pickle' % test_jar
   try:
     tests = _GetTestsFromPickle(pickle_path, test_jar)
   except TestListPickleException as e:
     logging.info('Could not get tests from pickle: %s', e)
     logging.info('Getting tests from JAR via proguard.')
     tests = _GetTestsFromProguard(test_jar)
     _SaveTestsToPickle(pickle_path, test_jar, tests)
   return tests
 
 
 def GetAllTestsFromApk(test_apk):
   pickle_path = '%s-dexdump.pickle' % test_apk
   try:
     tests = _GetTestsFromPickle(pickle_path, test_apk)
   except TestListPickleException as e:
     logging.info('Could not get tests from pickle: %s', e)
     logging.info('Getting tests from dex via dexdump.')
     tests = _GetTestsFromDexdump(test_apk)
     _SaveTestsToPickle(pickle_path, test_apk, tests)
   return tests
 
-
 def _GetTestsFromPickle(pickle_path, jar_path):
   if not os.path.exists(pickle_path):
     raise TestListPickleException('%s does not exist.' % pickle_path)
   if os.path.getmtime(pickle_path) <= os.path.getmtime(jar_path):
     raise TestListPickleException(
         '%s newer than %s.' % (jar_path, pickle_path))
 
   with open(pickle_path, 'r') as f:
     pickle_data = pickle.load(f)
   jar_md5 = md5sum.CalculateHostMd5Sums(jar_path)[jar_path]
 
   if pickle_data['VERSION'] != _PICKLE_FORMAT_VERSION:
     raise TestListPickleException('PICKLE_FORMAT_VERSION has changed.')
   if pickle_data['JAR_MD5SUM'] != jar_md5:
     raise TestListPickleException('JAR file MD5 sum differs.')
   return pickle_data['TEST_METHODS']
 
 
+# TODO(yolandyan): remove this once the test listing from java runner lands
 def _GetTestsFromProguard(jar_path):
   p = proguard.Dump(jar_path)
   class_lookup = dict((c['class'], c) for c in p['classes'])
 
   def is_test_class(c):
     return c['class'].endswith('Test')
 
   def is_test_method(m):
     return m['method'].startswith('test')
 
@@ skipping 91 matching lines @@
   This will include text to disambiguate between tests for which GetTestName
   would return the same name.
 
   Args:
     test: the instrumentation test dict.
     sep: the character(s) that should join the class name and the method name.
   Returns:
     The unique test name as a string.
   """
   display_name = GetTestName(test, sep=sep)
-  if 'flags' in test:
-    flags = test['flags']
-    if flags.add:
-      display_name = '%s with {%s}' % (display_name, ' '.join(flags.add))
-    if flags.remove:
-      display_name = '%s without {%s}' % (display_name, ' '.join(flags.remove))
+  if 'flags' in test and test['flags'] and test['flags'][0]:

[mikecase (-- gone --), 2017/07/11 18:16:18]

You could do something like...

if test.get('flags', [None])[0]:

...idk if that is cleaner actually. Your choice

[the real yoland, 2017/07/13 23:49:19]

Done

+    display_name = '%s with %s' % (display_name, ' '.join(test['flags']))

[jbudorick, 2017/07/13 18:45:53]

Why does the if statement check the first element in the list if you use the
entire thing here?

[the real yoland, 2017/07/13 23:49:19]

because if commandline annotation is set as @CommandLineParameter({"",
SOME_SWITCH}), we don't want to display the first test as "Test with ".

   return display_name
 
 
 class InstrumentationTestInstance(test_instance.TestInstance):
 
   def __init__(self, args, data_deps_delegate, error_func):
     super(InstrumentationTestInstance, self).__init__()
 
     self._additional_apks = []
     self._apk_under_test = None
@@ skipping 105 matching lines @@
       error_func('Unable to find test APK: %s' % self._test_apk.path)
     if not self._test_jar:
       logging.warning('Test jar not specified. Test runner will not have '
                       'Java annotation info available. May not handle test '
                       'timeouts correctly.')
     elif not os.path.exists(self._test_jar):
       error_func('Unable to find test JAR: %s' % self._test_jar)
 
     self._test_package = self._test_apk.GetPackageName()
     all_instrumentations = self._test_apk.GetAllInstrumentations()
-    junit3_runners = [
+    test_runners = [
         x for x in all_instrumentations if ('true' not in x.get(
             'chromium-junit4', ''))]
-    junit4_runners = [
+    test_runners_junit4 = [
         x for x in all_instrumentations if ('true' in x.get(
             'chromium-junit4', ''))]
 
-    if len(junit3_runners) > 1:
+    if len(test_runners) > 1:
       logging.warning('This test apk has more than one JUnit3 instrumentation')
-    if len(junit4_runners) > 1:
+    if len(test_runners_junit4) > 1:
       logging.warning('This test apk has more than one JUnit4 instrumentation')
 
     self._test_runner = (
-      junit3_runners[0]['android:name'] if junit3_runners else
+      test_runners[0]['android:name'] if test_runners else
       self.test_apk.GetInstrumentationName())
     self._test_runner_junit4 = (
-      junit4_runners[0]['android:name'] if junit4_runners else None)
+      test_runners_junit4[0]['android:name'] if test_runners_junit4 else None)
 
     self._package_info = None
     if self._apk_under_test:
       package_under_test = self._apk_under_test.GetPackageName()
       for package_info in constants.PACKAGE_INFO.itervalues():
         if package_under_test == package_info.package:
           self._package_info = package_info
           break
     if not self._package_info:
       logging.warning('Unable to find package info for %s', self._test_package)
@@ skipping 217 matching lines @@
         self._data_deps_delegate(self._runtime_deps_path))
 
   def GetDataDependencies(self):
     return self._data_deps
 
   def GetTests(self):
     if self.test_jar:
       tests = GetAllTestsFromJar(self.test_jar)
     else:
       tests = GetAllTestsFromApk(self.test_apk.path)
-    inflated_tests = self._ParametrizeTestsWithFlags(self._InflateTests(tests))
+    inflated_tests = self._ParameterizeTestsWithFlags(self._InflateTests(tests))
     if self._test_runner_junit4 is None and any(
         t['is_junit4'] for t in inflated_tests):
       raise MissingJUnit4RunnerException()
     filtered_tests = FilterTests(
         inflated_tests, self._test_filter, self._annotations,
         self._excluded_annotations)
     if self._test_filter and not filtered_tests:
       for t in inflated_tests:
         logging.debug('  %s', GetUniqueTestName(t))
       raise UnmatchedFilterException(self._test_filter)
     return filtered_tests
 
   # pylint: disable=no-self-use
   def _InflateTests(self, tests):
     inflated_tests = []
     for c in tests:
       for m in c['methods']:
         a = dict(c['annotations'])
         a.update(m['annotations'])
         inflated_tests.append({
             'class': c['class'],
             'method': m['method'],
             'annotations': a,
             'is_junit4': c['superclass'] == 'java.lang.Object'
         })
     return inflated_tests
 
-  def _ParametrizeTestsWithFlags(self, tests):
+  def _ParameterizeTestsWithFlags(self, tests):
     new_tests = []
     for t in tests:
-      parameters = ParseCommandLineFlagParameters(t['annotations'])
+      annotations = t['annotations']
+      parameters = None
+      if annotations.get(_COMMANDLINE_PARAMETERIZATION):
+        parameters = (
+            annotations[_COMMANDLINE_PARAMETERIZATION]['value'] if

[jbudorick, 2017/07/13 18:45:53]

nit: format this as

  annotations...
  if ...
  else ...

Doing so makes this a bit more readable.

[the real yoland, 2017/07/13 23:49:18]

Done

+            _SKIP_PARAMETERIZATION not in annotations else None)
       if parameters:
-        t['flags'] = parameters[0]
+        t['flags'] = [parameters[0]]
         for p in parameters[1:]:
           parameterized_t = copy.copy(t)
-          parameterized_t['flags'] = p
+          parameterized_t['flags'] = [p]
           new_tests.append(parameterized_t)
     return tests + new_tests
 
   def GetDriverEnvironmentVars(
       self, test_list=None, test_list_file_path=None):
     env = {
       _EXTRA_DRIVER_TARGET_PACKAGE: self.test_package,
       _EXTRA_DRIVER_TARGET_CLASS: self.test_runner,
       _EXTRA_TIMEOUT_SCALE: self._timeout_scale,
     }
@@ skipping 13 matching lines @@
 
   @staticmethod
   def GenerateTestResults(
       result_code, result_bundle, statuses, start_ms, duration_ms):
     return GenerateTestResults(result_code, result_bundle, statuses,
                                start_ms, duration_ms)
 
   #override
   def TearDown(self):
     pass