New issue
Advanced search Search tips

Issue 698386 link

Starred by 1 user
Status: Started
Owner:
derat@chromium.org
Cc:
ka...@chromium.org
snanda@chromium.org
tbroch@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Make autotests use dump_power_status instead of power_supply_info

Project Member Reported by derat@chromium.org, Mar 3 2017 
power_supply_info generates indented, human-friendly output describing the status of the battery and external power supply, if any. There's a bunch of fragile code in the autotest repository that attempts to parse the output with varying amounts of rigor.

I added a dump_power_status tool a while back that produces similar output that's trivial for computers to parse. I'm going to make tests use it instead of power_supply_info.

As far as I can tell, the power_BatteryStateOnResume and power_ChargeStatus server tests aren't running anywhere. Can I just delete those?

Comment 1 by tbroch@chromium.org, Mar 4 2017

Labels: Restrict-View-Google
Similar findings here about these tests:  crbug.com/218129 

IMO, both power_BatteryStateOnResume & power_ChargeStatus test important things we should verify and probably did.  Looks like they may have been authored by test team I'd suspect in an effort to reduce manual testing.

In surveying https://testtracker.googleplex.com/efforts/search/ I wasn't able to find any reference to them currently however.

In the case of power_BatteryStateOnResume the absence of reliable RPMs is one reason it probably is not in a suite.  Not sure about power_ChargeStatus absence.

