Find annotated tests by exposing API in instrumentation_test_instance

tools/android/find_disabled_tests.py

Index: tools/android/find_disabled_tests.py
diff --git a/tools/android/find_disabled_tests.py b/tools/android/find_disabled_tests.py
new file mode 100755
index 0000000000000000000000000000000000000000..30f43a4f28c8416fe8d30bc50ac5640677f3a7a0
--- /dev/null
+++ b/tools/android/find_disabled_tests.py
@@ -0,0 +1,202 @@
+#!/usr/bin/env python

[jbudorick, 2016/04/28 14:17:14]

Can you move this to a subdirectory of tools/android/?

[Yoland Yan(Google), 2016/04/29 19:51:43]

Will move to tools/android/find_annotated_tests/find_annotated_tests?

+# Copyright (c) 2016 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.
+
+"""Finds all the tracked(disabled/flaky) tests from proguard dump"""
+
+import argparse
+import datetime
+import json
+import linecache
+import logging
+import os
+import pprint
+import re
+import sys
+import time
+
+_SRC_DIR = os.path.abspath(os.path.join(
+      os.path.dirname(__file__), '..', '..'))
+
+sys.path.append(os.path.join(_SRC_DIR, 'build', 'android'))
+

[jbudorick, 2016/04/28 14:17:15]

nit: no blank line here

[Yoland Yan(Google), 2016/04/29 19:51:42]

Done.

+from pylib import constants
+from pylib.instrumentation import instrumentation_test_instance
+
+sys.path.append(os.path.join(_SRC_DIR, 'third_party', 'catapult', 'devil'))
+from devil.utils import cmd_helper
+
+_GIT_TIME_FORMAT = '%Y-%m-%dT%H:%M:%S'
+_EXPORT_TIME_FORMAT = '%Y%m%dT%H%M%S'
+
+class _JSONKeyName(object):

[jbudorick, 2016/04/28 14:17:14]

I didn't read the earlier reviews, so perhaps this is contradicting someone
else, but I think these constants make the code that generates the JSON _less_
readable.

[perezju, 2016/04/28 14:33:58]

I've been saying the same thing for several reviews now! :P

[Yoland Yan(Google), 2016/04/29 19:51:43]

wow, what a bunch of bullies
lol
Done

+  TEST_MASTER_KEY = 'tests'
+  REPORT_MASTER_KEY = 'metadata'
+  REVISION_KEY = 'revision'
+  COMMIT_POS_KEY = 'commit_pos'
+  ANNOTATIONS_KEY = 'annotations'
+  TEST_NAME_KEY = 'test_name'
+  TEST_APK_KEY = 'test_apk_name'
+  CRBUG_KEY = 'bug_id'
+  CLASS_PATH_KEY = 'class_path'
+  CLASS_NAME_KEY = 'class_name'
+  TRACKED_TEST_COUNT_KEY = 'tracked_test_count'
+  DISABLED_TEST_COUNT_KEY = 'disabled_test_count'
+  FLAKY_TEST_COUNT_KEY = 'flaky_test_count'
+  TOTAL_TEST_COUNT_KEY = 'total_test_count'
+  UTC_BUILDTIME_KEY = 'build_time'
+  UTC_REVISIONTIME_KEY = 'revision_time'
+  PLATFORM_KEY = 'platform'
+  PLATFORM_VALUE = 'android'
+
+
+def _GetBugId(message):
+  """Validate bug message format and get bug id"""
+  result = re.search(r'crbug(?:.com)?/(\d+)', message)

[jbudorick, 2016/04/28 14:17:15]

compile this into a module-scope constant

[Yoland Yan(Google), 2016/04/29 19:51:42]

Done.

+  if result:
+    return int(result.group(1))
+  else:
+    return None
+

[jbudorick, 2016/04/28 14:17:14]

nit: two lines

[Yoland Yan(Google), 2016/04/29 19:51:43]

Done.

+def _GetAnnotations(test_annotations):
+  """Store annotations in the existing anntation_dict and return bug id"""
+  bug_id = None
+  annotations_dict = {}
+  for annotation, content in test_annotations.iteritems():
+    if content is not None and content.get('message') is not None:
+        bug_id = _GetBugId(content.get('message'))
+    annotations_dict.update({annotation: content})
+  return bug_id, annotations_dict
+
+
+def _GetTests(test_apks):
+  """All the all the tests"""

[jbudorick, 2016/04/28 14:17:14]

nit: "All the all the tests"?

[Yoland Yan(Google), 2016/04/29 19:51:43]

got it s/All the all the tests/All the all the all/g
Done

+  result = []
+  total_test_count = 0
+  for test_apk in test_apks:
+    logging.info('Current test apk: %s', test_apk)
+    test_jar = os.path.join(
+        constants.GetOutDirectory(), constants.SDK_BUILD_TEST_JAVALIB_DIR,
+        '%s.jar' % test_apk)
+    all_tests = instrumentation_test_instance.GetAllTests(test_jar=test_jar)
+    for test_class in all_tests:
+      class_path = test_class['class']
+      class_name = test_class['class'].split('.')[-1]
+
+      bug_id, class_annotation = _GetAnnotations(test_class['annotations'])

