Find annotated tests by exposing API in instrumentation_test_instance

build/android/pylib/instrumentation/instrumentation_test_instance.py

Index: build/android/pylib/instrumentation/instrumentation_test_instance.py
diff --git a/build/android/pylib/instrumentation/instrumentation_test_instance.py b/build/android/pylib/instrumentation/instrumentation_test_instance.py
index cd8617cfc15f7bb84fa79a60caa1a71261dd2de9..c4fc4953770f3a2b37848b980f27dc225e1ce09b 100644
--- a/build/android/pylib/instrumentation/instrumentation_test_instance.py
+++ b/build/android/pylib/instrumentation/instrumentation_test_instance.py
@@ -137,7 +137,6 @@ def GenerateTestResults(
 
   return results
 
-

[perezju, 2016/04/12 10:29:02]

nit: bring that blank line back

[Yoland Yan(Google), 2016/04/13 01:03:10]

Done.

 def ParseCommandLineFlagParameters(annotations):
   """Determines whether the test is parameterized to be run with different
      command-line flags.
@@ -187,6 +186,150 @@ def ParseCommandLineFlagParameters(annotations):
         result.append(ParamsTuple(to_add, to_remove))
   return result if result else None
 

[perezju, 2016/04/12 10:29:03]

nit: two blank lines between module level elements.

[Yoland Yan(Google), 2016/04/13 01:03:11]

Done.

+def TotalTestCount(test_jar):
+  """Return the total amount of tests in a test apk base on proguard dump"""
+  total_test_count = 0
+  tests = _GetAllTests(test_jar)
+  for c in tests:
+    total_test_count += len(c['methods'])
+  return total_test_count

[perezju, 2016/04/12 10:29:03]

nit: return sum(len(c['methods']) for c in _GetAllTests(test_jar))

[Yoland Yan(Google), 2016/04/13 01:03:10]

Done.

+
+def GetFilteredTests(test_jar, test_filter, annotations, exclude_annotations):

[perezju, 2016/04/12 10:29:03]

make test_filter, annotations, and exclude_annotations optional

[Yoland Yan(Google), 2016/04/13 01:03:10]

No longer an issue, but changed the now public FilterTests with optional
variables

+  """Get a list of filtered tests in a test apk jar file.
+
+  Args:
+    test_jar: the full path to the test apk jar file.
+    test_filter: googletest-style filter string.
+    annotations: a dict of wanted annotations for test methods.
+    exclude_annotations: a dict of annotations to exclude.

[perezju, 2016/04/12 10:29:03]

Why are these dicts? How do they look like?

[Yoland Yan(Google), 2016/04/13 01:03:11]

No longer an issue here in the next patch because I got rid of GetFilteredTests

but it's because the annotations has their values, I assume it's to be more
specific when it comes to excluding annotations (e.g. exclude
Restriction(RESTRICTION_TYPE_INTERNET) only), and exclude_annotations would be
{'Restriction': {'value': ['Internet']}}

[perezju, 2016/04/25 12:45:07]

I think that's a bit awkward to use. Probably the caller should do the filtering
if it needs anything more fancy. But now I see that this interface was like that
before your CL, so let's not change it.

+
+  Return:
+    A list of filtered tests

[perezju, 2016/04/12 10:29:03]

Add an example of what each such "test" looks like.

[Yoland Yan(Google), 2016/04/13 01:03:11]

Done.

+    """

[perezju, 2016/04/12 10:29:03]

nit: move those quotes three spaces to the left.

[Yoland Yan(Google), 2016/04/13 01:03:10]

Done.

+  tests = _GetAllTests(test_jar)
+  filtered_tests = _FilterTests(tests, test_filter, annotations,
+                               exclude_annotations)

[perezju, 2016/04/12 10:29:03]

nit: one space to the right

[Yoland Yan(Google), 2016/04/13 01:03:10]

Done.

+  return filtered_tests
+
+def _GetTestsFromPickle(pickle_path, jar_path):
+  if not os.path.exists(pickle_path):
+    raise ProguardPickleException('%s does not exist.' % pickle_path)
+  if os.path.getmtime(pickle_path) <= os.path.getmtime(jar_path):
+    raise ProguardPickleException(
+        '%s newer than %s.' % (jar_path, pickle_path))
+
+  with open(pickle_path, 'r') as pickle_file:
+    pickle_data = pickle.loads(pickle_file.read())
+  jar_md5 = md5sum.CalculateHostMd5Sums(jar_path)[jar_path]
+
+  try:
+    if pickle_data['VERSION'] != _PICKLE_FORMAT_VERSION:
+      raise ProguardPickleException('PICKLE_FORMAT_VERSION has changed.')
+    if pickle_data['JAR_MD5SUM'] != jar_md5:
+      raise ProguardPickleException('JAR file MD5 sum differs.')
+    return pickle_data['TEST_METHODS']
+  except TypeError as e:

[perezju, 2016/04/12 10:29:03]

what are you hoping to catch with TypeError?

[Yoland Yan(Google), 2016/04/13 01:03:10]

If from before, I think might have been some calculation that happened in the
try block could have cause TypeError

Deleted

+    logging.error(pickle_data)
+    raise ProguardPickleException(str(e))
+
+# pylint: disable=no-self-use

[perezju, 2016/04/12 10:29:03]

not needed anymore

[Yoland Yan(Google), 2016/04/13 01:03:11]

Done.

+def _GetTestsFromProguard(jar_path):
+  p = proguard.Dump(jar_path)
+
+  def is_test_class(c):
+    return c['class'].endswith('Test')
+
+  def is_test_method(m):
+    return m['method'].startswith('test')
+
+  class_lookup = dict((c['class'], c) for c in p['classes'])

[perezju, 2016/04/12 10:29:03]

nit: {c['class']: c for c in p['classes']}

and move this line next to the p = ... above.

[Yoland Yan(Google), 2016/04/13 01:03:11]

Done.

+  def recursive_get_class_annotations(c):

[perezju, 2016/04/12 10:29:03]

nit: recursive_class_annotations should be a fine name

[Yoland Yan(Google), 2016/04/13 01:03:10]

Done.

+    s = c['superclass']
+    if s in class_lookup:
+      a = recursive_get_class_annotations(class_lookup[s])
+    else:
+      a = {}
+    a.update(c['annotations'])

[perezju, 2016/04/12 10:29:02]

you could write this non-recursively:

    a = {}
    while c is not None:
      a.update(c['annotations'])
      c = class_lookup.get(c['superclass'])
    return a

Hmm .. unless the order is important and you need a class to override the
annotations of its superclasses? Then just ignore my comment.

[Yoland Yan(Google), 2016/04/13 02:18:43]

I am not sure whether order is important, will look into it before commit

[perezju, 2016/04/25 12:45:07]

It's fine, leave it as is.

+    return a
+
+  def stripped_test_class(c):
+    return {
+      'class': c['class'],
+      'annotations': recursive_get_class_annotations(c),
+      'methods': [m for m in c['methods'] if is_test_method(m)],
+    }
+
+  return [stripped_test_class(c) for c in p['classes']
+          if is_test_class(c)]
+
+def _SaveTestsToPickle(pickle_path, jar_path, tests):
+  jar_md5 = md5sum.CalculateHostMd5Sums(jar_path)[jar_path]
+  pickle_data = {
+    'VERSION': _PICKLE_FORMAT_VERSION,
+    'JAR_MD5SUM': jar_md5,
+    'TEST_METHODS': tests,
+  }
+  with open(pickle_path, 'w') as pickle_file:
+    pickle.dump(pickle_data, pickle_file)
+
+def _FilterTests(tests, test_filter, annotations, excluded_annotations):
+
+  def gtest_filter(c, m):
+    t = ['%s.%s' % (c['class'].split('.')[-1], m['method'])]
+    return (not test_filter
+            or unittest_util.FilterTestNames(t, test_filter))
+
+  def annotation_filter(all_annotations):
+    if not annotations:
+      return True
+    return any_annotation_matches(annotations, all_annotations)

[perezju, 2016/04/12 10:29:02]

Not particularly proud of this suggestion, but you could short-circuit the test
by doing something like:

    if annotations:
      annotation_filter = lambda found: any_annotation_matches(annotations,
found)
    else:
      annotation_filter = lambda _: True

[Yoland Yan(Google), 2016/04/13 01:03:11]

I did a timeit speed run, and the difference isn't big (7%) when annotations is
None
and in this case, annotation is always passed in. When test_runner is using it,
the self._annotation is most of time set as _DEFAULT_ANNOTATIONS.

I don't think it's worth to hurt the readability.

[perezju, 2016/04/13 09:53:46]

Acknowledged.

[Yoland Yan(Google), 2016/04/22 17:50:26]

Done.

+
+  def excluded_annotation_filter(all_annotations):
+    if not excluded_annotations:
+      return True
+    return not any_annotation_matches(excluded_annotations,
+                                      all_annotations)
+
+  def any_annotation_matches(annotations, all_annotations):
+    return any(
+        ak in all_annotations and (av is None or av == all_annotations[ak])
+        for ak, av in annotations.iteritems())
+
+  filtered_classes = []
+  for c in tests:
+    filtered_methods = []
+    for m in c['methods']:
+      # Gtest filtering
+      if not gtest_filter(c, m):
+        continue
+
+      all_annotations = dict(c['annotations'])
+      all_annotations.update(m['annotations'])
+      if (not annotation_filter(all_annotations)
+          or not excluded_annotation_filter(all_annotations)):
+        continue
+
+      filtered_methods.append(m)
+
+    if filtered_methods:
+      filtered_class = dict(c)
+      filtered_class['methods'] = filtered_methods
+      filtered_classes.append(filtered_class)
+
+  return filtered_classes
+
+def _GetAllTests(test_jar):
+  pickle_path = '%s-proguard.pickle' % test_jar
+  try:
+    tests = _GetTestsFromPickle(pickle_path, test_jar)
+  except ProguardPickleException as e:
+    logging.info('Getting tests from JAR via proguard. (%s)', str(e))

[perezju, 2016/04/12 10:29:03]

maybe:

    logging.info('Could not get tests from pickle: %s', e)
    logging.info('Getting tests from JAR via proguard.')

[Yoland Yan(Google), 2016/04/13 01:03:10]

Done.

+    tests = _GetTestsFromProguard(test_jar)
+    _SaveTestsToPickle(pickle_path, test_jar, tests)
+  return tests
+
+class ProguardPickleException(Exception):

[perezju, 2016/04/12 10:29:03]

Move declarations of exceptions near the top of the module.

[Yoland Yan(Google), 2016/04/13 01:03:10]

Done.

+  pass
 
 class InstrumentationTestInstance(test_instance.TestInstance):
 
@@ -465,125 +608,12 @@ class InstrumentationTestInstance(test_instance.TestInstance):
     return self._data_deps
 
   def GetTests(self):
-    pickle_path = '%s-proguard.pickle' % self.test_jar
-    try:
-      tests = self._GetTestsFromPickle(pickle_path, self.test_jar)
-    except self.ProguardPickleException as e:
-      logging.info('Getting tests from JAR via proguard. (%s)', str(e))
-      tests = self._GetTestsFromProguard(self.test_jar)
-      self._SaveTestsToPickle(pickle_path, self.test_jar, tests)
-    return self._ParametrizeTestsWithFlags(
-        self._InflateTests(self._FilterTests(tests)))
-
-  class ProguardPickleException(Exception):
-    pass
-
-  def _GetTestsFromPickle(self, pickle_path, jar_path):
-    if not os.path.exists(pickle_path):
-      raise self.ProguardPickleException('%s does not exist.' % pickle_path)
-    if os.path.getmtime(pickle_path) <= os.path.getmtime(jar_path):
-      raise self.ProguardPickleException(
-          '%s newer than %s.' % (jar_path, pickle_path))
-
-    with open(pickle_path, 'r') as pickle_file:
-      pickle_data = pickle.loads(pickle_file.read())
-    jar_md5 = md5sum.CalculateHostMd5Sums(jar_path)[jar_path]
-
-    try:
-      if pickle_data['VERSION'] != _PICKLE_FORMAT_VERSION:
-        raise self.ProguardPickleException('PICKLE_FORMAT_VERSION has changed.')
-      if pickle_data['JAR_MD5SUM'] != jar_md5:
-        raise self.ProguardPickleException('JAR file MD5 sum differs.')
-      return pickle_data['TEST_METHODS']
-    except TypeError as e:
-      logging.error(pickle_data)
-      raise self.ProguardPickleException(str(e))
-
-  # pylint: disable=no-self-use
-  def _GetTestsFromProguard(self, jar_path):
-    p = proguard.Dump(jar_path)
-
-    def is_test_class(c):
-      return c['class'].endswith('Test')
-
-    def is_test_method(m):
-      return m['method'].startswith('test')
-
-    class_lookup = dict((c['class'], c) for c in p['classes'])
-    def recursive_get_class_annotations(c):
-      s = c['superclass']
-      if s in class_lookup:
-        a = recursive_get_class_annotations(class_lookup[s])
-      else:
-        a = {}
-      a.update(c['annotations'])
-      return a
-
-    def stripped_test_class(c):
-      return {
-        'class': c['class'],
-        'annotations': recursive_get_class_annotations(c),
-        'methods': [m for m in c['methods'] if is_test_method(m)],
-      }
-
-    return [stripped_test_class(c) for c in p['classes']
-            if is_test_class(c)]
-
-  def _SaveTestsToPickle(self, pickle_path, jar_path, tests):
-    jar_md5 = md5sum.CalculateHostMd5Sums(jar_path)[jar_path]
-    pickle_data = {
-      'VERSION': _PICKLE_FORMAT_VERSION,
-      'JAR_MD5SUM': jar_md5,
-      'TEST_METHODS': tests,
-    }
-    with open(pickle_path, 'w') as pickle_file:
-      pickle.dump(pickle_data, pickle_file)
-
-  def _FilterTests(self, tests):
-
-    def gtest_filter(c, m):
-      t = ['%s.%s' % (c['class'].split('.')[-1], m['method'])]
-      return (not self._test_filter
-              or unittest_util.FilterTestNames(t, self._test_filter))
-
-    def annotation_filter(all_annotations):
-      if not self._annotations:
-        return True
-      return any_annotation_matches(self._annotations, all_annotations)
-
-    def excluded_annotation_filter(all_annotations):
-      if not self._excluded_annotations:
-        return True
-      return not any_annotation_matches(self._excluded_annotations,
-                                        all_annotations)
-
-    def any_annotation_matches(annotations, all_annotations):
-      return any(
-          ak in all_annotations and (av is None or av == all_annotations[ak])
-          for ak, av in annotations.iteritems())
-
-    filtered_classes = []
-    for c in tests:
-      filtered_methods = []
-      for m in c['methods']:
-        # Gtest filtering
-        if not gtest_filter(c, m):
-          continue
-
-        all_annotations = dict(c['annotations'])
-        all_annotations.update(m['annotations'])
-        if (not annotation_filter(all_annotations)
-            or not excluded_annotation_filter(all_annotations)):
-          continue
-
-        filtered_methods.append(m)
-
-      if filtered_methods:
-        filtered_class = dict(c)
-        filtered_class['methods'] = filtered_methods
-        filtered_classes.append(filtered_class)
-
-    return filtered_classes
+    filtered_tests = GetFilteredTests(
+        self.test_jar,
+        self._test_filter,
+        self._annotations,
+        self._excluded_annotations)
+    return self._ParametrizeTestsWithFlags(self._InflateTests(filtered_tests))
 
   def _InflateTests(self, tests):
     inflated_tests = []
@@ -641,4 +671,3 @@ class InstrumentationTestInstance(test_instance.TestInstance):
   def TearDown(self):
     if self._isolate_delegate:
       self._isolate_delegate.Clear()
-