If we remove them (https://chromium-review.googlesource.com/c/448944/) is the plan to re-create their function elsewhere or do we think they're coverage is duplicated somewhere?  I can't find the duplication as of yet.

If not should we re-factor to use dump_power_status and add them as experimental?
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 4 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/5ade4272fa8cc7d4a2aa2442c5e8572329a36473

commit 5ade4272fa8cc7d4a2aa2442c5e8572329a36473
Author: Daniel Erat <derat@chromium.org>
Date: Sat Mar 04 10:59:07 2017

power: Make dump_power_status print display percentage.

Make dump_power_status print a "battery_display_percent"
field in addition to "battery_percent". The
platform_FullyChargedPowerStatus test needs the former.

BUG=chromium:698386
TEST=none

Change-Id: Idcdba437b38d2a11d354526cea7e5c8dae3b590e
Reviewed-on: https://chromium-review.googlesource.com/449958
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/5ade4272fa8cc7d4a2aa2442c5e8572329a36473/power_manager/tools/dump_power_status.cc

Comment 3 by derat@chromium.org, Mar 4 2017

Components: OS>Kernel>Power
powerd's code for reading the line power and battery state from sysfs is well-covered by unit tests. Were these autotests intended to catch bugs in firmware or the kernel that result in bad data being exposed through sysfs? If so, I'd prefer that we had tests that checked that data directly instead of running a powerd helper binary.

Comment 4 by tbroch@chromium.org, Mar 6 2017 

yes, I think they're meant to do end-to-end (E2E) / integration testing.

Isn't the powerd helper meant to exactly match the same API that powerd would use to expose same data to the user?  If so I'd think we'd want to keep using it to simplify alternatives to getting that data like crawling sysfs or asking EC as those attempts tend to be fragile.

Would it make more sense for test to ask powerd directly for the same info it provides to ash (power_supply_properties.proto?) and make sure that matches expectations?

Comment 5 by derat@chromium.org, Mar 6 2017 

Yes, that would make more sense, although I'm not sure how straightforward it'll be to decode protos from autotests.
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 8 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/482295d12adf1dd5c8204ed46752117ddb15995b

commit 482295d12adf1dd5c8204ed46752117ddb15995b
Author: Daniel Erat <derat@chromium.org>
Date: Wed Mar 08 00:18:22 2017

autotest: Remove old power server tests.

Remove power_BatteryStateOnResume and power_ChargeStatus,
both of which contain fragile code for parsing
power_supply_info's output and neither of which appear to be
run at present.

BUG=chromium:698386
TEST=none

Change-Id: I2d2f5fa0f142d28020844f453535811d51f6596f
Reviewed-on: https://chromium-review.googlesource.com/448944
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Todd Broch <tbroch@chromium.org>

[delete] https://crrev.com/541debad7b2f90f8add9b4ae69b1489ba6752a0d/server/site_tests/power_ChargeStatus/control
[delete] https://crrev.com/541debad7b2f90f8add9b4ae69b1489ba6752a0d/server/site_tests/power_BatteryStateOnResume/control
[delete] https://crrev.com/541debad7b2f90f8add9b4ae69b1489ba6752a0d/server/site_tests/power_ChargeStatus/power_ChargeStatus.py
[delete] https://crrev.com/541debad7b2f90f8add9b4ae69b1489ba6752a0d/server/site_tests/power_BatteryStateOnResume/power_BatteryStateOnResume.py

Comment 7 by derat@chromium.org, Mar 8 2017 

I'm looking at updating existing tests to use dump_power_status instead of power_supply_info, but I'm having trouble getting them to run even before I change them.

platform_FullyChargedPowerStatus's control file lists "rpm" and "power:battery" as dependencies. I don't see "rpm" in the labels list at http://cautotest/afe/#tab_id=hosts. When I tried to run the test on some random samus and lumpy DUTs in the lab, I got timeouts like this:

19:28:08 INFO | autoserv| self.host.power_on()
19:28:08 INFO | autoserv| File "/build/lumpy/usr/local/build/autotest/server/hosts/cros_host.py", line 1815, in power_on
19:28:08 INFO | autoserv| self._set_power('ON', power_method)
19:28:08 INFO | autoserv| File "/build/lumpy/usr/local/build/autotest/server/hosts/cros_host.py", line 1792, in _set_power
19:28:08 INFO | autoserv| hostname=self.hostname)
19:28:08 INFO | autoserv| File "/build/lumpy/usr/local/build/autotest/server/frontend.py", line 570, in set_host_attribute
19:28:08 INFO | autoserv| self.run('set_host_attribute', attribute=attr, value=val, **dargs)
19:28:08 INFO | autoserv| File "/build/lumpy/usr/local/build/autotest/server/cros/dynamic_suite/frontend_wrappers.py", line 111, in run
19:28:08 INFO | autoserv| self, call, **dargs)
19:28:08 INFO | autoserv| File "/usr/lib64/python2.7/site-packages/chromite/lib/retry_util.py", line 114, in GenericRetry
19:28:08 INFO | autoserv| time.sleep(sleep_time)
19:28:08 INFO | autoserv| File "/usr/lib64/python2.7/site-packages/chromite/lib/timeout_util.py", line 62, in kill_us
19:28:08 INFO | autoserv| raise TimeoutError(error_message % {'time': max_run_time})
19:28:08 INFO | autoserv| TimeoutError: Timeout occurred- waited 300 seconds.