[perezju, 2016/04/27 09:29:15]

hmm.. looks like you always end up throwing away this bug_id from the class.
(It's overridden below)

[Yoland Yan(Google), 2016/04/29 19:51:43]

Done.

[Yoland Yan(Google), 2016/04/29 19:51:43]

Changed to class_bug_id and test_bug_id for cases where an entire class is
marked as disabled/flaky with an crbug

+      for test_method in test_class['methods']:
+        total_test_count += 1
+        # getting annotation of each test case
+        bug_id, test_annotations = _GetAnnotations(test_method['annotations'])
+        test_annotations.update(class_annotation)
+        # getting test method name of each test
+        test_name = test_method['method']
+        test_dict = {
+            _JSONKeyName.CRBUG_KEY: bug_id,

[jbudorick, 2016/04/28 14:17:15]

nit: 2 space indent

[Yoland Yan(Google), 2016/04/29 19:51:43]

Keep it as 4 after a serious discussion

+            _JSONKeyName.ANNOTATIONS_KEY: test_annotations,
+            _JSONKeyName.TEST_NAME_KEY: test_name,
+            _JSONKeyName.TEST_APK_KEY: test_apk,
+            _JSONKeyName.CLASS_NAME_KEY: class_name,
+            _JSONKeyName.CLASS_PATH_KEY: class_path
+        }
+        result.append(test_dict)
+  logging.info('Total count of tests in all test apks: %d', total_test_count)
+  return result, total_test_count
+
+
+def _GetReportMeta(utc_buildtime_string, total_test_count):
+  """Returns a dictionary of the report's metadata"""
+  revision = cmd_helper.GetCmdOutput(['git', 'rev-parse', 'HEAD']).strip()
+  raw_string = cmd_helper.GetCmdOutput(
+      ['git', 'log', '--pretty=format:%aI', '--max-count=1', 'HEAD'])

[perezju, 2016/04/27 09:29:15]

hmm, when I run this locally I get:

  2016-04-22T06:42:55-07:00

you probably need to parse that thing and convert to utc, right?

... (tries to figure out how to parse iso 8601 with tz offset in python) ...

wha!? how come there is no easy/standard way to do this!?

I guess the cleanest thing would be to use --pretty=format:%at to get the unix
timestamp, and then datetime.utcfromtimestamp(..)?

[Yoland Yan(Google), 2016/04/29 19:51:42]

Done.

+  git_log_time_pattern = r'\d+-\d+-\d+T\d+:\d+:\d+'

[perezju, 2016/04/27 09:29:15]

nit: a module level constant :)

you could also pre-compile it with re.compile

[jbudorick, 2016/04/28 14:17:15]

+1.

[Yoland Yan(Google), 2016/04/29 19:51:43]

Done.

+  time_string_search = re.search(git_log_time_pattern, raw_string)
+  if time_string_search is None:
+    raise Exception('Time format incorrect')

[jbudorick, 2016/04/28 14:17:15]

This error should be a little more verbose -- _why_ is it incorrect?

[Yoland Yan(Google), 2016/04/29 19:51:42]

Done.

+
+  raw_string = cmd_helper.GetCmdOutput(
+      ['git', 'log', '--pretty=format:%b', '--max-count=1', 'HEAD'])
+  git_log_message_pattern = r'Cr-Commit-Position: (.*)'

[perezju, 2016/04/27 09:29:15]

strip away the "refs/heads/master" blurb, and cast the commit position to an
int. See e.g.:
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave...

[jbudorick, 2016/04/28 14:17:15]

note that this should also be compiled into a module-scope constant

[Yoland Yan(Google), 2016/04/29 19:51:43]

Done.

+  commit_pos_search = re.search(git_log_message_pattern, raw_string)
+  if commit_pos_search is None:
+    raise Exception('Cr commit position is not found, potentially running with '

[jbudorick, 2016/04/28 14:17:14]

nit: this should say "Cr-Commit-Position ..." rather than "Cr commit position
..."

[Yoland Yan(Google), 2016/04/29 19:51:42]

Done.

+                    'uncommited HEAD')
+  commit_pos = commit_pos_search.group(1)
+
+  revision_time = time.strptime(time_string_search.group(0), _GIT_TIME_FORMAT)
+  utc_revision_time = datetime.datetime.utcfromtimestamp(
+      time.mktime(revision_time))
+  utc_revision_time = utc_revision_time.strftime(_EXPORT_TIME_FORMAT)
+  logging.info(
+      'revision is %s, revision time is %s', revision, utc_revision_time)
+
+  return {
+      _JSONKeyName.REVISION_KEY: revision,

[jbudorick, 2016/04/28 14:17:14]

nit: 2 space indent

[Yoland Yan(Google), 2016/04/29 19:51:43]

Keep as 4 space indent after serious discussion with +jbudorick

+      _JSONKeyName.COMMIT_POS_KEY: commit_pos,
+      _JSONKeyName.UTC_BUILDTIME_KEY: utc_buildtime_string,
+      _JSONKeyName.UTC_REVISIONTIME_KEY: utc_revision_time,
+      _JSONKeyName.PLATFORM_KEY: _JSONKeyName.PLATFORM_VALUE,
+      _JSONKeyName.TOTAL_TEST_COUNT_KEY: total_test_count}

