Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14652029:
Alert on zero online devices when last devices file is missing or empty. (Closed)
|
Created:
7 years, 7 months ago by Siva Chandra Modified:
7 years, 7 months ago Reviewers:
Isaac (away) CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, klundberg+watch_chromium.org, ilevy+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAlert on zero online devices when last devices file is missing or empty.
BUG=238535
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198931
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix ilevy's nit #Messages
Total messages: 10 (0 generated)
https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... build/android/device_status_check.py:113: if not adb_online_devs and not last_devices: Not sure this is needed. Can you move the devices check on line 191 to line 167 and remove the if clause on 168?
https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... build/android/device_status_check.py:113: if not adb_online_devs and not last_devices: On 2013/05/07 01:57:08, Isaac wrote: > Not sure this is needed. Can you move the devices check on line 191 to line 167 > and remove the if clause on 168? Wouldn't it be useful to report last devices (if .last_devices was present) rather than returning immediately when no devices are found?
https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... build/android/device_status_check.py:113: if not adb_online_devs and not last_devices: On 2013/05/07 06:42:55, Siva Chandra wrote: > On 2013/05/07 01:57:08, Isaac wrote: > > Not sure this is needed. Can you move the devices check on line 191 to line > 167 > > and remove the if clause on 168? > > Wouldn't it be useful to report last devices (if .last_devices was present) > rather than returning immediately when no devices are found? But this bypasses all the other logic. Why not integrate. For example if you want to information give last_devices is present, you're now skipping lines 117-119. You're also not writing a .last_devices file anymore. In short, you cannot early return from this function.
https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... build/android/device_status_check.py:113: if not adb_online_devs and not last_devices: On 2013/05/07 07:20:13, Isaac wrote: > But this bypasses all the other logic. Why not integrate. For example if you > want to information give last_devices is present, you're now skipping lines > 117-119. The 'if' at line 117 is executed if there were missing devices. For missing devices to be present, last_devices have to exist. In which case, the 'if' block I added will not execute. > You're also not writing a .last_devices file anymore. .last_devices and .last_missing are written at line 110 and 111 above, before the new 'if' block.
lgtm w/ nit https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... File build/android/device_status_check.py (right): https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... build/android/device_status_check.py:110: WriteDeviceList('.last_devices', (adb_online_devs + last_devices)) make a variable: all_known_devices = adb_online_devs + last_devices and use on line 113 https://chromiumcodereview.appspot.com/14652029/diff/1/build/android/device_s... build/android/device_status_check.py:113: if not adb_online_devs and not last_devices: On 2013/05/07 07:43:10, Siva Chandra wrote: > On 2013/05/07 07:20:13, Isaac wrote: > > But this bypasses all the other logic. Why not integrate. For example if you > > want to information give last_devices is present, you're now skipping lines > > 117-119. > > The 'if' at line 117 is executed if there were missing devices. For missing > devices to be present, last_devices have to exist. In which case, the 'if' block > I added will not execute. > > > You're also not writing a .last_devices file anymore. > > .last_devices and .last_missing are written at line 110 and 111 above, before > the new 'if' block. Good point.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/14652029/11001
Retried try job too often on linux_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/14652029/11001
Message was sent while issue was closed.
Change committed as 198931 |