PowerStatusStress (should that be platform_PowerStatusStress? confusingly, the name in the control file doesn't match the directory or .py name) depends on "servo" and "power:battery", but it got stuck when I tried to run it on chromeos2-row10-rack7-host13.cros:

19:49:10 INFO | autoserv| error: [Errno 110] Connection timed out
19:49:10 INFO | autoserv| Skipping this operation: pwr_button control is normal
19:49:10 INFO | autoserv| Skipping this operation: lid_open control is normal
19:49:10 INFO | autoserv| This repair action reported success: Start servod with the proper config settings.
19:49:10 INFO | autoserv| Skipping this operation: pwr_button control is normal
19:49:10 INFO | autoserv| Skipping this operation: lid_open control is normal
19:49:10 INFO | autoserv| Attempting this repair action: Wait for update, then reboot servo host.
(I Ctrl-C-ed here after a while)

Todd, can you offer any advice on how to run these?

Comment 8 by tbroch@chromium.org, Mar 9 2017 

> rpm stuff

RPMs are governed by this source:
  src/third_party/autotest/files/site_utils/rpm_control_system/rpm_client.py

Running the test on a host that has rpm functionality should generally pass and did for me a moment ago

test_that --fast chromeos2-row3-rack10-host21.cros platform_FullyChargedPowerStatus


-------------------------------------------------------------------------------------------------------------------------------------------
/tmp/test_that_results_6tdTLY/results-1-platform_FullyChargedPowerStatus                                                        [  PASSED  ]
/tmp/test_that_results_6tdTLY/results-1-platform_FullyChargedPowerStatus/platform_FullyChargedPowerStatus                       [  PASSED  ]
/tmp/test_that_results_6tdTLY/results-1-platform_FullyChargedPowerStatus/platform_FullyChargedPowerStatus/desktopui_SimpleLogin [  PASSED  ]
-------------------------------------------------------------------------------------------------------------------------------------------

I don't believe the rpm dep has any bearing.  Not sure about the power:battery dep checking w/o taking a closer look.

> PowerStatusStress

Agreed I think NAME= should match the sub-path (platform_PowerStatusStress in this case) + any control file suffixes.  Without it test_that certainly can't pick a singular test for execution:


test_that --fast chromeos2-row3-rack10-host21.cros PowerStatusStress
...
18:00:04 INFO | ... scheduled 3 job(s).


I see similar differences across other tests that have control.stress* files.  Should definitely follow-up w/ the test team w/ regard to any intent there.

Didn't get a chance to actually look at running that test to address your last question yet.

Comment 9 by derat@chromium.org, Mar 9 2017 

Thanks for the reply! I still don't understand what I'm doing wrong, though. I locked chromeos2-row3-rack9-host13 (which looked like it was one of six ready, unlocked samus devices in the lab) and ran "test_that --board=samus --fast chromeos2-row3-rack9-host13.cros platform_FullyChargedPowerStatus". test_that printed the following and is now hanging:

11:16:54 INFO | autoserv| START platform_FullyChargedPowerStatus        platform_FullyChargedPowerStatus timestamp=1489079814     localtime=Mar 09 11:16:54
11:16:54 INFO | autoserv| Starting master ssh connection '/usr/bin/ssh -a -x   -N -o ControlMaster=yes -o ControlPath=/tmp/_autotmp_KkRudLssh-master/socket -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o BatchMode=yes -o ConnectTimeout=30 -o ServerAliveInterval=900 -o ServerAliveCountMax=3 -o ConnectionAttempts=4 -o Protocol=2 -l root -p 22 chromeos2-row3-rack9-host13.cros'
11:21:56 INFO | autoserv| The test failed with the following exception
11:21:56 INFO | autoserv| Traceback (most recent call last):
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/client/common_lib/test.py", line 609, in _exec
11:21:56 INFO | autoserv| _call_test_function(self.execute, *p_args, **p_dargs)
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/client/common_lib/test.py", line 823, in _call_test_function
11:21:56 INFO | autoserv| raise error.UnhandledTestFail(e)
11:21:56 INFO | autoserv| UnhandledTestFail: Unhandled TimeoutError: Timeout occurred- waited 300 seconds.
11:21:56 INFO | autoserv| Traceback (most recent call last):
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/client/common_lib/test.py", line 817, in _call_test_function
11:21:56 INFO | autoserv| return func(*args, **dargs)
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/client/common_lib/test.py", line 470, in execute
11:21:56 INFO | autoserv| dargs)
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/client/common_lib/test.py", line 347, in _call_run_once_with_retry
11:21:56 INFO | autoserv| postprocess_profiled_run, args, dargs)
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/client/common_lib/test.py", line 380, in _call_run_once
11:21:56 INFO | autoserv| self.run_once(*args, **dargs)
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/server/site_tests/platform_FullyChargedPowerStatus/platform_FullyChargedPowerStatus.py", line 101, in run_once
11:21:56 INFO | autoserv| self.host.power_on()
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/server/hosts/cros_host.py", line 1845, in power_on
11:21:56 INFO | autoserv| self._set_power('ON', power_method)
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/server/hosts/cros_host.py", line 1822, in _set_power
11:21:56 INFO | autoserv| hostname=self.hostname)
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/server/frontend.py", line 570, in set_host_attribute
11:21:56 INFO | autoserv| self.run('set_host_attribute', attribute=attr, value=val, **dargs)
11:21:56 INFO | autoserv| File "/build/samus/usr/local/build/autotest/server/cros/dynamic_suite/frontend_wrappers.py", line 111, in run
11:21:56 INFO | autoserv| self, call, **dargs)
11:21:56 INFO | autoserv| File "/usr/lib64/python2.7/site-packages/chromite/lib/retry_util.py", line 114, in GenericRetry
11:21:56 INFO | autoserv| time.sleep(sleep_time)
11:21:56 INFO | autoserv| File "/usr/lib64/python2.7/site-packages/chromite/lib/timeout_util.py", line 62, in kill_us
11:21:56 INFO | autoserv| raise TimeoutError(error_message % {'time': max_run_time})
11:21:56 INFO | autoserv| TimeoutError: Timeout occurred- waited 300 seconds.

