Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
321403002:
Add option device in install apk script (Closed)
|
Created:
6 years, 6 months ago by Yang Gu Modified:
6 years, 6 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, bulach Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd option device in install apk script
Sometimes we need to designate device to install apk on, so this option
is needed.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277050
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update according to comment #
Total comments: 1
Patch Set 3 : Remove () in help #Patch Set 4 : Rebase to latest code #Messages
Total messages: 20 (0 generated)
Please review.
Thank you for the patch Yang. I'm not a peer but I have a few minor comments about the patch, see below. https://codereview.chromium.org/321403002/diff/1/build/android/adb_install_ap... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/321403002/diff/1/build/android/adb_install_ap... build/android/adb_install_apk.py:54: def _GetAttachedDevices(device=None): It seems that the _Foo() convention isn't used in that file. Methods are called Foo() instead. https://codereview.chromium.org/321403002/diff/1/build/android/adb_install_ap... build/android/adb_install_apk.py:54: def _GetAttachedDevices(device=None): I would call the method GetTargetDevices() instead because it will not return attached devices only, it can return only one device. https://codereview.chromium.org/321403002/diff/1/build/android/adb_install_ap... build/android/adb_install_apk.py:67: assert device in attached_devices, ( It seems that we use |raise Exception('Error: ...')| instead of assert in this file.
Thanks a lot for prompt comments! The function _GetAttachedDevices() is just a copy from build/android/test_runner.py (I only changed the parameter name from test_device to device as it's not suitable here). I think it's better to keep consistency within these scripts.
+jbudorick for OWNERS
I don't really have an objection to adding this, but I am curious about the context in which it will be used. Is a script handling different devices attached to a host differently? Is this primarily intended for manual use? https://codereview.chromium.org/321403002/diff/1/build/android/adb_install_ap... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/321403002/diff/1/build/android/adb_install_ap... build/android/adb_install_apk.py:86: devices = _GetAttachedDevices(options.device) I'd much rather see something like this both here and in test_runner.py (though I don't expect you to change the latter): devices = android_commands.GetAttachedDevices() if options.device: if options.device not in devices: raise Exception('Error: %s not in attached devices %s' % (device, ' '.join(devices))) devices = [options.device]
On 2014/06/11 14:32:00, jbudorick wrote: > I don't really have an objection to adding this, but I am curious about the > context in which it will be used. Is a script handling different devices > attached to a host differently? Is this primarily intended for manual use? We typically have several devices (real hardware and emulator) connected to same host machine. And to do regular test, we wrote some scripts and sometimes, we just use them to test part of devices. Recently, we use adb_install_apk.py to install ContentShell.apk apk and ContentShellTest.apk so that we can enable instrumentation test. So this feature is necessary for us to install apk on designated device within a fully automatic test. > https://codereview.chromium.org/321403002/diff/1/build/android/adb_install_ap... > File build/android/adb_install_apk.py (right): > > https://codereview.chromium.org/321403002/diff/1/build/android/adb_install_ap... > build/android/adb_install_apk.py:86: devices = > _GetAttachedDevices(options.device) > I'd much rather see something like this both here and in test_runner.py (though > I don't expect you to change the latter): > > devices = android_commands.GetAttachedDevices() > > if options.device: > if options.device not in devices: > raise Exception('Error: %s not in attached devices %s' % (device, ' > '.join(devices))) > devices = [options.device] I have updated CL as you suggested.
lgtm
lgtm https://codereview.chromium.org/321403002/diff/20001/build/android/adb_instal... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/321403002/diff/20001/build/android/adb_instal... build/android/adb_install_apk.py:42: help=('Target device for apk to install on.')) no () required.
On 2014/06/12 17:25:47, frankf wrote: > lgtm > > https://codereview.chromium.org/321403002/diff/20001/build/android/adb_instal... > File build/android/adb_install_apk.py (right): > > https://codereview.chromium.org/321403002/diff/20001/build/android/adb_instal... > build/android/adb_install_apk.py:42: help=('Target device for apk to install > on.')) > no () required. Thanks! I removed () and will try to commit.
The CQ bit was checked by yang.gu@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yang.gu@intel.com/321403002/40001
The CQ bit was checked by yang.gu@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yang.gu@intel.com/321403002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by yang.gu@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yang.gu@intel.com/321403002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 277050 |
