Adding PyAuto Battery test for ChromeOS

chrome/test/functional/chromeos_battery.py

+#!/usr/bin/python
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that
+# can be found in the LICENSE file.
+
+import time
+import logging

[dtu, 2011/05/25 04:02:55]

Each section of imports should be alphabetized. Actually, if you follow
everything these guys say, these imports might become unnecessary and can be
removed.

[scottc, 2011/05/27 22:59:27]

Done.

+
+import pyauto_functional
+import pyauto
+
+from chromeos.power_strip import PowerStrip
+
+class ChromeosBattery(pyauto.PyUITest):
+  """Tests ChromeOS Battery Status API.

[Nirnimesh, 2011/05/25 00:45:57]

Mention upfront the setup assumptions for this test, like running in Stan's lab,
etc, access to the power strip, etc.

[scottc, 2011/05/27 22:59:27]

Done.

+
+     Battery Info is a dictionary with the following keys:

[Nirnimesh, 2011/05/25 00:45:57]

why this description? This is a repeat of the doc in GetBatteryInfo()

[scottc, 2011/05/27 22:59:27]

It explains why the battery status must be polled to get
a valid value for 'bettery_time_to_full' and 'battery_time_to_empty'.  I've
added this info to the description.

+      'battery_is_present': bool
+      'line_power_on': bool
+      if 'battery_is_present':
+        'battery_percentage': float (0 ~ 100)
+        'battery_fully_charged': bool
+        if 'line_power_on':
+          'battery_time_to_full': int (seconds)
+        else:
+          'battery_time_to_empty': int (seconds)
+
+    If battery is still calculating the time left, then
+    'battery_time_to_full' and 'battery_time_to_empty'
+    will be absent.
+    """
+
+  _OUTLET_WITH_BATTERY = '.a4'
+
+
+  def setUp(self):
+    pyauto.PyUITest.setUp(self)
+    POWERSTRIP_IPADDR = '172.22.12.99'

[Nirnimesh, 2011/05/25 00:45:57]

Please fetch ip address from a private file (like Stan uses
chrome/test/data/pyauto_private/chromeos/network/wifi_testbed_config for wifi
tests)

[Nirnimesh, 2011/05/25 00:45:57]

local variables should be small_letters_with_underscores

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Done.

+    POWERSTRIP_USERNAME = 'admn'

[stanleyw_google.com, 2011/05/25 00:06:00]

Username and password not necessary.  Default is admn/admn.

[scottc, 2011/05/27 22:59:27]

Removed.

+    POWERSTRIP_PASSWORD = 'admn'
+    self._power_strip =
+      PowerStrip(POWERSTRIP_IPADDR, POWERSTRIP_USERNAME, POWERSTRIP_PASSWORD)
+
+
+  def tearDown(self):
+    #self._turnOn(ChromeosBattery._OUTLET_WITH_BATTERY) # Leave power on

[Nirnimesh, 2011/05/25 00:45:57]

Remove commented out code

[scottc, 2011/05/27 22:59:27]

Done.

+    self._turnOff(ChromeosBattery._OUTLET_WITH_BATTERY) # Leave power off

[stanleyw_google.com, 2011/05/25 00:06:00]

You probably want to turn the device on after the tests are done.

[scottc, 2011/05/27 22:59:27]

Done.

+    pyauto.PyUITest.tearDown(self)
+
+
+  def _turnOn(self, outlet):

[Nirnimesh, 2011/05/25 00:45:57]

This is a one-liner function. Do you really need it?

[scottc, 2011/05/27 22:59:27]

Removed.

+    """Power on specified outlet"""
+    self._power_strip.PowerOn(outlet)
+

[stanleyw_google.com, 2011/05/25 00:06:00]

I think we can do without the _turnOn and turnOff function.  Just call
self._power_strip.PowerOn(outlet) instead of self._turnOn(outlet).

[scottc, 2011/05/27 22:59:27]

Removed.

+
+  def _turnOff(self, outlet):

[stanleyw_google.com, 2011/05/25 00:06:00]

Same as the comment for the _turnOn method.

[scottc, 2011/05/27 22:59:27]

Removed.

+    """Power off specified outlet"""
+    self._power_strip.PowerOff(outlet)
+
+
+  def WaitUntilBatteryTimeIsCalculated(self):

[Nirnimesh, 2011/05/25 00:45:57]

How long does this typically take?

[scottc, 2011/05/27 22:59:27]

Sometimes right away (<2 seconds), usually about 15-30, and sometimes 45.