Comment 10 by derat@chromium.org, Mar 9 2017 

Maybe I don't understand how to find hosts that support RPM. Is https://sites.google.com/a/google.com/englab-dev/resources/infrastructure/chromeos/chromeos-infrastructure/other-devices/rpm the canonical source of documentation about this? It sounds like only certain racks and rows support RPM... I think? I'm out of my depth here. How do we usually run these tests?

Comment 11 by derat@chromium.org, Apr 27 2017

Cc: ka...@chromium.org
Labels: -Restrict-View-Google
[Removing RVG since there's nothing confidential here.]

Kalin, how useful is platform_PowerStatusStress? From looking at https://wmatrix.googleplex.com/unfiltered?days_back=30&suites=stress&tests=platform_PowerStatusStress, it appears to fail about as often as it succeeds. Does anyone investigate the failures? I would prefer to delete it if it doesn't produce actionable results.

I'm also unable to run it successfully:

$ test_that --board=samus chromeos1-row1-rack1-host2.cros PowerStatusStress
...
15:55:35 INFO | autoserv| Attempting this repair action: Start servod with the proper config settings.
15:55:36 INFO | autoserv| Board for DUT is unknown; starting servod assuming a pre-configured board.
15:55:56 INFO | autoserv| Verifying this condition: host is available via ssh
15:55:57 INFO | autoserv| Verifying this condition: servo BOARD setting is correct
15:55:57 INFO | autoserv| Verifying this condition: servo SERIAL setting is correct
15:55:57 INFO | autoserv| Verifying this condition: servod upstart job is running
15:55:58 INFO | autoserv| Verifying this condition: servod service is taking calls
15:56:13 INFO | autoserv| Failed: servod service is taking calls
15:56:13 INFO | autoserv| Traceback (most recent call last):
15:56:13 INFO | autoserv| File "/build/samus/usr/local/build/autotest/client/common_lib/hosts/repair.py", line 329, in _verify_host
15:56:13 INFO | autoserv| self.verify(host)
15:56:13 INFO | autoserv| File "/build/samus/usr/local/build/autotest/server/hosts/servo_repair.py", line 211, in verify
15:56:13 INFO | autoserv| host.connect_servo()
15:56:13 INFO | autoserv| File "/build/samus/usr/local/build/autotest/server/hosts/servo_host.py", line 135, in connect_servo
15:56:13 INFO | autoserv| timeout_sec=self.INITIALIZE_SERVO_TIMEOUT_SECS)
15:56:13 INFO | autoserv| File "/build/samus/usr/local/build/autotest/client/common_lib/cros/retry.py", line 132, in timeout
15:56:13 INFO | autoserv| default_result = func(*args, **kwargs)
15:56:13 INFO | autoserv| File "/build/samus/usr/local/build/autotest/server/cros/servo/servo.py", line 218, in initialize_dut
15:56:13 INFO | autoserv| self._server.hwinit()
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/xmlrpclib.py", line 1240, in __call__
15:56:13 INFO | autoserv| return self.__send(self.__name, args)
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/xmlrpclib.py", line 1599, in __request
15:56:13 INFO | autoserv| verbose=self.__verbose
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/xmlrpclib.py", line 1280, in request
15:56:13 INFO | autoserv| return self.single_request(host, handler, request_body, verbose)
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/xmlrpclib.py", line 1308, in single_request
15:56:13 INFO | autoserv| self.send_content(h, request_body)
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/xmlrpclib.py", line 1456, in send_content
15:56:13 INFO | autoserv| connection.endheaders(request_body)
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/httplib.py", line 1049, in endheaders
15:56:13 INFO | autoserv| self._send_output(message_body)
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/httplib.py", line 893, in _send_output
15:56:13 INFO | autoserv| self.send(msg)
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/httplib.py", line 855, in send
15:56:13 INFO | autoserv| self.connect()
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/httplib.py", line 832, in connect
15:56:13 INFO | autoserv| self.timeout, self.source_address)
15:56:13 INFO | autoserv| File "/usr/lib64/python2.7/socket.py", line 575, in create_connection
15:56:13 INFO | autoserv| raise err
15:56:13 INFO | autoserv| error: [Errno 110] Connection timed out

