Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6990069:
Adding PyAuto Battery test for ChromeOS (Closed)
|
Created:
9 years, 7 months ago by scottc Modified:
9 years, 6 months ago CC:
chromium-reviews, John Grabowski, anantha, Nirnimesh, dyu1, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdding PyAuto Battery test for ChromeOS
BUG=none
TEST=run chromeos_battery.py.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88633
Patch Set 1 #
Total comments: 92
Patch Set 2 : '' #
Total comments: 17
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 18
Patch Set 5 : '' #
Total comments: 21
Patch Set 6 : '' #Patch Set 7 : '' #Messages
Total messages: 18 (0 generated)
Review me!
Some Comments. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:39: POWERSTRIP_USERNAME = 'admn' Username and password not necessary. Default is admn/admn. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:47: self._turnOff(ChromeosBattery._OUTLET_WITH_BATTERY) # Leave power off You probably want to turn the device on after the tests are done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:54: I think we can do without the _turnOn and turnOff function. Just call self._power_strip.PowerOn(outlet) instead of self._turnOn(outlet). http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:56: def _turnOff(self, outlet): Same as the comment for the _turnOn method. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:68: return self.WaitUntil(lambda: self._timeIsCalculated(time_key), Instead of having _timeIsCalculated(time_key) maybe just self.GetBatteryInfo().get(time_key). http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:69: timeout=60, This and the line below should line up with the word 'lambda' above. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:80: """AC power ON to CrOS device with battery""" Period at the end. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:82: time.sleep(2) Why the sleep here? Is it necessary? http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:90: self.assertTrue(battery_status['battery_is_present'], It's probably better to use battery_status.get('battery_is_present'), in the event that 'battery_is_present' key is not defined. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:91: 'Battery is not present') Period after present. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:92: self.assertTrue(battery_status['line_power_on'], Use battery_status.get(...). http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:93: 'Line power is off') Period after off. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:94: self.assertTrue(battery_status['battery_time_to_full'] >= 0, Use battery_status.get(...). http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:95: 'Battery charge time is negative') Period after negative. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:99: """AC power OFF to CrOS device with battery""" Period after battery. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:101: time.sleep(2) Why the sleep here? http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:109: self.assertTrue(battery_status['battery_is_present'], It's probably better to use battery_status.get('battery_is_present'), in the event that 'battery_is_present' key is not defined. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:110: 'Battery is not present') Period after present. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:111: self.assertFalse(battery_status['line_power_on'], Use battery_status.get(...). http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:112: 'Line power is off') Period after off. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:113: self.assertTrue(battery_status['battery_time_to_empty'] >= 0, Use battery_status.get(...). http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:114: 'Battery discharge time is negative') Period after negative. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:117: def printBatteryStatus(self, battery_status): We might not want to print all this. I think a simple pass fail is good enough. Though for Nirnimesh to decide.
http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:15: """Tests ChromeOS Battery Status API. Mention upfront the setup assumptions for this test, like running in Stan's lab, etc, access to the power strip, etc. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:17: Battery Info is a dictionary with the following keys: why this description? This is a repeat of the doc in GetBatteryInfo() http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:38: POWERSTRIP_IPADDR = '172.22.12.99' Please fetch ip address from a private file (like Stan uses chrome/test/data/pyauto_private/chromeos/network/wifi_testbed_config for wifi tests) http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:38: POWERSTRIP_IPADDR = '172.22.12.99' local variables should be small_letters_with_underscores http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:46: #self._turnOn(ChromeosBattery._OUTLET_WITH_BATTERY) # Leave power on Remove commented out code http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:51: def _turnOn(self, outlet): This is a one-liner function. Do you really need it? http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:61: def WaitUntilBatteryTimeIsCalculated(self): How long does this typically take? http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:63: if (battery_status['line_power_on'] == True): remove outer parens == True is redundant http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:75: logging.info(str(battery_status)) remove http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:80: """AC power ON to CrOS device with battery""" It's not clear from this description what this test verifies. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:84: """Get info about charging battery.""" Is this a comment? Prefix # in that case (do not use """) http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:86: 'Battery charge time was not calculated') indent by 2 more space chars http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:117: def printBatteryStatus(self, battery_status): On 2011/05/25 00:06:01, stanleyw1 wrote: > We might not want to print all this. I think a simple pass fail is good enough. > Though for Nirnimesh to decide. +1 You don't want to dump all this info. You're not going to remember what all this in one month anyway.
http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:7: import logging Each section of imports should be alphabetized. Actually, if you follow everything these guys say, these imports might become unnecessary and can be removed. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:68: return self.WaitUntil(lambda: self._timeIsCalculated(time_key), Instead of returning this and doing assertTrue in the callers, just put assertTrue here and don't return anything. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:117: def printBatteryStatus(self, battery_status): 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.
http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:86: 'Battery charge time was not calculated') before the message string please use msg='the string' http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:91: 'Battery is not present') before the message string please use msg='the string' http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:93: 'Line power is off') before the message string please use msg='the string' http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:95: 'Battery charge time is negative') before the message string please use msg='the string' http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:111: self.assertFalse(battery_status['line_power_on'], before the message string please use msg='the string' http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:113: self.assertTrue(battery_status['battery_time_to_empty'] >= 0, before the message string please use msg='the string' http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:116: 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.
Suggested changes incorporated. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:7: import logging On 2011/05/25 04:02:55, Dave Tu wrote: > Each section of imports should be alphabetized. Actually, if you follow > everything these guys say, these imports might become unnecessary and can be > removed. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:15: """Tests ChromeOS Battery Status API. On 2011/05/25 00:45:57, Nirnimesh wrote: > Mention upfront the setup assumptions for this test, like running in Stan's lab, > etc, access to the power strip, etc. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:17: Battery Info is a dictionary with the following keys: On 2011/05/25 00:45:57, Nirnimesh wrote: > why this description? This is a repeat of the doc in GetBatteryInfo() 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. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:38: POWERSTRIP_IPADDR = '172.22.12.99' On 2011/05/25 00:45:57, Nirnimesh wrote: > local variables should be small_letters_with_underscores Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:38: POWERSTRIP_IPADDR = '172.22.12.99' On 2011/05/25 00:45:57, Nirnimesh wrote: > Please fetch ip address from a private file (like Stan uses > chrome/test/data/pyauto_private/chromeos/network/wifi_testbed_config for wifi > tests) Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:39: POWERSTRIP_USERNAME = 'admn' On 2011/05/25 00:06:00, stanleyw1 wrote: > Username and password not necessary. Default is admn/admn. Removed. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:46: #self._turnOn(ChromeosBattery._OUTLET_WITH_BATTERY) # Leave power on On 2011/05/25 00:45:57, Nirnimesh wrote: > Remove commented out code Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:47: self._turnOff(ChromeosBattery._OUTLET_WITH_BATTERY) # Leave power off On 2011/05/25 00:06:00, stanleyw1 wrote: > You probably want to turn the device on after the tests are done. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:51: def _turnOn(self, outlet): On 2011/05/25 00:45:57, Nirnimesh wrote: > This is a one-liner function. Do you really need it? Removed. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:54: On 2011/05/25 00:06:00, stanleyw1 wrote: > I think we can do without the _turnOn and turnOff function. Just call > self._power_strip.PowerOn(outlet) instead of self._turnOn(outlet). Removed. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:56: def _turnOff(self, outlet): On 2011/05/25 00:06:00, stanleyw1 wrote: > Same as the comment for the _turnOn method. Removed. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:61: def WaitUntilBatteryTimeIsCalculated(self): On 2011/05/25 00:45:57, Nirnimesh wrote: > How long does this typically take? Sometimes right away (<2 seconds), usually about 15-30, and sometimes 45. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:63: if (battery_status['line_power_on'] == True): On 2011/05/25 00:45:57, Nirnimesh wrote: > remove outer parens > > == True is redundant Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:68: return self.WaitUntil(lambda: self._timeIsCalculated(time_key), On 2011/05/25 00:06:00, stanleyw1 wrote: > Instead of having _timeIsCalculated(time_key) maybe just > self.GetBatteryInfo().get(time_key). 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. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:68: return self.WaitUntil(lambda: self._timeIsCalculated(time_key), On 2011/05/25 00:06:00, stanleyw1 wrote: > Instead of having _timeIsCalculated(time_key) maybe just > self.GetBatteryInfo().get(time_key). Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:69: timeout=60, On 2011/05/25 00:06:00, stanleyw1 wrote: > This and the line below should line up with the word 'lambda' above. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:75: logging.info(str(battery_status)) On 2011/05/25 00:45:57, Nirnimesh wrote: > remove 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. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:80: """AC power ON to CrOS device with battery""" On 2011/05/25 00:06:00, stanleyw1 wrote: > Period at the end. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:80: """AC power ON to CrOS device with battery""" On 2011/05/25 00:45:57, Nirnimesh wrote: > It's not clear from this description what this test verifies. Renamed to testBatteryChargesWhenACisOn http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:82: time.sleep(2) On 2011/05/25 00:06:00, stanleyw1 wrote: > Why the sleep here? Is it necessary? Give PowerStrip switch time to settle. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:84: """Get info about charging battery.""" On 2011/05/25 00:45:57, Nirnimesh wrote: > Is this a comment? Prefix # in that case (do not use """) Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:86: 'Battery charge time was not calculated') On 2011/05/25 00:45:57, Nirnimesh wrote: > indent by 2 more space chars Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:86: 'Battery charge time was not calculated') On 2011/05/25 00:45:57, Nirnimesh wrote: > indent by 2 more space chars Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:90: self.assertTrue(battery_status['battery_is_present'], On 2011/05/25 00:06:00, stanleyw1 wrote: > It's probably better to use battery_status.get('battery_is_present'), in the > event that 'battery_is_present' key is not defined. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:91: 'Battery is not present') On 2011/05/25 00:06:00, stanleyw1 wrote: > Period after present. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:91: 'Battery is not present') On 2011/05/25 00:06:00, stanleyw1 wrote: > Period after present. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:92: self.assertTrue(battery_status['line_power_on'], On 2011/05/25 00:06:00, stanleyw1 wrote: > Use battery_status.get(...). Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:93: 'Line power is off') On 2011/05/25 05:35:29, krisr wrote: > before the message string please use msg='the string' Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:93: 'Line power is off') On 2011/05/25 00:06:00, stanleyw1 wrote: > Period after off. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:94: self.assertTrue(battery_status['battery_time_to_full'] >= 0, On 2011/05/25 00:06:00, stanleyw1 wrote: > Use battery_status.get(...). Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:95: 'Battery charge time is negative') On 2011/05/25 00:06:00, stanleyw1 wrote: > Period after negative. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:95: 'Battery charge time is negative') On 2011/05/25 05:35:29, krisr wrote: > before the message string please use msg='the string' Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:99: """AC power OFF to CrOS device with battery""" On 2011/05/25 00:06:00, stanleyw1 wrote: > Period after battery. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:101: time.sleep(2) On 2011/05/25 00:06:00, stanleyw1 wrote: > Why the sleep here? Give switch and AC power adapter time to settle. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:109: self.assertTrue(battery_status['battery_is_present'], On 2011/05/25 00:06:00, stanleyw1 wrote: > It's probably better to use battery_status.get('battery_is_present'), in the > event that 'battery_is_present' key is not defined. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:110: 'Battery is not present') On 2011/05/25 00:06:00, stanleyw1 wrote: > Period after present. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:111: self.assertFalse(battery_status['line_power_on'], On 2011/05/25 00:06:00, stanleyw1 wrote: > Use battery_status.get(...). Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:111: self.assertFalse(battery_status['line_power_on'], On 2011/05/25 05:35:29, krisr wrote: > before the message string please use msg='the string' Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:112: 'Line power is off') On 2011/05/25 00:06:00, stanleyw1 wrote: > Period after off. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:113: self.assertTrue(battery_status['battery_time_to_empty'] >= 0, On 2011/05/25 00:06:01, stanleyw1 wrote: > Use battery_status.get(...). Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:113: self.assertTrue(battery_status['battery_time_to_empty'] >= 0, On 2011/05/25 05:35:29, krisr wrote: > before the message string please use msg='the string' Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:114: 'Battery discharge time is negative') On 2011/05/25 00:06:01, stanleyw1 wrote: > Period after negative. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:116: On 2011/05/25 05:35:29, krisr wrote: > 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. Added testBatteryTimesAreDifferent() http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:117: def printBatteryStatus(self, battery_status): On 2011/05/25 00:45:57, Nirnimesh wrote: > On 2011/05/25 00:06:01, stanleyw1 wrote: > > We might not want to print all this. I think a simple pass fail is good > enough. > > Though for Nirnimesh to decide. > > +1 > You don't want to dump all this info. You're not going to remember what all this > in one month anyway. Done. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:117: def printBatteryStatus(self, battery_status): On 2011/05/25 00:06:01, stanleyw1 wrote: > We might not want to print all this. I think a simple pass fail is good enough. > Though for Nirnimesh to decide. Removed printing. http://codereview.chromium.org/6990069/diff/1/chrome/test/functional/chromeos... chrome/test/functional/chromeos_battery.py:117: def printBatteryStatus(self, battery_status): On 2011/05/25 04:02:55, Dave Tu wrote: > 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. Printing removed.
http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:6: import logging Unused import. Remove. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:10: import pyauto_functional Please say this: import pyauto_functional # Must be imported before pyauto http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:27: Battery Info is a dictionary with keys: You don't need to include the entire dictionary in a comment here, it's already in the GetBatteryInfo() doc. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:53: One blank line between function definitions inside a class. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:77: retry_sleep=2) Probably want to shorten the timeout and keep the default retry_sleep (which is 0.25s). 60 seconds is a long time for a test to hang. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:83: time.sleep(2) # Wait for swtich to settle switch* http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:100: time.sleep(2) # Wait for switch to settle Does it take some time for it to notice that the line power is on/off? In that case, use something like: self.assertTrue(self.WaitUntil(lambda: self.GetBatteryInfo()['line_power_on'])) Just calling time.sleep() in tests is discouraged because the timing can change across different builds or platforms, leading to a flaky test. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:138: 'If test failed falsely, Kris ows Scott a beer. ') owes*
Comments incorporated. One issue open on timeout. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:6: import logging On 2011/05/27 23:33:43, Dave Tu wrote: > Unused import. Remove. Done. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:10: import pyauto_functional On 2011/05/27 23:33:43, Dave Tu wrote: > Please say this: > import pyauto_functional # Must be imported before pyauto Done. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:27: Battery Info is a dictionary with keys: On 2011/05/27 23:33:43, Dave Tu wrote: > You don't need to include the entire dictionary in a comment here, it's already > in the GetBatteryInfo() doc. Removed. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:53: On 2011/05/27 23:33:43, Dave Tu wrote: > One blank line between function definitions inside a class. Done. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:77: retry_sleep=2) On 2011/05/27 23:33:43, Dave Tu wrote: > Probably want to shorten the timeout and keep the default retry_sleep (which is > 0.25s). 60 seconds is a long time for a test to hang. During testing, I've seen it sometimes take up to 40 seconds for the time_to_full and time_to_empty to calculate. Is there a stated requirement for the longest it can take? If so, we should use that as the timeout. If not, timeout should be high enough to eliminate false failures. I see that power_BatteryCharge.py uses 60 seconds for the timeout (http://git.chromium.org/gitweb/?p=chromiumos/third_party/autotest.git;a=blob;...). http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:83: time.sleep(2) # Wait for swtich to settle On 2011/05/27 23:33:43, Dave Tu wrote: > switch* Removed. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:100: time.sleep(2) # Wait for switch to settle On 2011/05/27 23:33:43, Dave Tu wrote: > Does it take some time for it to notice that the line power is on/off? In that > case, use something like: > self.assertTrue(self.WaitUntil(lambda: self.GetBatteryInfo()['line_power_on'])) > Just calling time.sleep() in tests is discouraged because the timing can change > across different builds or platforms, leading to a flaky test. Removed. http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:138: 'If test failed falsely, Kris ows Scott a beer. ') On 2011/05/27 23:33:43, Dave Tu wrote: > owes* Done.
A few minor comments only. Thanks http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:15: """Tests ChromeOS Battery Status API. nit: leave a blank line after this line Remove 'API' in this sentence http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:17: 1) Device under test (DUT) is connected to the corporate LAN remove reference to 'corporate'. Just mention that you need access to the power switch http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:21: the IP controlled Power Switch, outlet #4, located in the remove name of the lab http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:48: self.assertTrue( replace self.assertTrue with 'assert' Use self.assertTrue only to verify test assertions http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:118: nit: need another blank line here
http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/6001/chrome/test/functional/chrom... chrome/test/functional/chromeos_battery.py:77: retry_sleep=2) Okay, in that case it is fine. On 2011/05/31 15:40:21, scottc wrote: > On 2011/05/27 23:33:43, Dave Tu wrote: > > Probably want to shorten the timeout and keep the default retry_sleep (which > is > > 0.25s). 60 seconds is a long time for a test to hang. > > During testing, I've seen it sometimes take up to 40 seconds for the > time_to_full and time_to_empty to calculate. Is there a stated requirement for > the longest it can take? If so, we should use that as the timeout. If not, > timeout should be high enough to eliminate false failures. > > I see that power_BatteryCharge.py uses 60 seconds for the timeout > (http://git.chromium.org/gitweb/?p=chromiumos/third_party/autotest.git;a=blob;...). http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:7: import os Remove extra import. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:33: if 'line_power_on': Put this in the configuration file and read it from there. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:35: else: nit: Indent these lines by one space. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:36: 'battery_time_to_empty': int (seconds) Does this configuration file exist yet? Please start an internal code review for that file if not.
Implemented all your comments. Had to re-write how code determined that the time-to-full/empty was present, so a thorough review is requested. Note: Will send review for changes to the config file in a separate CL. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:7: import time On 2011/06/01 01:09:23, Dave Tu wrote: > Remove extra import. Done. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:15: """Tests ChromeOS Battery Status API. On 2011/06/01 00:33:00, Nirnimesh wrote: > nit: leave a blank line after this line > > Remove 'API' in this sentence Done. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:17: 1) Device under test (DUT) is connected to the corporate LAN On 2011/06/01 00:33:00, Nirnimesh wrote: > remove reference to 'corporate'. Just mention that you need access to the power > switch Done. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:21: the IP controlled Power Switch, outlet #4, located in the On 2011/06/01 00:33:00, Nirnimesh wrote: > remove name of the lab Done. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:33: _OUTLET_WITH_BATTERY = '.a4' On 2011/06/01 01:09:23, Dave Tu wrote: > Put this in the configuration file and read it from there. Done. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:35: 'pyauto_private', 'chromeos', 'power', On 2011/06/01 01:09:23, Dave Tu wrote: > nit: Indent these lines by one space. Done. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:36: 'battery_testbed_config') On 2011/06/01 01:09:23, Dave Tu wrote: > Does this configuration file exist yet? Please start an internal code review for > that file if not. Yes file exists. It was sent out as issue https://chromereviews.googleplex.com/2897016/. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:48: self.assertTrue( On 2011/06/01 00:33:00, Nirnimesh wrote: > replace self.assertTrue with 'assert' > > Use self.assertTrue only to verify test assertions Done. http://codereview.chromium.org/6990069/diff/12001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:118: On 2011/06/01 00:33:00, Nirnimesh wrote: > nit: need another blank line here Done.
LGTM with mostly minor nits. Use the 'Commit:' checkbox to commit when ready. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:46: assert os.path.exists(ChromeosBattery._BATTERY_CONFIG_FILE),\ nit: put a space before \ Repeat elsewhere in this script http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:57: return ((battery_status.get('line_power_on') == ac_power_on) and nit: remove extra parens. You don't need any parens other than the outermost one http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:58: (battery_status.get(time_key) != None)) '!= None' is redundant http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:66: True, 'battery_time_to_full'), timeout=60, retry_sleep=1),\ Isn't the default timeout (~25 secs) not enough?
One fix looks required. The other is just a nit. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:88: msg='Line power is off.') Do you mean line power is on? http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:114: # Compare times Maybe explain why you are comparing the times. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:118: 'If test failed falsely, Kris owes Scott a beer. ') Nice!
http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:58: (battery_status.get(time_key) != None)) On 2011/06/06 18:09:01, Nirnimesh wrote: > '!= None' is redundant No, it should be "time_key in battery_status". 0 is a valid value, and you want to return True if it is 0. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:66: True, 'battery_time_to_full'), timeout=60, retry_sleep=1),\ You can pull this entire WaitUntil thing out to a helper function. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:66: True, 'battery_time_to_full'), timeout=60, retry_sleep=1),\ On 2011/06/06 18:09:01, Nirnimesh wrote: > Isn't the default timeout (~25 secs) not enough? scunningham said he's seen it take up to 40s, so 60s is a safe value. There is no specification as to how long it should take. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:118: 'If test failed falsely, Kris owes Scott a beer. ') Extra space after the last period.
Fixed! http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... File chrome/test/functional/chromeos_battery.py (right): http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:46: assert os.path.exists(ChromeosBattery._BATTERY_CONFIG_FILE),\ On 2011/06/06 18:09:01, Nirnimesh wrote: > nit: put a space before \ > Repeat elsewhere in this script Done. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:57: return ((battery_status.get('line_power_on') == ac_power_on) and On 2011/06/06 18:09:01, Nirnimesh wrote: > nit: remove extra parens. You don't need any parens other than the outermost one Done. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:58: (battery_status.get(time_key) != None)) On 2011/06/06 21:06:58, Dave Tu wrote: > On 2011/06/06 18:09:01, Nirnimesh wrote: > > '!= None' is redundant > > No, it should be "time_key in battery_status". 0 is a valid value, and you want > to return True if it is 0. replaced with "time_key in battery_status" http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:58: (battery_status.get(time_key) != None)) On 2011/06/06 18:09:01, Nirnimesh wrote: > '!= None' is redundant replaced with "time_key in battery_status" http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:66: True, 'battery_time_to_full'), timeout=60, retry_sleep=1),\ On 2011/06/06 21:06:58, Dave Tu wrote: > You can pull this entire WaitUntil thing out to a helper function. Okay, but only if you don't complain about me creating a function with a single line! :-) http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:66: True, 'battery_time_to_full'), timeout=60, retry_sleep=1),\ On 2011/06/06 21:06:58, Dave Tu wrote: > On 2011/06/06 18:09:01, Nirnimesh wrote: > > Isn't the default timeout (~25 secs) not enough? > > scunningham said he's seen it take up to 40s, so 60s is a safe value. There is > no specification as to how long it should take. Done. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:66: True, 'battery_time_to_full'), timeout=60, retry_sleep=1),\ On 2011/06/06 18:09:01, Nirnimesh wrote: > Isn't the default timeout (~25 secs) not enough? Nope. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:88: msg='Line power is off.') On 2011/06/06 20:54:50, stanleyw wrote: > Do you mean line power is on? Good catch! http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:118: 'If test failed falsely, Kris owes Scott a beer. ') On 2011/06/06 20:54:50, stanleyw wrote: > Nice! Done. http://codereview.chromium.org/6990069/diff/16001/chrome/test/functional/chro... chrome/test/functional/chromeos_battery.py:118: 'If test failed falsely, Kris owes Scott a beer. ') On 2011/06/06 21:06:58, Dave Tu wrote: > Extra space after the last period. Done.
Presubmit check for 6990069-21001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit Warnings ** License must match: .*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.\n Found a bad license header in these files: chrome/test/functional/chromeos_battery.py Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
Presubmit check for 6990069-21001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** License must match: .*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.\n Found a bad license header in these files: chrome/test/functional/chromeos_battery.py Presubmit checks took 1.1s to calculate.
Change committed as 88633 |
