Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
879983002:
Add multiple device/os filtering and a config file. (Closed)
|
Created:
5 years, 11 months ago by rnephew (Reviews Here) Modified:
5 years, 10 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd multiple device/os filtering and a config file.
BUG=
Committed: https://crrev.com/241e6870d1f5cb7e2f23a8b1edc33eb6db64ad24
Cr-Commit-Position: refs/heads/master@{#314151}
Patch Set 1 #
Total comments: 18
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 34
Patch Set 4 : #
Total comments: 3
Patch Set 5 : Fix variables not being set #
Total comments: 10
Patch Set 6 : #
Total comments: 2
Patch Set 7 : fix logic #
Total comments: 13
Patch Set 8 : #
Total comments: 4
Patch Set 9 : #
Total comments: 1
Patch Set 10 : #
Total comments: 1
Messages
Total messages: 33 (4 generated)
rnephew@google.com changed reviewers: + jbudorick@chromium.org, rnephew@google.com
https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... File build/android/pylib/remote/device/example.config (right): https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/example.config:1: { - Why is this a separate file? - Why isn't this just a JSON? https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:7: from distutils.version import LooseVersion Do we know we'll have this everywhere? Also, if this sticks, it should just be "from distutils import version" and "version.LooseVersion(...)" https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:53: self._remote_device = args.remote_device.split(',') I'm not sure these should necessarily support multiples in a single argument. What if we just made a user pass in multiple instances of --remote-device if that's what they want? https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:57: self._remote_device_os = args.remote_device_os.split(',') ditto https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:76: self._remote_device = device_json['remote_device'].split(',') oh, it is json. Why aren't remote_device and remote_device_os just lists? https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:87: self._api_key = device_json['api_key'] Most of these can just be self._blah = device_json.get('blah', None) https://codereview.chromium.org/879983002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/879983002/diff/1/build/android/test_runner.py... build/android/test_runner.py:145: ' remote device. Overrides all other flags.')) I imagine this and remote-device + remote-device-os should be mutually exclusive.
https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... File build/android/pylib/remote/device/example.config (right): https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/example.config:1: { On 2015/01/27 17:58:55, jbudorick wrote: > - Why is this a separate file? > - Why isn't this just a JSON? I could add it as a comment somewhere. I just wanted to give an example config file. It is. https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:7: from distutils.version import LooseVersion On 2015/01/27 17:58:55, jbudorick wrote: > Do we know we'll have this everywhere? > > Also, if this sticks, it should just be "from distutils import version" and > "version.LooseVersion(...)" I didnt' have to install it, and as far as I can find it comes with all python versions after 2.6. https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:53: self._remote_device = args.remote_device.split(',') On 2015/01/27 17:58:55, jbudorick wrote: > I'm not sure these should necessarily support multiples in a single argument. > What if we just made a user pass in multiple instances of --remote-device if > that's what they want? Done. https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:57: self._remote_device_os = args.remote_device_os.split(',') On 2015/01/27 17:58:55, jbudorick wrote: > ditto Done. https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:76: self._remote_device = device_json['remote_device'].split(',') On 2015/01/27 17:58:55, jbudorick wrote: > oh, it is json. > > Why aren't remote_device and remote_device_os just lists? Done. https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/remote_device_environment.py:87: self._api_key = device_json['api_key'] On 2015/01/27 17:58:55, jbudorick wrote: > Most of these can just be > > self._blah = device_json.get('blah', None) I went with ..get('blah', self._blah) that way if you set it on the command line it will still get set there. https://codereview.chromium.org/879983002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/879983002/diff/1/build/android/test_runner.py... build/android/test_runner.py:145: ' remote device. Overrides all other flags.')) On 2015/01/27 17:58:55, jbudorick wrote: > I imagine this and remote-device + remote-device-os should be mutually > exclusive. Under my current implementation you can set somethings from the command line. So you could have --remote-device set on the command line and not have it present in the config file, but have other things set there. Doing that would get rid of this; is that ok?
https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... File build/android/pylib/remote/device/example.config (right): https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/example.config:1: { On 2015/01/27 18:46:21, rnephew wrote: > On 2015/01/27 17:58:55, jbudorick wrote: > > - Why is this a separate file? > > - Why isn't this just a JSON? > > I could add it as a comment somewhere. I just wanted to give an example config > file. > > It is. Yeah, I think this should be a comment somewhere. https://codereview.chromium.org/879983002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/879983002/diff/1/build/android/test_runner.py... build/android/test_runner.py:145: ' remote device. Overrides all other flags.')) On 2015/01/27 18:46:21, rnephew wrote: > On 2015/01/27 17:58:55, jbudorick wrote: > > I imagine this and remote-device + remote-device-os should be mutually > > exclusive. > > Under my current implementation you can set somethings from the command line. So > you could have --remote-device set on the command line and not have it present > in the config file, but have other things set there. Doing that would get rid of > this; is that ok? I think we should start by supporting a user providing either the remote-device-file or everything it provides, but not both concurrently. We can always revisit that if users (or bots) want to mix the two. https://codereview.chromium.org/879983002/diff/20001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/20001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:7: from distutils import version changed my mind, import distutils.version I don't think any of our system imports use "from x import y" https://codereview.chromium.org/879983002/diff/20001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:106: Perhaps we should log the attributes here? (Minus the login credentials.) As-is, if a bot uses a remote-device-file (and they will), it will be very hard for a user to see how it's configured. https://codereview.chromium.org/879983002/diff/20001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:181: for device in device_list: It may be helpful to be able to select on OEM (I think it's 'brand' in the json).
https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... File build/android/pylib/remote/device/example.config (right): https://codereview.chromium.org/879983002/diff/1/build/android/pylib/remote/d... build/android/pylib/remote/device/example.config:1: { On 2015/01/27 20:01:50, jbudorick wrote: > On 2015/01/27 18:46:21, rnephew wrote: > > On 2015/01/27 17:58:55, jbudorick wrote: > > > - Why is this a separate file? > > > - Why isn't this just a JSON? > > > > I could add it as a comment somewhere. I just wanted to give an example config > > file. > > > > It is. > > Yeah, I think this should be a comment somewhere. Added comment to remote_device_environment. https://codereview.chromium.org/879983002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/879983002/diff/1/build/android/test_runner.py... build/android/test_runner.py:145: ' remote device. Overrides all other flags.')) On 2015/01/27 20:01:50, jbudorick wrote: > On 2015/01/27 18:46:21, rnephew wrote: > > On 2015/01/27 17:58:55, jbudorick wrote: > > > I imagine this and remote-device + remote-device-os should be mutually > > > exclusive. > > > > Under my current implementation you can set somethings from the command line. > So > > you could have --remote-device set on the command line and not have it present > > in the config file, but have other things set there. Doing that would get rid > of > > this; is that ok? > > I think we should start by supporting a user providing either the > remote-device-file or everything it provides, but not both concurrently. We can > always revisit that if users (or bots) want to mix the two. Done. https://codereview.chromium.org/879983002/diff/20001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/20001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:7: from distutils import version On 2015/01/27 20:01:50, jbudorick wrote: > changed my mind, import distutils.version > > I don't think any of our system imports use "from x import y" Done. https://codereview.chromium.org/879983002/diff/20001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:106: On 2015/01/27 20:01:50, jbudorick wrote: > Perhaps we should log the attributes here? (Minus the login credentials.) > > As-is, if a bot uses a remote-device-file (and they will), it will be very hard > for a user to see how it's configured. Done. https://codereview.chromium.org/879983002/diff/20001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:181: for device in device_list: On 2015/01/27 20:01:50, jbudorick wrote: > It may be helpful to be able to select on OEM (I think it's 'brand' in the > json). Done.
https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:55: self._device_oem=args.device_oem nit: spaces around = https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:69: # "remote_device": ["Galaxy S4", "Galaxy S3"], nit: 2 space indent https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:78: # "queueing": 600, nit: 2 space indent https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:85: and ( args.remote_device_os or args.remote_device)): nit: no space after ( https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:101: self._timeouts = device_json.get('timeouts', self._timeouts) Could we let a user specify only some of the timeouts, then use the default values for whatever isn't specified? https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:222: if (self._device_oem and device['brand'] != self._device_oem): Let's make device_oem a list, too. https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... build/android/test_runner.py:128: help=('Only collects the test results if set. ' w.r.t. help formatting: like this https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... build/android/test_runner.py:145: group.add_argument('--remote-device-file', help=('File with JSON to select' nit: put help on its own line and indent subsequent lines (e.g. above) https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... build/android/test_runner.py:148: device_os_group = group.add_mutually_exclusive_group() I still need to look at nested mutex groups.
https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:55: self._device_oem=args.device_oem On 2015/01/28 16:09:54, jbudorick wrote: > nit: spaces around = Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:69: # "remote_device": ["Galaxy S4", "Galaxy S3"], On 2015/01/28 16:09:54, jbudorick wrote: > nit: 2 space indent Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:78: # "queueing": 600, On 2015/01/28 16:09:54, jbudorick wrote: > nit: 2 space indent Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:85: and ( args.remote_device_os or args.remote_device)): On 2015/01/28 16:09:54, jbudorick wrote: > nit: no space after ( Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:101: self._timeouts = device_json.get('timeouts', self._timeouts) On 2015/01/28 16:09:54, jbudorick wrote: > Could we let a user specify only some of the timeouts, then use the default > values for whatever isn't specified? Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:222: if (self._device_oem and device['brand'] != self._device_oem): On 2015/01/28 16:09:54, jbudorick wrote: > Let's make device_oem a list, too. Done. https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... build/android/test_runner.py:128: help=('Only collects the test results if set. ' On 2015/01/28 16:09:54, jbudorick wrote: > w.r.t. help formatting: like this Acknowledged. https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... build/android/test_runner.py:145: group.add_argument('--remote-device-file', help=('File with JSON to select' On 2015/01/28 16:09:54, jbudorick wrote: > nit: put help on its own line and indent subsequent lines (e.g. above) Done. https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... build/android/test_runner.py:148: device_os_group = group.add_mutually_exclusive_group() On 2015/01/28 16:09:54, jbudorick wrote: > I still need to look at nested mutex groups. Looking around, everything I found says you cant do it; but SO isn't always correct on things.
https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:71: # "remote_device_minimum_os": "4.4.2" comma at the end https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:72: # "api_address": www.example.com needs quotes around example and a comma at the end https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:96: self._api_address = device_json.get('api_address', None) I think you mentioned this somewhere (out-of-band?) but I've come around to the idea of doing self._blah = device_json.get('blah', self._blah) In fact, I wonder if the command-line should override the file (instead of vice versa)? https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:107: 'or in --remote-device-file.') same as below https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:113: 'or in --remote-device-file') needs a space before "or" https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:116: 'or in --remote-device-file') same https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:119: 'or in --remote-device-file') same https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... build/android/test_runner.py:148: device_os_group = group.add_mutually_exclusive_group() On 2015/01/28 16:47:53, rnephew wrote: > On 2015/01/28 16:09:54, jbudorick wrote: > > I still need to look at nested mutex groups. > > Looking around, everything I found says you cant do it; but SO isn't always > correct on things. Yeah, it doesn't look like it works the way I'd like it to. Oh well.
https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:71: # "remote_device_minimum_os": "4.4.2" On 2015/01/28 23:19:09, jbudorick wrote: > comma at the end Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:72: # "api_address": www.example.com On 2015/01/28 23:19:09, jbudorick wrote: > needs quotes around example and a comma at the end Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:96: self._api_address = device_json.get('api_address', None) On 2015/01/28 23:19:09, jbudorick wrote: > I think you mentioned this somewhere (out-of-band?) but I've come around to the > idea of doing > > self._blah = device_json.get('blah', self._blah) > > In fact, I wonder if the command-line should override the file (instead of vice > versa)? I kept them as none, and switched them to have command line override the file. It seemed easier to do it that way. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:107: 'or in --remote-device-file.') On 2015/01/28 23:19:09, jbudorick wrote: > same as below Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:113: 'or in --remote-device-file') On 2015/01/28 23:19:09, jbudorick wrote: > needs a space before "or" Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:116: 'or in --remote-device-file') On 2015/01/28 23:19:09, jbudorick wrote: > same Done. https://codereview.chromium.org/879983002/diff/40001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:119: 'or in --remote-device-file') On 2015/01/28 23:19:09, jbudorick wrote: > same Done. https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... File build/android/test_runner.py (right): https://codereview.chromium.org/879983002/diff/40001/build/android/test_runne... build/android/test_runner.py:148: device_os_group = group.add_mutually_exclusive_group() On 2015/01/28 23:19:09, jbudorick wrote: > On 2015/01/28 16:47:53, rnephew wrote: > > On 2015/01/28 16:09:54, jbudorick wrote: > > > I still need to look at nested mutex groups. > > > > Looking around, everything I found says you cant do it; but SO isn't always > > correct on things. > > Yeah, it doesn't look like it works the way I'd like it to. Oh well. Acknowledged.
https://codereview.chromium.org/879983002/diff/60001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/60001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:34: if (args.remote_device_file Since it now has command line options overriding the file options; I don't know if it makes sense to do this exclusion anymore.
https://codereview.chromium.org/879983002/diff/60001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/60001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:34: if (args.remote_device_file On 2015/01/29 15:44:26, rnephew wrote: > Since it now has command line options overriding the file options; I don't know > if it makes sense to do this exclusion anymore. I don't think it does. https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:59: # "queueing": 600, nit: 2-space indent relative to timeouts https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:71: self._remote_device = device_json.get('remote_device', None) nit: These sections would be easier to read if alphabetized. https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:101: if args.api_protocol: We should log something (probably at info level) if a command line overrides a non-None value. https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:137: if not isinstance(self._timeouts, dict): Is this even possible? https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:139: if ('queueing' not in self._timeouts Same question.
https://codereview.chromium.org/879983002/diff/60001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/60001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:34: if (args.remote_device_file On 2015/01/29 17:52:56, jbudorick wrote: > On 2015/01/29 15:44:26, rnephew wrote: > > Since it now has command line options overriding the file options; I don't > know > > if it makes sense to do this exclusion anymore. > > I don't think it does. Done. https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:59: # "queueing": 600, On 2015/01/29 17:52:56, jbudorick wrote: > nit: 2-space indent relative to timeouts Done. https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:71: self._remote_device = device_json.get('remote_device', None) On 2015/01/29 17:52:56, jbudorick wrote: > nit: These sections would be easier to read if alphabetized. Done. https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:101: if args.api_protocol: On 2015/01/29 17:52:56, jbudorick wrote: > We should log something (probably at info level) if a command line overrides a > non-None value. Done. https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:137: if not isinstance(self._timeouts, dict): On 2015/01/29 17:52:56, jbudorick wrote: > Is this even possible? Before yes, after the last set of changes no. https://codereview.chromium.org/879983002/diff/80001/build/android/pylib/remo... build/android/pylib/remote/device/remote_device_environment.py:139: if ('queueing' not in self._timeouts On 2015/01/29 17:52:56, jbudorick wrote: > Same question. Same.
https://codereview.chromium.org/879983002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:106: if args.api_address: Fixing these to only be one if statement.
https://codereview.chromium.org/879983002/diff/100001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/100001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:106: if args.api_address: On 2015/01/29 22:12:00, rnephew wrote: > Fixing these to only be one if statement. Actually I had incorrect up logic and the self._blah = args.blah shouldn't be with the logging statement.
https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:66: self._api_address = device_json.get('api_address', None) Shouldn't we only do the device_json parsing if --remote-device-file is passed? No point in doing it if device_json is {}, right? https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:107: if self._api_address and self._api_address != args.api_address: This blocok is pretty repetitive. Can we make it into a function? (Probably a local one?) e.g. maybe something like this: def maybe_assign_and_log_override(attr, arg, desc): if attr: if attr and attr != arg: logging.info('Overriding %s from %s to %s', desc, attr, arg) attr = arg maybe_assign_and_log_override(self._api_address, args.api_address, 'api_address') https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:134: if( self._remote_device_os nit: if (self._remote_device_os https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:141: logging.info('Overriding device oem from %s to %s.', nit: one space after from https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:185: logging.info('Remote device minimum os: %s', nit: OS https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:187: logging.info('Remote device os: %s', self._remote_device_os) same nit
https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:107: if self._api_address and self._api_address != args.api_address: On 2015/01/30 14:24:59, jbudorick wrote: > This blocok is pretty repetitive. Can we make it into a function? (Probably a Wow, "blocok" ... yikes. > local one?) e.g. maybe something like this: > > def maybe_assign_and_log_override(attr, arg, desc): > if attr: > if attr and attr != arg: > logging.info('Overriding %s from %s to %s', desc, attr, arg) > attr = arg > > maybe_assign_and_log_override(self._api_address, args.api_address, > 'api_address')
https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:66: self._api_address = device_json.get('api_address', None) On 2015/01/30 14:24:59, jbudorick wrote: > Shouldn't we only do the device_json parsing if --remote-device-file is passed? > No point in doing it if device_json is {}, right? Talked offline about why Im doing this. https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:107: if self._api_address and self._api_address != args.api_address: On 2015/01/30 14:24:59, jbudorick wrote: > This blocok is pretty repetitive. Can we make it into a function? (Probably a > local one?) e.g. maybe something like this: > > def maybe_assign_and_log_override(attr, arg, desc): > if attr: > if attr and attr != arg: > logging.info('Overriding %s from %s to %s', desc, attr, arg) > attr = arg > > maybe_assign_and_log_override(self._api_address, args.api_address, > 'api_address') Done. https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:134: if( self._remote_device_os On 2015/01/30 14:24:59, jbudorick wrote: > nit: if (self._remote_device_os Done. https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:141: logging.info('Overriding device oem from %s to %s.', On 2015/01/30 14:24:59, jbudorick wrote: > nit: one space after from Done. https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:185: logging.info('Remote device minimum os: %s', On 2015/01/30 14:24:59, jbudorick wrote: > nit: OS Done. https://codereview.chromium.org/879983002/diff/120001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:187: logging.info('Remote device os: %s', self._remote_device_os) On 2015/01/30 14:24:59, jbudorick wrote: > same nit Done.
lgtm w/ nits https://codereview.chromium.org/879983002/diff/140001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/140001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:84: def command_line_override(file_set, cmd_line_set, desc): nit: *_set -> *_value https://codereview.chromium.org/879983002/diff/140001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:116: if args.api_key_file: nit: Either add a comment explaining why this doesn't use command_line_override or add use command_line_override w/ an option that prevents logging values.
https://codereview.chromium.org/879983002/diff/140001/build/android/pylib/rem... File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/879983002/diff/140001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:84: def command_line_override(file_set, cmd_line_set, desc): On 2015/01/30 17:56:40, jbudorick wrote: > nit: *_set -> *_value Done. https://codereview.chromium.org/879983002/diff/140001/build/android/pylib/rem... build/android/pylib/remote/device/remote_device_environment.py:116: if args.api_key_file: On 2015/01/30 17:56:40, jbudorick wrote: > nit: Either add a comment explaining why this doesn't use command_line_override > or add use command_line_override w/ an option that prevents logging values. Done.
The CQ bit was checked by rnephew@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/879983002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_gn_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)
The CQ bit was checked by rnephew@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/879983002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/241e6870d1f5cb7e2f23a8b1edc33eb6db64ad24 Cr-Commit-Position: refs/heads/master@{#314151}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/890143004/ by rnephew@google.com. The reason for reverting is: Not working on bots..
Message was sent while issue was closed.
https://codereview.chromium.org/879983002/diff/160001/build/android/test_runn... File build/android/test_runner.py (left): https://codereview.chromium.org/879983002/diff/160001/build/android/test_runn... build/android/test_runner.py:124: group.add_argument('--trigger', default='', This appears to be the problem: RemoteDeviceTestRun was depending on self._env.trigger being exactly '' s.t. it was an instance of basestring but also evaluated to False.
Message was sent while issue was closed.
On 2015/02/02 19:14:30, jbudorick wrote: > https://codereview.chromium.org/879983002/diff/160001/build/android/test_runn... > File build/android/test_runner.py (left): > > https://codereview.chromium.org/879983002/diff/160001/build/android/test_runn... > build/android/test_runner.py:124: group.add_argument('--trigger', default='', > This appears to be the problem: RemoteDeviceTestRun was depending on > self._env.trigger being exactly '' s.t. it was an instance of basestring but > also evaluated to False. I was having it check self._env.trigger when it should have been checking self._env.collect. trigger being '' was masking this logic error.
Message was sent while issue was closed.
https://codereview.chromium.org/879983002/diff/180001/build/android/pylib/uir... File build/android/pylib/uirobot/uirobot_test_instance.py (right): https://codereview.chromium.org/879983002/diff/180001/build/android/pylib/uir... build/android/pylib/uirobot/uirobot_test_instance.py:33: device_type = device_json.get('device_type', 'Android') Perhaps we should either: - have the remote_device_file parsing logic elsewhere (i.e. outside both the environment and the test instance), or - have this suite and package_name logic in the test run s.t. it has access to the environment
Message was sent while issue was closed.
On 2015/02/02 20:28:06, jbudorick wrote: > https://codereview.chromium.org/879983002/diff/180001/build/android/pylib/uir... > File build/android/pylib/uirobot/uirobot_test_instance.py (right): > > https://codereview.chromium.org/879983002/diff/180001/build/android/pylib/uir... > build/android/pylib/uirobot/uirobot_test_instance.py:33: device_type = > device_json.get('device_type', 'Android') > Perhaps we should either: > - have the remote_device_file parsing logic elsewhere (i.e. outside both the > environment and the test instance), or > - have this suite and package_name logic in the test run s.t. it has access to > the environment ah, wrong review. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