Comment 12 by tbroch@chromium.org, Apr 27 2017 

FWIW, my inclusion of RVG was based on reading/hearing something somewhere about not sharing internal URLs & hostnames publicly.
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/806c0253c4279bf5d0aa448503a813f758f8437d

commit 806c0253c4279bf5d0aa448503a813f758f8437d
Author: Daniel Erat <derat@chromium.org>
Date: Fri Apr 28 09:58:15 2017

autotest: Delete platform_FullyChargedPowerStatus.

This test was removed from the bvt-perbuild and stress
suites in April 2016.

BUG=chromium:698386
TEST=none

Change-Id: I70bd5d32cf1a3d48ed802687320720738097c9a7
Reviewed-on: https://chromium-review.googlesource.com/489445
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Kalin Stoyanov <kalin@chromium.org>

[delete] https://crrev.com/7b81a313d627bc9458f1a072e4259333cfba7337/server/site_tests/platform_FullyChargedPowerStatus/platform_FullyChargedPowerStatus.py
[delete] https://crrev.com/7b81a313d627bc9458f1a072e4259333cfba7337/server/site_tests/platform_FullyChargedPowerStatus/control
[delete] https://crrev.com/7b81a313d627bc9458f1a072e4259333cfba7337/server/site_tests/platform_FullyChargedPowerStatus/control.stress
Project Member

Comment 14 by bugdroid1@chromium.org, May 3 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f7e87754b3296be922083842c537be2e56a2e9f8

commit f7e87754b3296be922083842c537be2e56a2e9f8
Author: Daniel Erat <derat@chromium.org>
Date: Wed May 03 03:24:19 2017

autotest: Delete platform_PowerStatusStress.

This test appears to fail about as often as it passes,
contains fragile-seeming regular expressions for parsing
power_supply_info's output, and has allegedly never caught a
real issue.

BUG=chromium:698386
TEST=none

Change-Id: I7cbe7cc0dc8e96158a1f343b31b4fb3fdfd40986
Reviewed-on: https://chromium-review.googlesource.com/493939
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Kalin Stoyanov <kalin@chromium.org>

[delete] https://crrev.com/e5b5fa849448a63cf1b2ccd55df8b078037d65d8/server/site_tests/platform_PowerStatusStress/platform_PowerStatusStress.py
[delete] https://crrev.com/e5b5fa849448a63cf1b2ccd55df8b078037d65d8/server/site_tests/platform_PowerStatusStress/control
[delete] https://crrev.com/e5b5fa849448a63cf1b2ccd55df8b078037d65d8/server/site_tests/platform_PowerStatusStress/control.stress3
[delete] https://crrev.com/e5b5fa849448a63cf1b2ccd55df8b078037d65d8/server/site_tests/platform_PowerStatusStress/control.stress2
►

Sign in to add a comment