Find annotated tests by exposing API in instrumentation_test_instance

tools/android/find_disabled_tests.py

+#!/usr/bin/env python
+# 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'))
+
+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
+
+_DISABLED_TESTS_ANNOTATION_LIST = {'DisabledTest': None, 'FlakyTest': None}
+_GIT_TIME_FORMAT = '%Y-%m-%dT%H:%M:%S'
+_EXPORT_TIME_FORMAT = '%Y%m%dT%H%M%S'
+
+class _JSONKeyName(object):

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

do we really need this?

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

hmm, I thought this is good to organize global variables for readability.

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

I just wouldn't store these as constants at all. I bet you only use them once
each. And (my opinion) they hurt readability a bit when building the dicts,
rather than improving it.

+  TEST_MASTER_KEY = 'tests'
+  RECORD_MASTER_KEY = 'records'
+  REVISION_KEY = 'revision'
+  STATUS_KEY = 'status'
+  TEST_NAME_KEY = '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 = 'utc_buildtime'
+  UTC_REVISIONTIME_KEY = 'utc_revisiontime'
+  PLATFORM_KEY = 'platform'
+  PLATFORM_VALUE = 'android'
+
+  STATUS_DISABLED_TEST_VALUE = 'Disabled Test'
+  STATUS_FLAKY_TEST_VALUE = 'Flaky Test'
+
+def _SerializeTests(test_apks, annotations):
+  """Use GetFilteredTests to get tests info with wanted annotations"""
+  total_test_count = 0
+  tracked_test_list = []
+  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)
+    temp_tracked_test_list = instrumentation_test_instance.GetFilteredTests(

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

could we make getting all tests, and the filtering them two separate steps?

I feel it would be a bit more natural (and avoid to "_GetAllTests" twice) if you
do something like:


  all_tests = instrumentation_test_instance.GetAllTests(...)
  total_test_count += count_tests(all_tests)
  tracked_tests.extend(FilterTests(all_tests, ...))
  

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

Done

+        test_jar=test_jar,
+        test_filter=None,
+        annotations=annotations,
+        exclude_annotations=None
+        )
+    tracked_test_list.extend(temp_tracked_test_list)
+    total_test_count += instrumentation_test_instance.TotalTestCount(test_jar)
+  logging.info('Total count of tests in all test apks: %d', total_test_count)
+  result = []
+  for test_class in tracked_test_list:
+    class_path = test_class['class']
+    class_name = test_class['class'].split('.')[-1]
+    for test_method in test_class['methods']:
+      # getting annotation of each test case
+      bug_id = None
+      status = []
+      test_annotations = test_method['annotations']
+      assert len(test_annotations) != 0
+      for annotation, content in test_annotations.iteritems():
+        if content is not None:
+          bug_id = content['message']

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

probably a minor thing, but do we worry about multiple annotations having a
message/bud_id?

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

hmm, that's a valid point, on one hand I can add more condition here to specify
only when message is an crbug url, 

