Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1851143002:
Find annotated tests by exposing API in instrumentation_test_instance (Closed)
|
Created:
4 years, 8 months ago by Yoland Yan(Google) Modified:
4 years, 5 months ago Reviewers:
perezju, michaelbai, jbudorick, the real yoland, mikecase (-- gone --), Xianzhu, xianzhanzhu CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFind annotated tests by exposing API in instrumentation_test_instance
BUG=601909
Committed: https://crrev.com/97dc1912066a4312ffac05dbad24880897be80e9
Cr-Commit-Position: refs/heads/master@{#402699}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add find_disabled_tests.py #
Total comments: 2
Patch Set 3 : Reorg function, add logging, docstring, and other minor stuff #Patch Set 4 : Reorg function, add logging, docstring, and other minor stuff #Patch Set 5 : Minor change #
Total comments: 60
Patch Set 6 : #
Total comments: 18
Patch Set 7 : #Patch Set 8 : #
Total comments: 57
Patch Set 9 : #Patch Set 10 : #
Total comments: 10
Patch Set 11 : Rebase #Patch Set 12 : Change script name #Patch Set 13 : Change after mikecase's comments #
Total comments: 24
Patch Set 14 : Change after John's comments #
Total comments: 14
Patch Set 15 : Change after John's comment #Patch Set 16 : Changes to accommodate the recipe change in recipe #Patch Set 17 : Nit #Patch Set 18 : Changes after recipe side change #
Total comments: 6
Patch Set 19 : Change nits #Patch Set 20 : Rebase #Patch Set 21 : Changes on instrumentation_test_instance_test.py #Patch Set 22 : Changes on instrumentation_test_instance_test.py #
Total comments: 5
Patch Set 23 : Rebase #
Messages
Total messages: 78 (26 generated)
yolandyan@google.com changed reviewers: + jbudorick@chromium.org
Basically moved some private proguard related methods to functions for external usage. I wonder why putting them in a separate file (e.g. ... test_jar.py...) is a bad idea?
Description was changed from ========== Add GetFilteredTest API to instrumentation_test_instance.py BUG= ========== to ========== For detail of how the find_disabled_tests.py operate, check this CL: https://codereview.chromium.org/1853333004/ BUG= ==========
https://codereview.chromium.org/1851143002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:190: def GetTestsFromPickle(pickle_path, jar_path): Can you upload a version of tools/android/find_disabled_tests.py that uses this version? I'm leaning toward something in the middle of the two versions -- exposing some subset of what you have here outside of InstrumentationTestInstance -- but I'd like to see what you've got with this, first.
https://codereview.chromium.org/1851143002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:190: def GetTestsFromPickle(pickle_path, jar_path): On 2016/04/06 17:28:33, jbudorick wrote: > Can you upload a version of tools/android/find_disabled_tests.py that uses this > version? I'm leaning toward something in the middle of the two versions -- > exposing some subset of what you have here outside of > InstrumentationTestInstance -- but I'd like to see what you've got with this, > first. Done.
https://codereview.chromium.org/1851143002/diff/20001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/20001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:190: def GetTestsFromPickle(pickle_path, jar_path): Thanks for putting up with my waffling on this. Let's go with this version, but make everything that isn't called by your function module-private (i.e., have the names start with _)
yolandyan@google.com changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/1851143002/diff/20001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/20001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:190: def GetTestsFromPickle(pickle_path, jar_path): On 2016/04/06 20:33:30, jbudorick wrote: > Thanks for putting up with my waffling on this. > > Let's go with this version, but make everything that isn't called by your > function module-private (i.e., have the names start with _) dude, ofc Done.
yolandyan@google.com changed reviewers: + perezju@chromium.org
Lots of comments, sorry for that :-/ https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (left): https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:140: nit: bring that blank line back https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:188: nit: two blank lines between module level elements. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:195: return total_test_count nit: return sum(len(c['methods']) for c in _GetAllTests(test_jar)) https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:197: def GetFilteredTests(test_jar, test_filter, annotations, exclude_annotations): make test_filter, annotations, and exclude_annotations optional https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:204: exclude_annotations: a dict of annotations to exclude. Why are these dicts? How do they look like? https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:207: A list of filtered tests Add an example of what each such "test" looks like. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:208: """ nit: move those quotes three spaces to the left. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:211: exclude_annotations) nit: one space to the right https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:231: except TypeError as e: what are you hoping to catch with TypeError? https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:235: # pylint: disable=no-self-use not needed anymore https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:245: class_lookup = dict((c['class'], c) for c in p['classes']) nit: {c['class']: c for c in p['classes']} and move this line next to the p = ... above. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:246: def recursive_get_class_annotations(c): nit: recursive_class_annotations should be a fine name https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:252: a.update(c['annotations']) 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. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:285: return any_annotation_matches(annotations, all_annotations) 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 https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:326: logging.info('Getting tests from JAR via proguard. (%s)', str(e)) maybe: logging.info('Could not get tests from pickle: %s', e) logging.info('Getting tests from JAR via proguard.') https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:331: class ProguardPickleException(Exception): Move declarations of exceptions near the top of the module. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:34: class _JSONKeyName(object): do we really need this? https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:64: temp_tracked_test_list = instrumentation_test_instance.GetFilteredTests( 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, ...)) https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:85: bug_id = content['message'] probably a minor thing, but do we worry about multiple annotations having a message/bud_id? https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:95: } nit: The closing brace should be aligned with test_dict. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:161: 'Default is env var BUILDTYPE or Debug.')) should there be also a --release option? https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:166: help='Disabled tests annotations, seperated by ,' + 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 https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:176: logging.info('Use jar from build type: %s', arguments.build_type) nit: Use -> Using https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:179: buildtime_string = buildtime.strftime('%Y%m%dT%H%M%S') _EXPORT_TIME_FORMAT https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:183: record_data = _AddRecord(test_data, buildtime_string, total_test_count) 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. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:196: json.dump(export_data, f, indent=2, sort_keys=True) also add: separators=(': ', ',') otherwise you end up with trailing spaces between commas and end-of-lines.
On 2016/04/12 10:29:04, perezju wrote: > Lots of comments, sorry for that :-/ > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > File build/android/pylib/instrumentation/instrumentation_test_instance.py > (left): > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:140: > nit: bring that blank line back > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > File build/android/pylib/instrumentation/instrumentation_test_instance.py > (right): > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:188: > nit: two blank lines between module level elements. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:195: return > total_test_count > nit: return sum(len(c['methods']) for c in _GetAllTests(test_jar)) > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:197: def > GetFilteredTests(test_jar, test_filter, annotations, exclude_annotations): > make test_filter, annotations, and exclude_annotations optional > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:204: > exclude_annotations: a dict of annotations to exclude. > Why are these dicts? How do they look like? > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:207: A list > of filtered tests > Add an example of what each such "test" looks like. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:208: """ > nit: move those quotes three spaces to the left. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:211: > exclude_annotations) > nit: one space to the right > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:231: except > TypeError as e: > what are you hoping to catch with TypeError? > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:235: # > pylint: disable=no-self-use > not needed anymore > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:245: > class_lookup = dict((c['class'], c) for c in p['classes']) > nit: {c['class']: c for c in p['classes']} > > and move this line next to the p = ... above. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:246: def > recursive_get_class_annotations(c): > nit: recursive_class_annotations should be a fine name > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:252: > a.update(c['annotations']) > 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. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:285: return > any_annotation_matches(annotations, all_annotations) > 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 > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:326: > logging.info('Getting tests from JAR via proguard. (%s)', str(e)) > maybe: > > logging.info('Could not get tests from pickle: %s', e) > logging.info('Getting tests from JAR via proguard.') > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:331: class > ProguardPickleException(Exception): > Move declarations of exceptions near the top of the module. > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > File tools/android/find_disabled_tests.py (right): > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:34: class _JSONKeyName(object): > do we really need this? > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:64: temp_tracked_test_list = > instrumentation_test_instance.GetFilteredTests( > 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, ...)) > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:85: bug_id = content['message'] > probably a minor thing, but do we worry about multiple annotations having a > message/bud_id? > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:95: } > nit: The closing brace should be aligned with test_dict. > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:161: 'Default is env var BUILDTYPE or > Debug.')) > should there be also a --release option? > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:166: help='Disabled tests annotations, > seperated by ,' + > 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 > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:176: logging.info('Use jar from build type: > %s', arguments.build_type) > nit: Use -> Using > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:179: buildtime_string = > buildtime.strftime('%Y%m%dT%H%M%S') > _EXPORT_TIME_FORMAT > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:183: record_data = _AddRecord(test_data, > buildtime_string, total_test_count) > 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. > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:196: json.dump(export_data, f, indent=2, > sort_keys=True) > also add: separators=(': ', ',') > > otherwise you end up with trailing spaces between commas and end-of-lines. No this is great! The more correction, the more I learn, thank you!
On 2016/04/12 10:29:04, perezju wrote: > Lots of comments, sorry for that :-/ > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > File build/android/pylib/instrumentation/instrumentation_test_instance.py > (left): > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:140: > nit: bring that blank line back > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > File build/android/pylib/instrumentation/instrumentation_test_instance.py > (right): > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:188: > nit: two blank lines between module level elements. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:195: return > total_test_count > nit: return sum(len(c['methods']) for c in _GetAllTests(test_jar)) > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:197: def > GetFilteredTests(test_jar, test_filter, annotations, exclude_annotations): > make test_filter, annotations, and exclude_annotations optional > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:204: > exclude_annotations: a dict of annotations to exclude. > Why are these dicts? How do they look like? > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:207: A list > of filtered tests > Add an example of what each such "test" looks like. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:208: """ > nit: move those quotes three spaces to the left. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:211: > exclude_annotations) > nit: one space to the right > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:231: except > TypeError as e: > what are you hoping to catch with TypeError? > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:235: # > pylint: disable=no-self-use > not needed anymore > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:245: > class_lookup = dict((c['class'], c) for c in p['classes']) > nit: {c['class']: c for c in p['classes']} > > and move this line next to the p = ... above. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:246: def > recursive_get_class_annotations(c): > nit: recursive_class_annotations should be a fine name > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:252: > a.update(c['annotations']) > 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. > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:285: return > any_annotation_matches(annotations, all_annotations) > 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 > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:326: > logging.info('Getting tests from JAR via proguard. (%s)', str(e)) > maybe: > > logging.info('Could not get tests from pickle: %s', e) > logging.info('Getting tests from JAR via proguard.') > > https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... > build/android/pylib/instrumentation/instrumentation_test_instance.py:331: class > ProguardPickleException(Exception): > Move declarations of exceptions near the top of the module. > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > File tools/android/find_disabled_tests.py (right): > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:34: class _JSONKeyName(object): > do we really need this? > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:64: temp_tracked_test_list = > instrumentation_test_instance.GetFilteredTests( > 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, ...)) > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:85: bug_id = content['message'] > probably a minor thing, but do we worry about multiple annotations having a > message/bud_id? > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:95: } > nit: The closing brace should be aligned with test_dict. > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:161: 'Default is env var BUILDTYPE or > Debug.')) > should there be also a --release option? > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:166: help='Disabled tests annotations, > seperated by ,' + > 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 > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:176: logging.info('Use jar from build type: > %s', arguments.build_type) > nit: Use -> Using > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:179: buildtime_string = > buildtime.strftime('%Y%m%dT%H%M%S') > _EXPORT_TIME_FORMAT > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:183: record_data = _AddRecord(test_data, > buildtime_string, total_test_count) > 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. > > https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... > tools/android/find_disabled_tests.py:196: json.dump(export_data, f, indent=2, > sort_keys=True) > also add: separators=(': ', ',') > > otherwise you end up with trailing spaces between commas and end-of-lines. No this is great! The more correction, the more I learn, thank you!
Will upload the next patch once the discussion is finished in comments https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (left): https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:140: On 2016/04/12 10:29:02, perezju wrote: > nit: bring that blank line back Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:188: On 2016/04/12 10:29:03, perezju wrote: > nit: two blank lines between module level elements. Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:195: return total_test_count On 2016/04/12 10:29:03, perezju wrote: > nit: return sum(len(c['methods']) for c in _GetAllTests(test_jar)) Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:197: def GetFilteredTests(test_jar, test_filter, annotations, exclude_annotations): On 2016/04/12 10:29:03, perezju wrote: > make test_filter, annotations, and exclude_annotations optional No longer an issue, but changed the now public FilterTests with optional variables https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:204: exclude_annotations: a dict of annotations to exclude. On 2016/04/12 10:29:03, perezju wrote: > Why are these dicts? How do they look like? 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']}} https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:207: A list of filtered tests On 2016/04/12 10:29:03, perezju wrote: > Add an example of what each such "test" looks like. Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:208: """ On 2016/04/12 10:29:03, perezju wrote: > nit: move those quotes three spaces to the left. Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:211: exclude_annotations) On 2016/04/12 10:29:03, perezju wrote: > nit: one space to the right Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:231: except TypeError as e: On 2016/04/12 10:29:03, perezju wrote: > what are you hoping to catch with TypeError? If from before, I think might have been some calculation that happened in the try block could have cause TypeError Deleted https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:235: # pylint: disable=no-self-use On 2016/04/12 10:29:03, perezju wrote: > not needed anymore Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:245: class_lookup = dict((c['class'], c) for c in p['classes']) On 2016/04/12 10:29:03, perezju wrote: > nit: {c['class']: c for c in p['classes']} > > and move this line next to the p = ... above. Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:246: def recursive_get_class_annotations(c): On 2016/04/12 10:29:03, perezju wrote: > nit: recursive_class_annotations should be a fine name Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:285: return any_annotation_matches(annotations, all_annotations) On 2016/04/12 10:29:02, perezju wrote: > 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 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. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:326: logging.info('Getting tests from JAR via proguard. (%s)', str(e)) On 2016/04/12 10:29:03, perezju wrote: > maybe: > > logging.info('Could not get tests from pickle: %s', e) > logging.info('Getting tests from JAR via proguard.') Done. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:331: class ProguardPickleException(Exception): On 2016/04/12 10:29:03, perezju wrote: > Move declarations of exceptions near the top of the module. Done. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:34: class _JSONKeyName(object): On 2016/04/12 10:29:03, perezju wrote: > do we really need this? hmm, I thought this is good to organize global variables for readability. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:64: temp_tracked_test_list = instrumentation_test_instance.GetFilteredTests( On 2016/04/12 10:29:03, perezju wrote: > 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, ...)) > Done https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:85: bug_id = content['message'] On 2016/04/12 10:29:03, perezju wrote: > probably a minor thing, but do we worry about multiple annotations having a > message/bud_id? 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)` https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:95: } On 2016/04/12 10:29:03, perezju wrote: > nit: The closing brace should be aligned with test_dict. Done. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:161: 'Default is env var BUILDTYPE or Debug.')) On 2016/04/12 10:29:03, perezju wrote: > should there be also a --release option? Done. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:166: help='Disabled tests annotations, seperated by ,' + On 2016/04/12 10:29:04, perezju wrote: > 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 My bad, done https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:176: logging.info('Use jar from build type: %s', arguments.build_type) On 2016/04/12 10:29:03, perezju wrote: > nit: Use -> Using Done. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:179: buildtime_string = buildtime.strftime('%Y%m%dT%H%M%S') On 2016/04/12 10:29:03, perezju wrote: > _EXPORT_TIME_FORMAT Done. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:183: record_data = _AddRecord(test_data, buildtime_string, total_test_count) On 2016/04/12 10:29:03, perezju wrote: > 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. 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...] } https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:196: json.dump(export_data, f, indent=2, sort_keys=True) On 2016/04/12 10:29:03, perezju wrote: > also add: separators=(': ', ',') > > otherwise you end up with trailing spaces between commas and end-of-lines. Woah! Done
https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:252: a.update(c['annotations']) On 2016/04/12 10:29:02, perezju wrote: > 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. I am not sure whether order is important, will look into it before commit
https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:285: return any_annotation_matches(annotations, all_annotations) On 2016/04/13 01:03:11, yolandyan wrote: > On 2016/04/12 10:29:02, perezju wrote: > > 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 > > 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. Acknowledged. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:34: class _JSONKeyName(object): On 2016/04/13 01:03:11, yolandyan wrote: > On 2016/04/12 10:29:03, perezju wrote: > > do we really need this? > > hmm, I thought this is good to organize global variables for readability. 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. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:183: record_data = _AddRecord(test_data, buildtime_string, total_test_count) On 2016/04/13 01:03:11, yolandyan wrote: > On 2016/04/12 10:29:03, perezju wrote: > > 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. > > 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...] > } metadata? I would suggest: report = { "metadata": {report_meta_data}, # no singleton-list, pls "tests": [list of tests...] }
https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:285: return any_annotation_matches(annotations, all_annotations) On 2016/04/13 09:53:46, perezju wrote: > On 2016/04/13 01:03:11, yolandyan wrote: > > On 2016/04/12 10:29:02, perezju wrote: > > > 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 > > > > 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. > > Acknowledged. Done. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:183: record_data = _AddRecord(test_data, buildtime_string, total_test_count) On 2016/04/13 09:53:47, perezju wrote: > On 2016/04/13 01:03:11, yolandyan wrote: > > On 2016/04/12 10:29:03, perezju wrote: > > > 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. > > > > 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...] > > } > > metadata? I would suggest: > > report = { > "metadata": {report_meta_data}, # no singleton-list, pls > "tests": [list of tests...] > } Got it Done
Looking a lot better. Only a few more comments left. Also, could you run your script and post the resulting json somewhere for us to see? (maybe in x20 or something like that) https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:204: exclude_annotations: a dict of annotations to exclude. On 2016/04/13 01:03:11, yolandyan wrote: > On 2016/04/12 10:29:03, perezju wrote: > > Why are these dicts? How do they look like? > > 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']}} 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. https://codereview.chromium.org/1851143002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:252: a.update(c['annotations']) On 2016/04/13 02:18:43, yolandyan wrote: > On 2016/04/12 10:29:02, perezju wrote: > > 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. > > I am not sure whether order is important, will look into it before commit It's fine, leave it as is. https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/80001/tools/android/find_disa... tools/android/find_disabled_tests.py:85: bug_id = content['message'] On 2016/04/13 01:03:11, yolandyan wrote: > On 2016/04/12 10:29:03, perezju wrote: > > probably a minor thing, but do we worry about multiple annotations having a > > message/bud_id? > > 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)` 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. https://codereview.chromium.org/1851143002/diff/100001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/100001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:415: self._package_info = package_info should this also break? https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:32: _EXCLUDE_ANNOTATION_LIST = ["CommandLineFlags$Add"] why excluded? https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:34: class _JSONKeyName(object): I would still vote for removing this constant definitions and just use the strings directly below. https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:63: def _GetAnnotations(test_annotations, annotations_dict): don't pass annotations_dict; looks like it's always empty. Instead return the pair: bug_id, annotations_dict https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:84: all_test = instrumentation_test_instance.GetAllTests(test_jar=test_jar) nit: all_tests https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:117: time_string_search = re.search(r'\d+-\d+-\d+T\d+:\d+:\d+', raw_string) define a constant for this regex https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:123: commit_pos_search = re.search(r'Cr-Commit-Position: (.*)', raw_string) ditto https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:143: return record_data nit: just return { ... } https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:201: json.dump(report_data, f, indent=2, sort_keys=True, separators=(',',':')) ':' -> ': ' (missing a space)
I am WFH today and can't scp the json output file from my computer to my macbook. Will upload the json output file to x20 tomorrow morning. But I copied part of it in the design doc (link in crbug) https://codereview.chromium.org/1851143002/diff/100001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/100001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:415: self._package_info = package_info On 2016/04/25 12:45:07, perezju wrote: > should this also break? Ya, PACKAGE_INFO values should be unique https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:32: _EXCLUDE_ANNOTATION_LIST = ["CommandLineFlags$Add"] On 2016/04/25 12:45:08, perezju wrote: > why excluded? Because I thought annotation also has string element. which can be confused as bug id for a test But not true, removed https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:34: class _JSONKeyName(object): On 2016/04/25 12:45:08, perezju wrote: > I would still vote for removing this constant definitions and just use the > strings directly below. Got it https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:63: def _GetAnnotations(test_annotations, annotations_dict): On 2016/04/25 12:45:08, perezju wrote: > don't pass annotations_dict; looks like it's always empty. Instead return the > pair: bug_id, annotations_dict Done. https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:84: all_test = instrumentation_test_instance.GetAllTests(test_jar=test_jar) On 2016/04/25 12:45:08, perezju wrote: > nit: all_tests Done. https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:117: time_string_search = re.search(r'\d+-\d+-\d+T\d+:\d+:\d+', raw_string) On 2016/04/25 12:45:08, perezju wrote: > define a constant for this regex Done. https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:123: commit_pos_search = re.search(r'Cr-Commit-Position: (.*)', raw_string) On 2016/04/25 12:45:08, perezju wrote: > ditto Done. https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:143: return record_data On 2016/04/25 12:45:07, perezju wrote: > nit: just > > return { ... } Done. https://codereview.chromium.org/1851143002/diff/100001/tools/android/find_dis... tools/android/find_disabled_tests.py:201: json.dump(report_data, f, indent=2, sort_keys=True, separators=(',',':')) On 2016/04/25 12:45:08, perezju wrote: > ':' -> ': ' (missing a space) woah, done!
I am WFH today and can't scp the json output file from my computer to my macbook. Will upload the json output file to x20 tomorrow morning. But I copied part of it in the design doc (link in crbug)
Description was changed from ========== For detail of how the find_disabled_tests.py operate, check this CL: https://codereview.chromium.org/1853333004/ BUG= ========== to ========== For detail of how the find_disabled_tests.py operate, check this CL: https://codereview.chromium.org/1853333004/ BUG=601909 ==========
On 2016/04/26 01:10:12, yolandyan wrote: > I am WFH today and can't scp the json output file from my computer to my > macbook. Will upload the json output file to x20 tomorrow morning. > > But I copied part of it in the design doc (link in crbug) Just added the json output file to an internal link added in the crbug https://bugs.chromium.org/p/chromium/issues/detail?id=601909
On 2016/04/26 01:10:12, yolandyan wrote: > I am WFH today and can't scp the json output file from my computer to my > macbook. Will upload the json output file to x20 tomorrow morning. > > But I copied part of it in the design doc (link in crbug) Just added the json output file to an internal link added in the crbug https://bugs.chromium.org/p/chromium/issues/detail?id=601909
Thanks for the output json. Looking all good! Only a couple issues left. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:87: bug_id, class_annotation = _GetAnnotations(test_class['annotations']) hmm.. looks like you always end up throwing away this bug_id from the class. (It's overridden below) https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:112: ['git', 'log', '--pretty=format:%aI', '--max-count=1', 'HEAD']) 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(..)? https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:113: git_log_time_pattern = r'\d+-\d+-\d+T\d+:\d+:\d+' nit: a module level constant :) you could also pre-compile it with re.compile https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:120: git_log_message_pattern = r'Cr-Commit-Position: (.*)' 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...
https://codereview.chromium.org/1851143002/diff/140001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:195: def FilterTests(tests, test_filter=None, annotations=None, nit: GetAllTests (and its helpers) should precede FilterTests in here. https://codereview.chromium.org/1851143002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:211: t = ['%s.%s' % (c['class'].split('.')[-1], m['method'])] Need to rebase this; agrieve landed a CL earlier today to additionally support fully qualified test classes. https://codereview.chromium.org/1851143002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:283: nit: two blank lines between top-level functions. https://codereview.chromium.org/1851143002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:312: nit: same https://codereview.chromium.org/1851143002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:322: nit: same https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:1: #!/usr/bin/env python Can you move this to a subdirectory of tools/android/? https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:23: nit: no blank line here https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:33: class _JSONKeyName(object): 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. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:56: result = re.search(r'crbug(?:.com)?/(\d+)', message) compile this into a module-scope constant https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:61: nit: two lines https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:74: """All the all the tests""" nit: "All the all the tests"? https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:96: _JSONKeyName.CRBUG_KEY: bug_id, nit: 2 space indent https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:113: git_log_time_pattern = r'\d+-\d+-\d+T\d+:\d+:\d+' On 2016/04/27 09:29:15, perezju wrote: > nit: a module level constant :) > > you could also pre-compile it with re.compile +1. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:116: raise Exception('Time format incorrect') This error should be a little more verbose -- _why_ is it incorrect? https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:120: git_log_message_pattern = r'Cr-Commit-Position: (.*)' On 2016/04/27 09:29:15, perezju wrote: > 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... note that this should also be compiled into a module-scope constant https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:123: raise Exception('Cr commit position is not found, potentially running with ' nit: this should say "Cr-Commit-Position ..." rather than "Cr commit position ..." https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:135: _JSONKeyName.REVISION_KEY: revision, nit: 2 space indent https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:140: _JSONKeyName.TOTAL_TEST_COUNT_KEY: total_test_count} nit: drop the trailing } onto the next line https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:161: default_build_type = os.environ.get('BUILDTYPE', 'Debug') please no https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:162: parser = argparse.ArgumentParser() Which of these are required? https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:163: parser.add_argument('-t', '--test-apks', nargs='+', dest='test_apks', Why should this handle multiple APKs at once? Why wouldn't we just call it more than once? https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:166: parser.add_argument( nix both --debug and --release and instead support --output-directory. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:176: help='JSON file output to be uploaded on to gcs') This description is wrong; this is now the output directory. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:177: parser.add_argument('-v', '--verbose', action='store_true', default=False, -v = INFO, -vv = DEBUG plz https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:183: constants.SetBuildType(arguments.build_type) Get rid of these two lines. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:186: buildtime = datetime.datetime.utcnow() nit: buildtime is a bad name because this isn't when the APK was built. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:198: json.dump(report_data, f, indent=2, sort_keys=True, separators=(',',': ')) This shouldn't be using indent
https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:33: class _JSONKeyName(object): On 2016/04/28 14:17:14, jbudorick wrote: > 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. I've been saying the same thing for several reviews now! :P
lgtm modulo what we discussed today on gvc, and also wait for John. will have a look tomorrow at the dashboard code
https://codereview.chromium.org/1851143002/diff2/140001:180001/tools/android/... https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:1: #!/usr/bin/env python On 2016/04/28 14:17:14, jbudorick wrote: > Can you move this to a subdirectory of tools/android/? Will move to tools/android/find_annotated_tests/find_annotated_tests? https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:23: On 2016/04/28 14:17:15, jbudorick wrote: > nit: no blank line here Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:33: class _JSONKeyName(object): On 2016/04/28 14:33:58, perezju wrote: > On 2016/04/28 14:17:14, jbudorick wrote: > > 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. > > I've been saying the same thing for several reviews now! :P wow, what a bunch of bullies lol Done https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:56: result = re.search(r'crbug(?:.com)?/(\d+)', message) On 2016/04/28 14:17:15, jbudorick wrote: > compile this into a module-scope constant Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:61: On 2016/04/28 14:17:14, jbudorick wrote: > nit: two lines Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:74: """All the all the tests""" On 2016/04/28 14:17:14, jbudorick wrote: > nit: "All the all the tests"? got it s/All the all the tests/All the all the all/g Done https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:87: bug_id, class_annotation = _GetAnnotations(test_class['annotations']) On 2016/04/27 09:29:15, perezju wrote: > hmm.. looks like you always end up throwing away this bug_id from the class. > (It's overridden below) Changed to class_bug_id and test_bug_id for cases where an entire class is marked as disabled/flaky with an crbug https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:87: bug_id, class_annotation = _GetAnnotations(test_class['annotations']) On 2016/04/27 09:29:15, perezju wrote: > hmm.. looks like you always end up throwing away this bug_id from the class. > (It's overridden below) Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:96: _JSONKeyName.CRBUG_KEY: bug_id, On 2016/04/28 14:17:15, jbudorick wrote: > nit: 2 space indent Keep it as 4 after a serious discussion https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:112: ['git', 'log', '--pretty=format:%aI', '--max-count=1', 'HEAD']) On 2016/04/27 09:29:15, perezju wrote: > 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(..)? Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:113: git_log_time_pattern = r'\d+-\d+-\d+T\d+:\d+:\d+' On 2016/04/28 14:17:15, jbudorick wrote: > On 2016/04/27 09:29:15, perezju wrote: > > nit: a module level constant :) > > > > you could also pre-compile it with re.compile > > +1. Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:116: raise Exception('Time format incorrect') On 2016/04/28 14:17:15, jbudorick wrote: > This error should be a little more verbose -- _why_ is it incorrect? Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:120: git_log_message_pattern = r'Cr-Commit-Position: (.*)' On 2016/04/28 14:17:15, jbudorick wrote: > On 2016/04/27 09:29:15, perezju wrote: > > 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... > > note that this should also be compiled into a module-scope constant Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:123: raise Exception('Cr commit position is not found, potentially running with ' On 2016/04/28 14:17:14, jbudorick wrote: > nit: this should say "Cr-Commit-Position ..." rather than "Cr commit position > ..." Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:135: _JSONKeyName.REVISION_KEY: revision, On 2016/04/28 14:17:14, jbudorick wrote: > nit: 2 space indent Keep as 4 space indent after serious discussion with +jbudorick https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:140: _JSONKeyName.TOTAL_TEST_COUNT_KEY: total_test_count} On 2016/04/28 14:17:14, jbudorick wrote: > nit: drop the trailing } onto the next line Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:161: default_build_type = os.environ.get('BUILDTYPE', 'Debug') On 2016/04/28 14:17:14, jbudorick wrote: > please no but...lol (Explain: all this was added when the script was actually creating an instrumentation test instance) Done https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:162: parser = argparse.ArgumentParser() On 2016/04/28 14:17:15, jbudorick wrote: > Which of these are required? --test-apks, --json-output-dir Done https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:163: parser.add_argument('-t', '--test-apks', nargs='+', dest='test_apks', On 2016/04/28 14:17:15, jbudorick wrote: > Why should this handle multiple APKs at once? Why wouldn't we just call it more > than once? 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. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:166: parser.add_argument( On 2016/04/28 14:17:14, jbudorick wrote: > nix both --debug and --release and instead support --output-directory. Done https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:176: help='JSON file output to be uploaded on to gcs') On 2016/04/28 14:17:14, jbudorick wrote: > This description is wrong; this is now the output directory. Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:177: parser.add_argument('-v', '--verbose', action='store_true', default=False, On 2016/04/28 14:17:14, jbudorick wrote: > -v = INFO, -vv = DEBUG plz Done. https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:183: constants.SetBuildType(arguments.build_type) On 2016/04/28 14:17:14, jbudorick wrote: > Get rid of these two lines. Done https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:186: buildtime = datetime.datetime.utcnow() On 2016/04/28 14:17:15, jbudorick wrote: > nit: buildtime is a bad name because this isn't when the APK was built. 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 https://codereview.chromium.org/1851143002/diff/140001/tools/android/find_dis... tools/android/find_disabled_tests.py:198: json.dump(report_data, f, indent=2, sort_keys=True, separators=(',',': ')) On 2016/04/28 14:17:14, jbudorick wrote: > This shouldn't be using indent Done.
friendly ping ;)
a few comments. https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:1: #!/usr/bin/env python as mentioned, rename script to something more general since it isnt specific to disabled tests. https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:2: # Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) in the Copyright notice. https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:6: """Finds all the tracked(disabled/flaky) tests from proguard dump""" offline you said this was for all annotations. Still says disabled/flaky here. https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:29: _DEFAULT_APK_OUTPUT_DIR = os.path.join('out', 'Debug') Use pylib/constants like GetBuildDirectory or something instead of this. https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:71: bug_id = _GetBugId(content.get('message')) Ive seen some tests with multiple bugs attached, like @FlakyTest(message="crbug/1,crbug/2") I think. Will this work for that? https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:124: raw_string = cmd_helper.GetCmdOutput( Should probably explicitly set the cwd arg to somewhere inside Chromium when running a git command
yolandyan@chromium.org changed reviewers: + yolandyan@chromium.org
I believe this CL is ready too ;) https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... File tools/android/find_disabled_tests.py (right): https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:1: #!/usr/bin/env python On 2016/06/10 at 20:59:03, mikecase wrote: > as mentioned, rename script to something more general since it isnt specific to disabled tests. Done https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:2: # Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/06/10 at 20:59:02, mikecase wrote: > No (c) in the Copyright notice. Done https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:6: """Finds all the tracked(disabled/flaky) tests from proguard dump""" On 2016/06/10 at 20:59:03, mikecase wrote: > offline you said this was for all annotations. Still says disabled/flaky here. Done https://codereview.chromium.org/1851143002/diff/180001/tools/android/find_dis... tools/android/find_disabled_tests.py:71: bug_id = _GetBugId(content.get('message')) On 2016/06/10 at 20:59:02, mikecase wrote: > Ive seen some tests with multiple bugs attached, like @FlakyTest(message="crbug/1,crbug/2") I think. Will this work for that? That would, except I will do some parsing on the server side when importing or when displaying the information
yolandyan@chromium.org changed reviewers: + yolandyan@chromium.org
I believe this CL is ready too ;)
https://codereview.chromium.org/1851143002/diff/240001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/240001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:301: nit: +1 blank line https://codereview.chromium.org/1851143002/diff/240001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:330: nit: +1 blank line https://codereview.chromium.org/1851143002/diff/240001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:340: nit: +1 blank line https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... File tools/android/find_annotated_tests.py (right): https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:26: sys.path.append(os.path.join(_SRC_DIR, 'third_party', 'catapult', 'devil')) nit: devil should precede pylib in the import list. https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:29: _GIT_TIME_FORMAT = '%Y-%m-%dT%H:%M:%S' nit: alphabetize these constants within each subgroup, especially the last one. https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:37: _TEST_MASTER_KEY = 'tests' Are all of these only used once? If so, why are they constants and not just used in-place directly? https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:64: """Store annotations in the existing anntation_dict and return bug id""" nit: anntation_dict -> annotation_dict https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:66: annotations_dict = {} What's the point of annotations_dict? It's the same thing as test_annotations by the time you finish the for loop. https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:68: if content is not None and content.get('message') is not None: nit: collapse these two lines down to just bug_id = _GetBugId(content.get('message')) if content else None https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:69: bug_id = _GetBugId(content.get('message')) So this gets any annotation with a crbug message? https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:159: _TEST_MASTER_KEY: test_data} nit: drop } onto its own line https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:184: constants.DIR_SOURCE_ROOT, arguments.apk_output_dir)) nit: indent two spaces
Description was changed from ========== For detail of how the find_disabled_tests.py operate, check this CL: https://codereview.chromium.org/1853333004/ BUG=601909 ========== to ========== BUG=601909 ==========
https://codereview.chromium.org/1851143002/diff/240001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/1851143002/diff/240001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:301: On 2016/06/11 at 00:24:02, jbudorick wrote: > nit: +1 blank line Done https://codereview.chromium.org/1851143002/diff/240001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:330: On 2016/06/11 at 00:24:02, jbudorick wrote: > nit: +1 blank line Done https://codereview.chromium.org/1851143002/diff/240001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:340: On 2016/06/11 at 00:24:02, jbudorick wrote: > nit: +1 blank line Done https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... File tools/android/find_annotated_tests.py (right): https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:26: sys.path.append(os.path.join(_SRC_DIR, 'third_party', 'catapult', 'devil')) On 2016/06/11 at 00:24:02, jbudorick wrote: > nit: devil should precede pylib in the import list. Done https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:29: _GIT_TIME_FORMAT = '%Y-%m-%dT%H:%M:%S' On 2016/06/11 at 00:24:03, jbudorick wrote: > nit: alphabetize these constants within each subgroup, especially the last one. Done https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:37: _TEST_MASTER_KEY = 'tests' On 2016/06/11 at 00:24:03, jbudorick wrote: > Are all of these only used once? If so, why are they constants and not just used in-place directly? I was using them here so if any renaming is decided on the app engine CL, I can change them here Changed to use in-place Done https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:64: """Store annotations in the existing anntation_dict and return bug id""" On 2016/06/11 at 00:24:03, jbudorick wrote: > nit: anntation_dict -> annotation_dict ...O_o how Done https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:66: annotations_dict = {} On 2016/06/11 at 00:24:03, jbudorick wrote: > What's the point of annotations_dict? It's the same thing as test_annotations by the time you finish the for loop. My bad, inherited problem from previous CLs Done https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:68: if content is not None and content.get('message') is not None: On 2016/06/11 at 00:24:03, jbudorick wrote: > nit: collapse these two lines down to just > > bug_id = _GetBugId(content.get('message')) if content else None Done https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:69: bug_id = _GetBugId(content.get('message')) On 2016/06/11 at 00:24:03, jbudorick wrote: > So this gets any annotation with a crbug message? Yes, it parses any annotation with message to match any text that has the pattern of r'crbug(?:.com)?/(\d+)' and get the int Added a todo to support multiple crbug ids, right now it only gets one crbug id per method https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:159: _TEST_MASTER_KEY: test_data} On 2016/06/11 at 00:24:02, jbudorick wrote: > nit: drop } onto its own line Done https://codereview.chromium.org/1851143002/diff/240001/tools/android/find_ann... tools/android/find_annotated_tests.py:184: constants.DIR_SOURCE_ROOT, arguments.apk_output_dir)) On 2016/06/11 at 00:24:03, jbudorick wrote: > nit: indent two spaces Done
https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... File tools/android/find_annotated_tests.py (right): https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:40: # TODO(yolandyan): currently the script only support on bug id per method, nit: "only support on bug id" -> "only supports one bug id" https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:47: return None This shouldn't return here. It should only return None outside the for loop. https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:84: logging.info('Total count of tests in all test apks: %d', total_test_count) nit: blank line before this one https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:157: script_runtime = datetime.datetime.utcnow() Why are you doing this in main() and not in either _GetReport or _GetReportMeta? https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:165: temp_output_dir = arguments.json_output_dir nit: just use arguments.json_output_dir directly https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:171: nit: +1 blank line https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:173: main() nit: sys.exit(main())
https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... File tools/android/find_annotated_tests.py (right): https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:40: # TODO(yolandyan): currently the script only support on bug id per method, On 2016/06/13 at 17:35:15, jbudorick wrote: > nit: "only support on bug id" -> "only supports one bug id" Done https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:47: return None On 2016/06/13 at 17:35:15, jbudorick wrote: > This shouldn't return here. It should only return None outside the for loop. Done https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:84: logging.info('Total count of tests in all test apks: %d', total_test_count) On 2016/06/13 at 17:35:15, jbudorick wrote: > nit: blank line before this one Done https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:157: script_runtime = datetime.datetime.utcnow() On 2016/06/13 at 17:35:15, jbudorick wrote: > Why are you doing this in main() and not in either _GetReport or _GetReportMeta? I know it's not pretty, but I save the json file as '%s-android-chrome.json' % script_runtime_string and upload the file with exact name to app engine https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:165: temp_output_dir = arguments.json_output_dir On 2016/06/13 at 17:35:15, jbudorick wrote: > nit: just use arguments.json_output_dir directly Done https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:171: On 2016/06/13 at 17:35:15, jbudorick wrote: > nit: +1 blank line Done https://codereview.chromium.org/1851143002/diff/260001/tools/android/find_ann... tools/android/find_annotated_tests.py:173: main() On 2016/06/13 at 17:35:15, jbudorick wrote: > nit: sys.exit(main()) Done
This script is changed to accommodate recipe changes in https://codereview.chromium.org/2063323002/ This script's both soul and body is ready for review https://codereview.chromium.org/1851143002/diff2/260001:320001/tools/android/...
lgtm w nits https://codereview.chromium.org/1851143002/diff/340001/tools/android/find_ann... File tools/android/find_annotated_tests.py (right): https://codereview.chromium.org/1851143002/diff/340001/tools/android/find_ann... tools/android/find_annotated_tests.py:39: """Store annotations in the existing annotation_dict and return bug id""" this is no longer accurate https://codereview.chromium.org/1851143002/diff/340001/tools/android/find_ann... tools/android/find_annotated_tests.py:44: search_result = re.search(_CRBUG_ID_PATTERN, content.get('message')) you should check that this actually matches before using it on the next line https://codereview.chromium.org/1851143002/diff/340001/tools/android/find_ann... tools/android/find_annotated_tests.py:117: 'platform': _PLATFORM_VALUE, nit: just put android inline here
yolandyan@chromium.org changed reviewers: + michaelbai@chromium.org
https://codereview.chromium.org/1851143002/diff/340001/tools/android/find_ann... File tools/android/find_annotated_tests.py (right): https://codereview.chromium.org/1851143002/diff/340001/tools/android/find_ann... tools/android/find_annotated_tests.py:39: """Store annotations in the existing annotation_dict and return bug id""" On 2016/06/16 at 21:04:53, jbudorick wrote: > this is no longer accurate Done https://codereview.chromium.org/1851143002/diff/340001/tools/android/find_ann... tools/android/find_annotated_tests.py:44: search_result = re.search(_CRBUG_ID_PATTERN, content.get('message')) On 2016/06/16 at 21:04:53, jbudorick wrote: > you should check that this actually matches before using it on the next line Done https://codereview.chromium.org/1851143002/diff/340001/tools/android/find_ann... tools/android/find_annotated_tests.py:117: 'platform': _PLATFORM_VALUE, On 2016/06/16 at 21:04:53, jbudorick wrote: > nit: just put android inline here Done
The CQ bit was checked by yolandyan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1851143002/#ps360001 (title: "Change nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1851143002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yolandyan@chromium.org changed reviewers: + xianzhanzhu@chromium.org
wangxianzhu@chromium.org changed reviewers: + wangxianzhu@chromium.org
rs lgtm.
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yolandyan@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/06/24 21:07:00, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) > ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) > mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) maybe needs rebasing again?
still lgtm w/ nits Sorry about the rebase conflict. I think that was my fault. https://codereview.chromium.org/1851143002/diff/420001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance_test.py (right): https://codereview.chromium.org/1851143002/diff/420001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:19: _INSTRUMENTATION_TEST_INSTANCE_PATH = ('pylib.instrumentation.' nit: I think you can fit the string on one line if you drop pylib.instrumentation down https://codereview.chromium.org/1851143002/diff/420001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:98: return_value=raw_tests): nit: indentation is off. either: - drop the first param onto its own line + indent four spaces - indent subsequent lines to the depth of the first param (same throughout) https://codereview.chromium.org/1851143002/diff/420001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:135: o._test_filter = 'org.chromium.test.SampleTest.testMethod1' nit: everything other than the GetTests call can be outside the mock.patch context manager. (same throughout)
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== BUG=601909 ========== to ========== Find annotated tests by exposing API in instrumentation_test_instance BUG=601909 ==========
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, wangxianzhu@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1851143002/#ps440001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Find annotated tests by exposing API in instrumentation_test_instance BUG=601909 ========== to ========== Find annotated tests by exposing API in instrumentation_test_instance BUG=601909 Committed: https://crrev.com/97dc1912066a4312ffac05dbad24880897be80e9 Cr-Commit-Position: refs/heads/master@{#402699} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/97dc1912066a4312ffac05dbad24880897be80e9 Cr-Commit-Position: refs/heads/master@{#402699}
Message was sent while issue was closed.
https://codereview.chromium.org/1851143002/diff/420001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance_test.py (right): https://codereview.chromium.org/1851143002/diff/420001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:19: _INSTRUMENTATION_TEST_INSTANCE_PATH = ('pylib.instrumentation.' On 2016/06/28 at 07:46:42, jbudorick (EMEA til June 30) wrote: > nit: I think you can fit the string on one line if you drop pylib.instrumentation down Done https://codereview.chromium.org/1851143002/diff/420001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:98: return_value=raw_tests): On 2016/06/28 at 07:46:42, jbudorick (EMEA til June 30) wrote: > nit: indentation is off. either: > - drop the first param onto its own line + indent four spaces > - indent subsequent lines to the depth of the first param > > (same throughout) Done |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
