Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791613003:
[Instrumentation Test Speed] Install apks in parallel with concurrent_adb enabled (Closed)
|
Created:
3 years, 8 months ago by shenghuazhang Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Instrumentation Test Speed] Install apks in parallel with concurrent_adb enabled
The current install_apk installs all apks one by one. This CL enables
apks installation in parallel when execute the test with
command '--enable-concurrent-adb'. In this way, the total
individial_device_set_up time decreases.
BUG=670302
Review-Url: https://codereview.chromium.org/2791613003
Cr-Commit-Position: refs/heads/master@{#463529}
Committed: https://chromium.googlesource.com/chromium/src/+/7ff10a8c4f7295430af4d3bac32ffb713fb42761
Patch Set 1 #
Total comments: 2
Patch Set 2 : John Comment #
Total comments: 5
Patch Set 3 : John comment #Patch Set 4 : John comment #
Total comments: 13
Patch Set 5 : John & Case comment #Patch Set 6 : John & Case Comment #
Total comments: 6
Patch Set 7 : Case comment #
Total comments: 2
Patch Set 8 : agrieve & jbudorick comment #Messages
Total messages: 23 (7 generated)
shenghuazhang@chromium.org changed reviewers: + agrieve@chromium.org, jbudorick@chromium.org
Test speed comparison results by this CL is documented in this doc: https://docs.google.com/a/google.com/document/d/1IBGvSFpXv7mcJ6fvjIO_KeQNU-9S...
https://codereview.chromium.org/2791613003/diff/1/build/android/pylib/local/d... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:128: pool = ThreadPool(processes=len(apk_resource_array)) Instead of using a ThreadPool, split apk installation into multiple steps... https://codereview.chromium.org/2791613003/diff/1/build/android/pylib/local/d... build/android/pylib/local/device/local_device_instrumentation_test_run.py:200: steps = (install_apk, edit_shared_prefs, push_test_data, ... and then add the steps here.
Description was changed from ========== [Instrumentation Test Speed] Install apks in parallel with concurrent_adb enabled The current install_apk installs all apks one by one. This CL enables apks installation in parallel with ThreadPool when execute the test with command '--enable-concurrent-adb'. In this way, the total individial_device_set_up time decreases. BUG=670302 ========== to ========== [Instrumentation Test Speed] Install apks in parallel with concurrent_adb enabled The current install_apk installs all apks one by one. This CL enables apks installation in parallel when execute the test with command '--enable-concurrent-adb'. In this way, the total individial_device_set_up time decreases. BUG=670302 ==========
shenghuazhang@chromium.org changed reviewers: + mikecase@chromium.org
+mikecase as reviewer.
https://codereview.chromium.org/2791613003/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:78: def single_apk_install(apk_permission_pair): These three functions shouldn't be necessary w/ the suggestion below. https://codereview.chromium.org/2791613003/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:99: def install_apk(): Rather than keeping install_apk and having it return a list of steps, why not just pull the separate installation steps into their own steps and remove install_apk? e.g. steps = [] if self._test_instance.apk_under_test: if self._test_instance.apk_under_test_incremental_install_script: steps.append(lambda: local_device_test_run.IncrementalInstall(...)) else: steps.append(lambda: dev.Install(...)) ... steps.extend([lambda apk: dev.Install(apk) for apk in self._test_instance.additional_apks])
https://codereview.chromium.org/2791613003/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:99: def install_apk(): On 2017/04/04 02:52:23, jbudorick wrote: > Rather than keeping install_apk and having it return a list of steps, why not > just pull the separate installation steps into their own steps and remove > install_apk? e.g. > > steps = [] > > if self._test_instance.apk_under_test: > if self._test_instance.apk_under_test_incremental_install_script: > steps.append(lambda: local_device_test_run.IncrementalInstall(...)) > else: > steps.append(lambda: dev.Install(...)) > ... > steps.extend([lambda apk: dev.Install(apk) for apk in > self._test_instance.additional_apks]) My last line of ^ doesn't work, but something like this should as discussed offline def install_helper(apk): return lambda: dev.Install(apk) def install_helper(apk): def internal_helper(): dev.Install(apk) return internal_helper steps.extend([install_helper(apk) for apk in ...])
https://codereview.chromium.org/2791613003/diff/20001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:78: def single_apk_install(apk_permission_pair): On 2017/04/04 02:52:23, jbudorick wrote: > These three functions shouldn't be necessary w/ the suggestion below. Done. https://codereview.chromium.org/2791613003/diff/20001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:99: def install_apk(): On 2017/04/04 23:03:47, jbudorick wrote: > On 2017/04/04 02:52:23, jbudorick wrote: > > Rather than keeping install_apk and having it return a list of steps, why not > > just pull the separate installation steps into their own steps and remove > > install_apk? e.g. > > > > steps = [] > > > > if self._test_instance.apk_under_test: > > if self._test_instance.apk_under_test_incremental_install_script: > > steps.append(lambda: local_device_test_run.IncrementalInstall(...)) > > else: > > steps.append(lambda: dev.Install(...)) > > ... > > steps.extend([lambda apk: dev.Install(apk) for apk in > > self._test_instance.additional_apks]) > > My last line of ^ doesn't work, but something like this should as discussed > offline > > def install_helper(apk): > return lambda: dev.Install(apk) > > def install_helper(apk): > def internal_helper(): > dev.Install(apk) > return internal_helper > > steps.extend([install_helper(apk) for apk in ...]) > Done. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:91: steps.append(install_helper(self._test_instance.apk_under_test, Seems like python doesn't create closure binding the value 'permissions' to dev.Install(apk, permissions=permissions), and if pass this: steps.append(lambda: dev.Install(..., permissions=permission)) it will throw SecurityException e.g. 'Package org.chromium.chrome has not requested permission android.permission.READ_LOGS'. So use the install_helper here and below.
This seems really sweet. Some comments. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:106: for apk in self._test_instance.additional_apks]) nit: some of the indentation looks off. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:111: def install_apk_func(): Would rename to something like... sequential_install_function ...just so its purpose is clearer. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:184: steps = install_apk_steps + [set_debug_app, edit_shared_prefs, Could something go wrong is set_debug_app step is run before the apk is installed? Unsure if the command work if package not installed yet. It might.
https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:184: steps = install_apk_steps + [set_debug_app, edit_shared_prefs, On 2017/04/05 at 15:46:10, mikecase wrote: > Could something go wrong is set_debug_app step is run before the apk is installed? Unsure if the command work if package not installed yet. It might. Tested it. Seems to work fine.
https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:78: def install_apk(): This function should no longer exist. We should just create steps earlier in individual_device_set_up's scope and append to it directly. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:108: if self._env.concurrent_adb: Again: we shouldn't need to branch based on this here. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:111: def install_apk_func(): As mentioned, this shouldn't exist. Use the same steps.
Did a quick research about set-debug-app. Please look into the comment. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:78: def install_apk(): On 2017/04/05 16:32:29, jbudorick wrote: > This function should no longer exist. We should just create steps earlier in > individual_device_set_up's scope and append to it directly. Done. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:106: for apk in self._test_instance.additional_apks]) On 2017/04/05 15:46:10, mikecase wrote: > nit: some of the indentation looks off. Done. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:108: if self._env.concurrent_adb: On 2017/04/05 16:32:29, jbudorick wrote: > Again: we shouldn't need to branch based on this here. Done. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:111: def install_apk_func(): On 2017/04/05 16:32:29, jbudorick wrote: > As mentioned, this shouldn't exist. Use the same steps. Done. https://codereview.chromium.org/2791613003/diff/60001/build/android/pylib/loc... build/android/pylib/local/device/local_device_instrumentation_test_run.py:184: steps = install_apk_steps + [set_debug_app, edit_shared_prefs, On 2017/04/05 16:29:06, mikecase wrote: > On 2017/04/05 at 15:46:10, mikecase wrote: > > Could something go wrong is set_debug_app step is run before the apk is > installed? Unsure if the command work if package not installed yet. It might. > > Tested it. Seems to work fine. Thanks for bringing this up! I just do a quick research about it. The '--persistent' option of the set-debug-app command would 'retain this value', s.t., this option survives app re-installations and even uninstall-install sequence. So I think it would be fine to run set_debug_app step before apk installed. Referred from the blog: http://android-dev-life.blogspot.com/2015/02/do-you-adb-shell-am-set-debug-ap...
lgtm! just nits. https://codereview.chromium.org/2791613003/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:80: return lambda: dev.Install(apk, permissions=permissions) super nit: I think you should put empty line after install_helper() definition https://codereview.chromium.org/2791613003/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:87: apk_under_test_incremental_install_script)) nit: i think this line should be intented more than the line above it. Not sure if that pushes it above 80 chars. If so, you might have to put lambda on a new line. https://codereview.chromium.org/2791613003/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:98: test_apk_incremental_install_script)) nit: i think this line should be intented more than the line above it. Not sure if that pushes it above 80 chars. If so, you might have to put lambda on a new line.
https://codereview.chromium.org/2791613003/diff/100001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:80: return lambda: dev.Install(apk, permissions=permissions) On 2017/04/08 00:50:12, mikecase wrote: > super nit: I think you should put empty line after install_helper() definition Done. https://codereview.chromium.org/2791613003/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:87: apk_under_test_incremental_install_script)) On 2017/04/08 00:50:12, mikecase wrote: > nit: i think this line should be intented more than the line above it. Not sure > if that pushes it above 80 chars. If so, you might have to put lambda on a new > line. This change pushes above 80 characters. Will move the lambda to a new line. https://codereview.chromium.org/2791613003/diff/100001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:98: test_apk_incremental_install_script)) On 2017/04/08 00:50:12, mikecase wrote: > nit: i think this line should be intented more than the line above it. Not sure > if that pushes it above 80 chars. If so, you might have to put lambda on a new > line. This one is fine.
lgtm https://codereview.chromium.org/2791613003/diff/120001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/120001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:106: steps.extend([install_helper(apk, None) nit: you don't need the []s here.
lgtm https://codereview.chromium.org/2791613003/diff/120001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2791613003/diff/120001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:96: steps.append(lambda: local_device_test_run.IncrementalInstall( optional nit: maybe move this into a helper for consistency w/ non-incremental.
The CQ bit was checked by shenghuazhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2791613003/#ps140001 (title: "agrieve & jbudorick comment")
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": 140001, "attempt_start_ts": 1491879573451690,
"parent_rev": "6eaf7fc310350fba0b59738040d6f97b640d553e", "commit_rev":
"7ff10a8c4f7295430af4d3bac32ffb713fb42761"}
Message was sent while issue was closed.
Description was changed from ========== [Instrumentation Test Speed] Install apks in parallel with concurrent_adb enabled The current install_apk installs all apks one by one. This CL enables apks installation in parallel when execute the test with command '--enable-concurrent-adb'. In this way, the total individial_device_set_up time decreases. BUG=670302 ========== to ========== [Instrumentation Test Speed] Install apks in parallel with concurrent_adb enabled The current install_apk installs all apks one by one. This CL enables apks installation in parallel when execute the test with command '--enable-concurrent-adb'. In this way, the total individial_device_set_up time decreases. BUG=670302 Review-Url: https://codereview.chromium.org/2791613003 Cr-Commit-Position: refs/heads/master@{#463529} Committed: https://chromium.googlesource.com/chromium/src/+/7ff10a8c4f7295430af4d3bac32f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/7ff10a8c4f7295430af4d3bac32f... |
