Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632763003:
Change apk_helper.py for apk with multi instrumentations and JUnit4 (Closed)
|
Created:
3 years, 11 months ago by the real yoland Modified:
3 years, 11 months ago CC:
catapult-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionChange apk_helper.py for apk with multi instrumentations and JUnit4
Review-Url: https://codereview.chromium.org/2632763003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e22cb5a59ab593d629131fa69b16bde3ad27d83a
Patch Set 1 #
Total comments: 2
Patch Set 2 : add GetInstrumentationInfo method #
Total comments: 1
Patch Set 3 : return all instrumentations #
Total comments: 13
Patch Set 4 : format mistake #Patch Set 5 : Add mock tests #
Total comments: 7
Patch Set 6 : Fix GetActivityName #Patch Set 7 : Change name to new_apk_helper.py #Patch Set 8 : Change name back #
Total comments: 14
Patch Set 9 : nits and teardown #
Total comments: 2
Patch Set 10 : nits #
Messages
Total messages: 26 (6 generated)
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
https://codereview.chromium.org/2632763003/diff/1/devil/devil/android/apk_hel... File devil/devil/android/apk_helper.py (right): https://codereview.chromium.org/2632763003/diff/1/devil/devil/android/apk_hel... devil/devil/android/apk_helper.py:97: #TODO(yolandyan): clear JUnit3 related code once migration is over This is too hacky. It should be returning multiple instrumentation names, and the test runner should sort out which ones it cares about.
Description was changed from ========== Change apk_helper.py for apk with multi instrumentations and JUnit4 BUG=catapult:# ========== to ========== Change apk_helper.py for apk with multi instrumentations and JUnit4 ==========
https://codereview.chromium.org/2632763003/diff/1/devil/devil/android/apk_hel... File devil/devil/android/apk_helper.py (right): https://codereview.chromium.org/2632763003/diff/1/devil/devil/android/apk_hel... devil/devil/android/apk_helper.py:97: #TODO(yolandyan): clear JUnit3 related code once migration is over On 2017/01/14 at 03:17:01, jbudorick wrote: > This is too hacky. It should be returning multiple instrumentation names, and the test runner should sort out which ones it cares about. Ack https://codereview.chromium.org/2632763003/diff/20001/devil/devil/android/apk... File devil/devil/android/apk_helper.py (left): https://codereview.chromium.org/2632763003/diff/20001/devil/devil/android/apk... devil/devil/android/apk_helper.py:23: # TODO(jbudorick): Deprecate and remove this function once callers have been No callers using this anymore
The reason a number of `[0]` addition to is...
Previously, _GetManifest() returns a dict like this:
{'manifest': {'android:versionCode': ['0x1'],
'android:versionName': ['Developer Build'],
'application': {'activity': {'android:exported': ['0xffffffff'],
'android:name':
['org.chromium.test.broker.OnDeviceInstrumentationBroker']},
'uses-library': {'android:name':
['android.test.runner']}},
'instrumentation': {'android:label': ['Android JUnit 4 runner',
'Tests for
org.chromium.content_shell_apk'],
'android:name':
['org.chromium.base.test.BaseJUnitRunner',
'org.chromium.base.test.BaseChromiumInstrumentationTestRunner'],
'android:targetPackage':
['org.chromium.content_shell_apk',
'org.chromium.content_shell_apk'],
'junit4': ['0xffffffff (Raw: "true")']},
'package': ['org.chromium.content_shell_apk.tests'],
'platformBuildVersionCode': ['0x18 (Raw: "24")'],
'platformBuildVersionName': ['0x40e00000 (Raw: "7.0")'],
'uses-permission': {'android:name':
['android.permission.RUN_INSTRUMENTATION']},
'uses-sdk': {'android:minSdkVersion': ['0x10'],
'android:targetSdkVersion': ['0x17']}}}
Now _GetManifest() returns:
{'manifest': [{'android:versionCode': ['0x1'],
'android:versionName': ['Developer Build'],
'application': [{'activity': [{'android:exported':
['0xffffffff'],
'android:name':
['org.chromium.test.broker.OnDeviceInstrumentationBroker']}],
'uses-library': [{'android:name':
['android.test.runner']}]}],
'instrumentation': [{'android:label': ['Android JUnit 4 runner'],
'android:name':
['org.chromium.base.test.BaseJUnitRunner'],
'android:targetPackage':
['org.chromium.content_shell_apk'],
'junit4': ['0xffffffff (Raw: "true")']},
{'android:label': ['Tests for
org.chromium.content_shell_apk'],
'android:name':
['org.chromium.base.test.BaseChromiumInstrumentationTestRunner'],
'android:targetPackage':
['org.chromium.content_shell_apk']}],
'package': ['org.chromium.content_shell_apk.tests'],
'platformBuildVersionCode': ['0x18 (Raw: "24")'],
'platformBuildVersionName': ['0x40e00000 (Raw: "7.0")'],
'uses-permission': [{'android:name':
['android.permission.RUN_INSTRUMENTATION']}],
'uses-sdk': [{'android:minSdkVersion': ['0x10'],
'android:targetSdkVersion': ['0x17']}]}]}
Change the script use GetAllInstrumentations() to return all instrumentation information instead of junit specific dictionary
I think this looks pretty good. Just some comments. https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... File devil/devil/android/apk_helper.py (right): https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:103: all_instru = self.GetAllInstrumentations(default=default) all_instru isnt a great name https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:105: raise Exception("There isn't just one instrumentation.") Clearer error message would be something like. "There is more than one instrumentation. Expected one." https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:110: self, default='android.test.InstrumentationTestRunner'): It seems strange to have a default like this. https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:147: manifest_info['manifest'][0]['application']['service']] Why isnt this ... manifest_info['manifest'][0]['application'][0]['service'][0] ? It seems you made everything into a dict of lists?
https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... File devil/devil/android/apk_helper.py (right): https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:101: def GetInstrumentationName( This still needs a docstring. If you have it raise an exception, that should be documented. https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:103: all_instru = self.GetAllInstrumentations(default=default) nit: all_instr or all_instrumentations https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:105: raise Exception("There isn't just one instrumentation.") Don't just raise Exception. This should likely raise a dedicated BaseError subclass. https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:128: return [p['android:name'][0] for What's the motivation behind the non-instrumentation changes here?
https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... File devil/devil/android/apk_helper.py (right): https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:101: def GetInstrumentationName( On 2017/01/23 at 18:20:06, jbudorick wrote: > This still needs a docstring. > > If you have it raise an exception, that should be documented. Done https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:103: all_instru = self.GetAllInstrumentations(default=default) On 2017/01/23 at 18:20:06, jbudorick wrote: > nit: all_instr or all_instrumentations Done https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:105: raise Exception("There isn't just one instrumentation.") On 2017/01/23 at 18:20:06, jbudorick wrote: > Don't just raise Exception. This should likely raise a dedicated BaseError subclass. Done https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:128: return [p['android:name'][0] for On 2017/01/23 at 18:20:06, jbudorick wrote: > What's the motivation behind the non-instrumentation changes here? this changed is because how Manifest is parsed changed (apply to all elements and attributes), before when a manifest has multiple elements, the result is a single dictionary that maps string to a list of attribute values. Now because extra attribute is added to instrumentation, the return is a list of dictionaries, in which each dictionary maps string to string. e.g. <use-permission name="a"/><use-permission name="b"/> before: {"use-permission": {"name":["a", "b"]} after: {"use-permission": [{"name": "a"}, {"name": "b"}]} https://codereview.chromium.org/2632763003/diff/40001/devil/devil/android/apk... devil/devil/android/apk_helper.py:147: manifest_info['manifest'][0]['application']['service']] On 2017/01/23 at 18:13:57, mikecase wrote: > Why isnt this ... > > manifest_info['manifest'][0]['application'][0]['service'][0] ? > > It seems you made everything into a dict of lists? mistakes were made, changed
https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... File devil/devil/android/apk_helper.py (right): https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... devil/devil/android/apk_helper.py:94: manifest_info['manifest'][0]['application']['activity'][0] Doesn't application have to have a [0] after it? I might be crazy. manifest_info['manifest'][0]['application'][0]['activity'][0] https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... devil/devil/android/apk_helper.py:110: "There is more than one instrumentation. Expected one.") super nit: single quotes https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... devil/devil/android/apk_helper.py:120: return [{'android:name':default}] nit: space after : https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... devil/devil/android/apk_helper.py:120: return [{'android:name':default}] nit: space after :
https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... File devil/devil/android/apk_helper.py (right): https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... devil/devil/android/apk_helper.py:94: manifest_info['manifest'][0]['application']['activity'][0] On 2017/01/23 at 22:10:54, mikecase wrote: > Doesn't application have to have a [0] after it? > I might be crazy. > > manifest_info['manifest'][0]['application'][0]['activity'][0] Yes, you are right...holy thanks for that catch! https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... devil/devil/android/apk_helper.py:110: "There is more than one instrumentation. Expected one.") On 2017/01/23 at 22:10:55, mikecase wrote: > super nit: single quotes Done https://codereview.chromium.org/2632763003/diff/80001/devil/devil/android/apk... devil/devil/android/apk_helper.py:120: return [{'android:name':default}] On 2017/01/23 at 22:10:55, mikecase wrote: > nit: space after : Done
This lgtm.
Why did we rename this?
On 2017/01/24 at 00:06:51, jbudorick wrote: > Why did we rename this? I was stupid, changed
https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... File devil/devil/android/apk_helper_test.py (right): https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:8: #TODO(yolandyan): change back to apk_helper after change is done Reimplementing apk_helper as a separate module and gradually changing over isn't stupid :) I just don't think it's necessary in this case. (also, you can obviously remove this TODO) https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:15: def StartPatcher(return_value): This should be _StartPatcher if it stays a module-scope function. https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:20: nit: +1 blank line https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:24: 'N: android=http://schemas.android.com/apk/res/android', This should be formatted as one string literal and it should exceed 80 characters where necessary. If you need separate lines, split if afterwards. https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:67: StartPatcher(return_value) If you're going to do this in setUp, you should be tearing down the patcher in tearDown. https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:78: nit: -1 blank line https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:79: def testGetAllInstrumentation(self): nit: testGetAllInstrumentations
https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... File devil/devil/android/apk_helper_test.py (right): https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:8: #TODO(yolandyan): change back to apk_helper after change is done On 2017/01/24 at 14:16:06, jbudorick wrote: > Reimplementing apk_helper as a separate module and gradually changing over isn't stupid :) I just don't think it's necessary in this case. > > (also, you can obviously remove this TODO) Done https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:15: def StartPatcher(return_value): On 2017/01/24 at 14:16:06, jbudorick wrote: > This should be _StartPatcher if it stays a module-scope function. Done https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:20: On 2017/01/24 at 14:16:06, jbudorick wrote: > nit: +1 blank line Done https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:24: 'N: android=http://schemas.android.com/apk/res/android', On 2017/01/24 at 14:16:06, jbudorick wrote: > This should be formatted as one string literal and it should exceed 80 characters where necessary. If you need separate lines, split if afterwards. Nooo :( my day was gone trying to make this split pretty Done https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:67: StartPatcher(return_value) On 2017/01/24 at 14:16:06, jbudorick wrote: > If you're going to do this in setUp, you should be tearing down the patcher in tearDown. Done https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:78: On 2017/01/24 at 14:16:06, jbudorick wrote: > nit: -1 blank line Done https://codereview.chromium.org/2632763003/diff/140001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:79: def testGetAllInstrumentation(self): On 2017/01/24 at 14:16:06, jbudorick wrote: > nit: testGetAllInstrumentations Done
lgtm w/ nit https://codereview.chromium.org/2632763003/diff/160001/devil/devil/android/ap... File devil/devil/android/apk_helper_test.py (right): https://codereview.chromium.org/2632763003/diff/160001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:44: def _StartPatcher(manifest_dump): Does this need to be a separate method? Can you just create and start the patcher in setUp?
https://codereview.chromium.org/2632763003/diff/160001/devil/devil/android/ap... File devil/devil/android/apk_helper_test.py (right): https://codereview.chromium.org/2632763003/diff/160001/devil/devil/android/ap... devil/devil/android/apk_helper_test.py:44: def _StartPatcher(manifest_dump): On 2017/01/24 at 16:09:31, jbudorick wrote: > Does this need to be a separate method? Can you just create and start the patcher in setUp? I was originally thinking I needed to split test into different classes. But this test class covered all normal cases. Changed
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2632763003/#ps180001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485274398449260,
"parent_rev": "fd92b59934073a130c3634ffd4ef71570de86089", "commit_rev":
"e22cb5a59ab593d629131fa69b16bde3ad27d83a"}
Message was sent while issue was closed.
Description was changed from ========== Change apk_helper.py for apk with multi instrumentations and JUnit4 ========== to ========== Change apk_helper.py for apk with multi instrumentations and JUnit4 Review-Url: https://codereview.chromium.org/2632763003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2656103002/ by yolandyan@chromium.org. The reason for reverting is: https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