+    battery_status = self.GetBatteryInfo()
+    if (battery_status['line_power_on'] == True):

[Nirnimesh, 2011/05/25 00:45:57]

remove outer parens

== True is redundant

[scottc, 2011/05/27 22:59:27]

Done.

+      time_key = 'battery_time_to_full'
+    else:
+      time_key = 'battery_time_to_empty'
+    logging.info('Waiting for key = %s' % time_key)
+    return self.WaitUntil(lambda: self._timeIsCalculated(time_key),

[stanleyw_google.com, 2011/05/25 00:06:00]

Instead of having _timeIsCalculated(time_key) maybe just
self.GetBatteryInfo().get(time_key).

[dtu, 2011/05/25 04:02:55]

Instead of returning this and doing assertTrue in the callers, just put
assertTrue here and don't return anything.

[scottc, 2011/05/27 22:59:27]

Removed. I originally put this in so the script would show progress .. i.e.,
that it was polling the battery for time status.  Since we decided that this
level of logging is not helpful to others, I've taken it out.

[scottc, 2011/05/27 22:59:27]

Done.

+      timeout=60,

[stanleyw_google.com, 2011/05/25 00:06:00]

This and the line below should line up with the word 'lambda' above. 

[scottc, 2011/05/27 22:59:27]

Done.

+      retry_sleep=2)
+
+
+  def _timeIsCalculated(self, time_key):
+    battery_status = self.GetBatteryInfo()
+    logging.info(str(battery_status))

[Nirnimesh, 2011/05/25 00:45:57]

remove

[scottc, 2011/05/27 22:59:27]

Removed. I like seeing progress during a test run when it takes a long time for
the battery time to calculate. But, since nobody is going to be watching the
output while these tests run, there is no need for this.

+    return battery_status.has_key(time_key)
+
+
+  def testBatteryCharging(self):
+    """AC power ON to CrOS device with battery"""

[stanleyw_google.com, 2011/05/25 00:06:00]

Period at the end.

[Nirnimesh, 2011/05/25 00:45:57]

It's not clear from this description what this test verifies.

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Renamed to testBatteryChargesWhenACisOn

+    self._turnOn(self._OUTLET_WITH_BATTERY)
+    time.sleep(2)

[stanleyw_google.com, 2011/05/25 00:06:00]

Why the sleep here?  Is it necessary?

[scottc, 2011/05/27 22:59:27]

Give PowerStrip switch time to settle.

+
+    """Get info about charging battery."""

[Nirnimesh, 2011/05/25 00:45:57]

Is this a comment? Prefix # in that case (do not use """)

[scottc, 2011/05/27 22:59:27]

Done.

+    self.assertTrue(self.WaitUntilBatteryTimeIsCalculated(),
+      'Battery charge time was not calculated')

[Nirnimesh, 2011/05/25 00:45:57]

indent by 2 more space chars

[krisr, 2011/05/25 05:35:29]

before the message string please use msg='the string'

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Done.

+    battery_status = self.GetBatteryInfo()
+    self.printBatteryStatus(battery_status)
+
+    self.assertTrue(battery_status['battery_is_present'],

[stanleyw_google.com, 2011/05/25 00:06:00]

It's probably better to use battery_status.get('battery_is_present'), in the
event that 'battery_is_present' key is not defined. 

[scottc, 2011/05/27 22:59:27]

Done.

+      'Battery is not present')

[stanleyw_google.com, 2011/05/25 00:06:00]

Period after present.

[krisr, 2011/05/25 05:35:29]

before the message string please use msg='the string'

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Done.

+    self.assertTrue(battery_status['line_power_on'],

[stanleyw_google.com, 2011/05/25 00:06:00]

Use battery_status.get(...).

[scottc, 2011/05/27 22:59:27]

Done.

+      'Line power is off')

[stanleyw_google.com, 2011/05/25 00:06:00]

Period after off.

[krisr, 2011/05/25 05:35:29]

before the message string please use msg='the string'

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Done.

+    self.assertTrue(battery_status['battery_time_to_full'] >= 0,

[stanleyw_google.com, 2011/05/25 00:06:00]

Use battery_status.get(...).

[scottc, 2011/05/27 22:59:27]

Done.

+      'Battery charge time is negative')

[stanleyw_google.com, 2011/05/25 00:06:00]

Period after negative.

[krisr, 2011/05/25 05:35:29]

before the message string please use msg='the string'

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Done.

