Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
284953002:
If no battery information, do not try to split. (Closed)
|
Created:
6 years, 7 months ago by navabi Modified:
6 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIf no battery information, do not try to split.
The x86 device can not dump battery information with 'adb dumbsys battery'. This
is causing the x86 instrumentation bot to fail on device status check. This is a
temporary fix until we determine how to check battery info on x86 devices.
BUG=371719
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270516
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move inline logic for None battery output. #
Total comments: 2
Patch Set 3 : Add space for indentation. #
Total comments: 1
Patch Set 4 : Address nits. #Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/284953002/diff/1/build/android/buildbot/bb_de... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/284953002/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:74: ' %s' % '\n '.join(battery.split('\n')) if battery else '', This inline if is a bit dense and will be tough for anyone who doesn't know python well. Factor out into a named variable.
https://codereview.chromium.org/284953002/diff/1/build/android/buildbot/bb_de... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/284953002/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:74: ' %s' % '\n '.join(battery.split('\n')) if battery else '', On 2014/05/13 22:25:43, luqui wrote: > This inline if is a bit dense and will be tough for anyone who doesn't know > python well. Factor out into a named variable. Better yet, a function "indent", to state your intent
https://codereview.chromium.org/284953002/diff/1/build/android/buildbot/bb_de... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/284953002/diff/1/build/android/buildbot/bb_de... build/android/buildbot/bb_device_status_check.py:74: ' %s' % '\n '.join(battery.split('\n')) if battery else '', On 2014/05/13 22:25:43, luqui wrote: > This inline if is a bit dense and will be tough for anyone who doesn't know > python well. Factor out into a named variable. Done. I didn't think we needed an 'indent' function just for that part in _GetBatteryInfo()
lgtm % comment https://codereview.chromium.org/284953002/diff/20001/build/android/buildbot/b... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/284953002/diff/20001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:69: return battery_info[0] + '\n '.join(battery_info[1:]) Missed a space after \n
Thanks Luke. https://codereview.chromium.org/284953002/diff/20001/build/android/buildbot/b... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/284953002/diff/20001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:69: return battery_info[0] + '\n '.join(battery_info[1:]) On 2014/05/13 22:47:33, luqui wrote: > Missed a space after \n Done.
lgtm % nit, thanks! https://codereview.chromium.org/284953002/diff/40001/build/android/buildbot/b... File build/android/buildbot/bb_device_status_check.py (right): https://codereview.chromium.org/284953002/diff/40001/build/android/buildbot/b... build/android/buildbot/bb_device_status_check.py:67: if battery: nit: perhaps like _GetData above, it saves one indented line :) if not battery: return 'Unknown' battery_info = battery.split('\n') return battery_info[0] + '\n '.join(battery_info[1:])
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/284953002/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/284953002/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/284953002/60001
Message was sent while issue was closed.
Change committed as 270516 |