[jbudorick, 2016/04/28 14:17:14]

nit: drop the trailing } onto the next line

[Yoland Yan(Google), 2016/04/29 19:51:43]

Done.

+
+
+def _GetReport(test_apks, buildtime_string):
+  """Generate the dictionary of report data
+
+  Args:
+    test_apks: a list of apks for search for tests
+    buildtime_string: the time when the script is run at
+                      format: '%Y%m%dT%H%M%S'
+  """
+
+  test_data, total_test_count = _GetTests(test_apks)
+  report_meta = _GetReportMeta(buildtime_string, total_test_count)
+  report_data = {
+      _JSONKeyName.REPORT_MASTER_KEY: report_meta,
+      _JSONKeyName.TEST_MASTER_KEY: test_data}
+  return report_data
+
+
+def main():
+  default_build_type = os.environ.get('BUILDTYPE', 'Debug')

[jbudorick, 2016/04/28 14:17:14]

please no

[Yoland Yan(Google), 2016/04/29 19:51:43]

but...lol (Explain: all this was added when the script was actually creating an
instrumentation test instance)
Done

+  parser = argparse.ArgumentParser()

[jbudorick, 2016/04/28 14:17:15]

Which of these are required?

[Yoland Yan(Google), 2016/04/29 19:51:43]

--test-apks, --json-output-dir

Done

+  parser.add_argument('-t', '--test-apks', nargs='+', dest='test_apks',

[jbudorick, 2016/04/28 14:17:15]

Why should this handle multiple APKs at once? Why wouldn't we just call it more
than once?

[Yoland Yan(Google), 2016/04/29 19:51:42]

Because the that would produce multiple JSON file instead of one. So the
metafile of these JSON would contain total_test_count to be just that test apk's
total test count. Also backend is currently implemented to take only one file
per commit.

This might become a problem if Cronet builder is run on a different bot.

+                      help='List all test apks file name that the script uses '
+                           'to fetch tracked tests from')
+  parser.add_argument(

[jbudorick, 2016/04/28 14:17:14]

nix both --debug and --release and instead support --output-directory.

[Yoland Yan(Google), 2016/04/29 19:51:42]

Done

+      '--debug', action='store_const', const='Debug', dest='build_type',
+      default=default_build_type,
+      help=('If set, run test suites under out/Debug. '
+            'Default is env var BUILDTYPE or Debug.'))
+  parser.add_argument(
+      '--release', action='store_const', const='Release', dest='build_type',
+      help=('If set, run test suites under out/Release. '
+            'Default is env var BUILDTYPE or Debug.'))
+  parser.add_argument('-o', '--output-path',
+                      help='JSON file output to be uploaded on to gcs')

[jbudorick, 2016/04/28 14:17:14]

This description is wrong; this is now the output directory.

[Yoland Yan(Google), 2016/04/29 19:51:42]

Done.

+  parser.add_argument('-v', '--verbose', action='store_true', default=False,

[jbudorick, 2016/04/28 14:17:14]

-v = INFO, -vv = DEBUG plz

[Yoland Yan(Google), 2016/04/29 19:51:43]

Done.

+                      help='DEBUG verbosity')
+
+  arguments = parser.parse_args(sys.argv[1:])
+  logging.basicConfig(
+      level=logging.DEBUG if arguments.verbose else logging.WARNING)
+  constants.SetBuildType(arguments.build_type)

[jbudorick, 2016/04/28 14:17:14]

Get rid of these two lines.

[Yoland Yan(Google), 2016/04/29 19:51:43]

Done

+  logging.info('Using jar from build type: %s', arguments.build_type)
+
+  buildtime = datetime.datetime.utcnow()

[jbudorick, 2016/04/28 14:17:15]

nit: buildtime is a bad name because this isn't when the APK was built.

[Yoland Yan(Google), 2016/04/29 19:51:42]

Changed to script_run_time
(I was considering changing it to the_time_which_this_very_sript_was_run_at but
that ends a variable name with a preposition)
Done

+  buildtime_string = buildtime.strftime(_EXPORT_TIME_FORMAT)
+  logging.info('Build time is %s', buildtime_string)
+  report_data = _GetReport(arguments.test_apks, buildtime_string)
+
+  if arguments.output_path is None:
+    output_path = constants.GetOutDirectory()
+  else:
+    output_path = arguments.output_path
+  json_output_path = os.path.join(output_path,
+                                  '%s-android-chrome.json' % buildtime_string)
+  with open(json_output_path, 'w') as f:
+    json.dump(report_data, f, indent=2, sort_keys=True, separators=(',',': '))

[jbudorick, 2016/04/28 14:17:14]

This shouldn't be using indent

[Yoland Yan(Google), 2016/04/29 19:51:43]

Done.

+    logging.info('Saved json output file to %s', json_output_path)
+
+if __name__ == '__main__':
+  main()