on the other hand, if we were to track all kinds of annotations, then it's
probably important to have a flexible property for test annotations (list of
jsons, or list of pickles), so we can store everything detail related to an
annotation and someone can search for "Restriction, No internet" for tests with
@Restriction({NO_INTERNET}

Would love your opinion on this! Because the second option seems pretty awesome,
but I don't how slow the query/unpacking would be if the data is stored in
json/pickle. each query would require something like this:
`test_anno_dictionary_list = [{test: test.annotation.to_dict()} for test in
test.query([condition]).fetch()]`
`tests = find_tests_by_annotation_value(test_anno_dictionary_list)` 

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

my main worry was about the bug_id, if there are two annotations specifying a
bug_id, it seems that you'll get a random one identified as "the" bug id of your
test.

For this I can think if a few possibilities:

1. Live with it. It's probably a rare occurrence (is it?), so just keeping track
of a single bug id (picked at random) is fine in most cases.

2. Keep only the most recent (i.e. larger number) bug id.

3. Keep a list of all bug ids.

I would prefer maybe 2; since it feels a bit less random, and does not introduce
as much complexity in your dashboard code.

About the possibility of querying for other "sorts" of annotations (like "no
internet" restriction), let's worry about that later. If we want to query about
some other kind of property the solution would be to create an index for that on
cloud storage and, since you're keeping the whole json with all annotations
anyway, we can always retroactively index things from the past.

+        status.append(annotation)
+      # getting test method name of each test
+      test_name = test_method['method']
+      test_dict = {
+          _JSONKeyName.CRBUG_KEY: bug_id,
+          _JSONKeyName.STATUS_KEY: status,
+          _JSONKeyName.TEST_NAME_KEY: test_name,
+          _JSONKeyName.CLASS_NAME_KEY: class_name,
+          _JSONKeyName.CLASS_PATH_KEY: class_path
+          }

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

nit: The closing brace should be aligned with test_dict.

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

Done.

+      result.append(test_dict)
+  return result, total_test_count
+
+def _DisabledFlakyTestCount(test_list):
+  disabled_test_count = 0
+  flaky_test_count = 0
+  for test in test_list:
+    if _JSONKeyName.STATUS_DISABLED_TEST_VALUE in test.get(
+        _JSONKeyName.STATUS_KEY, ""):
+      disabled_test_count += 1
+    if _JSONKeyName.STATUS_FLAKY_TEST_VALUE in test.get(
+        _JSONKeyName.STATUS_KEY, ""):
+      flaky_test_count += 1
+  return disabled_test_count, flaky_test_count
+
+def _AddRecord(tracked_test_list, utc_buildtime_string, total_test_count):
+  tracked_flaky_test_count = len(tracked_test_list)
+  disabled_test_count, flaky_test_count = _DisabledFlakyTestCount(
+      tracked_test_list)
+
+  revision = cmd_helper.GetCmdOutput(['git', 'rev-parse', 'HEAD']).strip()
+  raw_string = cmd_helper.GetCmdOutput(
+      ['git', 'log', '--pretty=format:%aI', '--max-count=1', 'HEAD'])
+  time_string_search = re.search(r'\d+-\d+-\d+T\d+:\d+:\d+', raw_string)
+  if time_string_search is None:
+    raise Exception('Time format incorrect')
+
+  raw_string = cmd_helper.GetCmdOutput(
+      ['git', 'log', '--pretty=format:%b', '--max-count=1', 'HEAD'])
+  commit_pos_search = re.search(r'Cr-Commit-Position: (.*)', raw_string)
+  if commit_pos_search is None:
+    raise Exception('Cr commit position is not found, potentially running with '
+                    '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)
+
+  record_data = [{
+      _JSONKeyName.REVISION_KEY: revision,
+      _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.TRACKED_TEST_COUNT_KEY: tracked_flaky_test_count,
+      _JSONKeyName.DISABLED_TEST_COUNT_KEY: disabled_test_count,
+      _JSONKeyName.FLAKY_TEST_COUNT_KEY: flaky_test_count,
+      _JSONKeyName.TOTAL_TEST_COUNT_KEY: total_test_count
+      }]
+  return record_data
+
+def main():
+  default_build_type = os.environ.get('BUILDTYPE', 'Debug')
+  parser = argparse.ArgumentParser()
+  parser.add_argument('-t', '--test-apks', action='append', default=[],
+                      dest='test_apks', help='List all test apks file name ' +
+                           'that the script uses to fetch tracked tests from')
+  parser.add_argument(
+      '--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.'))

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

should there be also a --release option?

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

Done.

+  parser.add_argument('-o', '--output-path',
+                      help='JSON file output to be uploaded on to gcs')
+  parser.add_argument('-a', '--annotations', dest='annotations',
+                      action='append', default=['DisabledTest', 'FlakyTest'],
+                      help='Disabled tests annotations, seperated by ,' +

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

I would rephrase as: 'Test annotations to track. The default includes
DisabledTest and FlakyTest.'

Also when you use append, annotations are not separated by commas. In this case
you would have to write:

    --annotations DisabledTest --annotations FlakyTest

which probably isn't great either. Alternatively, you could replace
action='append' with nargs='+', so the interface becomes:

    --annotations DisabledTest FlakyTest

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

My bad, done

+                          'e.g. --annotation-str=DisabledTest,FlakyTest')
+  parser.add_argument('-v', '--verbose', action='store_true', default=False,
+                      help='DEBUG verbosity')
+
+  arguments = parser.parse_args(sys.argv[1:])
+  logging.basicConfig(
+      level=logging.DEBUG if arguments.verbose else logging.WARNING)
+  annotations = dict((x, None) for x in arguments.annotations)
+  constants.SetBuildType(arguments.build_type)
+  logging.info('Use jar from build type: %s', arguments.build_type)

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

nit: Use -> Using

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

Done.

+
+  buildtime = datetime.datetime.utcnow()
+  buildtime_string = buildtime.strftime('%Y%m%dT%H%M%S')

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

_EXPORT_TIME_FORMAT

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

Done.

+  logging.info('Build time is %s', buildtime_string)
+  test_data, total_test_count = _SerializeTests(arguments.test_apks,
+                                                 annotations)
+  record_data = _AddRecord(test_data, buildtime_string, total_test_count)

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

Instead of _AddRecord, I would have a single method (maybe _GetRecord? I'm still
not sure if "record" is the best name for this "report" or "json-thingy" you're
producing) that returns the whole json blurb (i.e. your export_data below) that
will go into the output file.

That method should call _SerializeTests (maybe just _GetTests?) and fill in the
other keys of the record.

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

I changed record to report everywhere (will patch in once we finish the
discussion over here), but if I change the whole thing to report, what should I
call the first part of the report (the old record section), I currently call it
report_meta.
report = {
    "report_meta": [{report_meta_data}],
    "tests": [list of tests...]
}

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

metadata? I would suggest:

    report = {
      "metadata": {report_meta_data},  # no singleton-list, pls
      "tests": [list of tests...]
    }

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

Got it
Done

+
+  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)
+  export_data = {
+      _JSONKeyName.RECORD_MASTER_KEY: record_data,
+      _JSONKeyName.TEST_MASTER_KEY: test_data
+      }
+  with open(json_output_path, 'w') as f:
+    json.dump(export_data, f, indent=2, sort_keys=True)

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

also add: separators=(': ', ',')

otherwise you end up with trailing spaces between commas and end-of-lines.

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

Woah!
Done

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