+
+
+  def testBatteryDischarging(self):
+    """AC power OFF to CrOS device with battery"""

[stanleyw_google.com, 2011/05/25 00:06:00]

Period after battery.

[scottc, 2011/05/27 22:59:27]

Done.

+    self._turnOff(self._OUTLET_WITH_BATTERY)
+    time.sleep(2)

[stanleyw_google.com, 2011/05/25 00:06:00]

Why the sleep here?

[scottc, 2011/05/27 22:59:27]

Give switch and AC power adapter time to settle.

+
+    """Get info about discharging battery."""
+    self.assertTrue(self.WaitUntilBatteryTimeIsCalculated(),
+      'Battery discharge time was not calculated')
+    battery_status = self.GetBatteryInfo()
+    self.printBatteryStatus(battery_status)
+
+    self.assertTrue(battery_status['battery_is_present'],

[stanleyw_google.com, 2011/05/25 00:06:00]

It's probably better to use battery_status.get('battery_is_present'), in the
event that 'battery_is_present' key is not defined. 

[scottc, 2011/05/27 22:59:27]

Done.

+      'Battery is not present')

[stanleyw_google.com, 2011/05/25 00:06:00]

Period after present.

[scottc, 2011/05/27 22:59:27]

Done.

+    self.assertFalse(battery_status['line_power_on'],

[stanleyw_google.com, 2011/05/25 00:06:00]

Use battery_status.get(...).

[krisr, 2011/05/25 05:35:29]

before the message string please use msg='the string'

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Done.

+      'Line power is off')

[stanleyw_google.com, 2011/05/25 00:06:00]

Period after off.

[scottc, 2011/05/27 22:59:27]

Done.

+    self.assertTrue(battery_status['battery_time_to_empty'] >= 0,

[stanleyw_google.com, 2011/05/25 00:06:01]

Use battery_status.get(...).

[krisr, 2011/05/25 05:35:29]

before the message string please use msg='the string'

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Done.

+      'Battery discharge time is negative')

[stanleyw_google.com, 2011/05/25 00:06:01]

Period after negative.

[scottc, 2011/05/27 22:59:27]

Done.

+
+

[krisr, 2011/05/25 05:35:29]

I would like a third test that:
  - Turns on A/C
  - Get time until charged
  - Turn off A/C
  - Get time until discharged
  - Verify the two times are not equal.

[scottc, 2011/05/27 22:59:27]

Added testBatteryTimesAreDifferent()

+  def printBatteryStatus(self, battery_status):

[stanleyw_google.com, 2011/05/25 00:06:01]

We might not want to print all this.  I think a simple pass fail is good enough.
 Though for Nirnimesh to decide.

[Nirnimesh, 2011/05/25 00:45:57]

+1
You don't want to dump all this info. You're not going to remember what all this
in one month anyway.

[dtu, 2011/05/25 04:02:55]

Note that you can do this to achieve a similar output for debugging purposes:
import pprint
pprint.pprint(self.GetBatteryInfo())
But, as the above guys said, after you're done debugging be sure to remove the
excess printing.

[scottc, 2011/05/27 22:59:27]

Done.

[scottc, 2011/05/27 22:59:27]

Removed printing.

[scottc, 2011/05/27 22:59:27]

Printing removed.

+    battery_is_present = battery_status['battery_is_present']
+    line_power_on = battery_status['line_power_on']
+    if (battery_is_present):
+      battery_percentage = battery_status['battery_percentage']
+      battery_fully_charged = battery_status['battery_fully_charged']
+      if (line_power_on):
+        battery_secs_to_full = battery_status['battery_time_to_full']
+        battery_secs_to_empty = 'n/a'
+      else:
+        battery_secs_to_empty = battery_status['battery_time_to_empty']
+        battery_secs_to_full = 'n/a'
+    else:
+      battery_percentage = 'n/a'
+      battery_fully_charged = 'n/a'
+
+    logging.info('')
+    logging.info('Battery Status:')
+    logging.info('  Battery present        : %s' % str(battery_is_present))
+    logging.info('  Line power on          : %s' % str(line_power_on))
+    logging.info('  Battery percent        : %2.2f' % battery_percentage)
+    logging.info('  Battery fully charged  : %s' % str(battery_fully_charged))
+    logging.info('  Time to full (seconds) : %s' % battery_secs_to_full)
+    logging.info('  Time to empty (seconds): %s' % battery_secs_to_empty)
+    logging.info('')
+
+
+if __name__ == '__main__':
+  pyauto_functional.Main()