New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 609045 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug-Regression



Sign in to add a comment

Refactor code for power / battery in DUT testing

Project Member Reported by kitching@google.com, May 4 2016

Issue description

Encountered a bug made evident by this CL:

https://chromium-review.googlesource.com/#/c/339543

The power module has a class variable called self._battery_path, which is only ever initialized when CheckBatteryPresent() is called.  In some cases (such as when accessing the battery property in status module), self._battery_path would be accessed before being set, returning None as the path.  This would then cause an exception in the _dut.path.join() function in power's GetBatteryAttribute method.


Additionally:
* discovered some code from the status module should be moved over to power
* wondered what we are even using the dict returned by the battery property in status module for
* discovered another bug when using SSHLink, where SCP always results in a broken pipe for transferring files from /sys


Creating a bug to keep track of these related changes.
 
Cc: hungte@chromium.org phoenixshen@chromium.org
Components: Factory
Labels: -Type-Bug Type-Bug-Regression
Owner: kitching@chromium.org
Status: Started (was: Unconfirmed)
CL:342175 is causing status_unittest.py to start failing (as it should) because of some problem with DUTProperty / LazyProperty.  I looked at it for a while but couldn't fix it -- maybe you could take a look, Hung-Te?

ERROR:'LazyProperty' object has no attribute 'GetChargeFull'
I think that's because you removed battery_sysfs_path.
Currently there are no references to battery_sysfs_path left...  I don't think that is the issue.

Actually, I wrote the wrong error here.  It should be:

'LazyProperty' object has no attribute 'GetInfoDict'


So it seems like when the Snapshot is being created, the DUTProperty battery is failing on the line:

result = self._dut.power.GetInfoDict()

Why would self._dut.power resolve to a LazyProperty instead of its actual property when being accessed?
Project Member

Comment 5 by bugdroid1@chromium.org, May 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/factory/+/96c41d40738aeee39bb32006277a50bc93c89b18

commit 96c41d40738aeee39bb32006277a50bc93c89b18
Author: Joel Kitching <kitching@google.com>
Date: Wed May 04 08:25:29 2016

power: fix bug using _battery_path without initialization

In some cases, self._battery_path was being used without
first being initialized (through CheckBatteryPresent).
Convert it to a @property, and ensure that it always
returns a proper value when available.

BUG= chromium:609045 
TEST=Manually on DUT

Change-Id: I45b6a5f068d0ba0cd78589a749dd698c2307f6cd
Reviewed-on: https://chromium-review.googlesource.com/342174
Commit-Ready: Joel Kitching <kitching@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/96c41d40738aeee39bb32006277a50bc93c89b18/py/test/dut/power.py

Project Member

Comment 6 by sheriffbot@chromium.org, Jun 2 2016

Labels: Hotlist-Google
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/factory/+/b6f3c961f624b8d2b96d41165fea2283216ccdaa

commit b6f3c961f624b8d2b96d41165fea2283216ccdaa
Author: Joel Kitching <kitching@google.com>
Date: Wed May 04 08:28:13 2016

status/power: move power-related code into power module

Status module contained some sysfs/power-related code that
belongs in the power module.

Also update the ReadFile calls to use ReadSpecialFile.

BUG= chromium:609045 
TEST=Manually on DUT

Change-Id: I44c9fccc35f0c02e00eb153bf2fbebbdd5128227
Reviewed-on: https://chromium-review.googlesource.com/342175
Commit-Ready: Joel Kitching <kitching@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/b6f3c961f624b8d2b96d41165fea2283216ccdaa/py/test/dut/power.py
[modify] https://crrev.com/b6f3c961f624b8d2b96d41165fea2283216ccdaa/py/test/dut/status.py

Looks like the only use of status.battery is:

- goofy/js/goofy.js:
  - status.battery.fraction_full (expects a fraction [0, 1])
  - status.battery.status (expects one of 'Full', 'Charging', 'Discharging', 'Idle')


Uses of GetInfoDict():

- device/status.py (returns from `battery` property, and adds the keys 'status' and 'force' )
- device/status_unittest.py (tests status.py)
- test/pytests/battery_cycle.py (just for logging purposes)
Doesn't look like it is ever possible for status.battery.status to contain 'Idle'.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/6196935730d703d02ddf10f1a0c2c41fd9a605ac

commit 6196935730d703d02ddf10f1a0c2c41fd9a605ac
Author: Hung-Te Lin <hungte@chromium.org>
Date: Tue Jul 26 06:24:09 2016

chromeos-factory-deps: Add enum34 into dependency.

The 'enum34' (Enum from Python 3.4) is going to be used in factory
toolkit and should be included.

BUG= chromium:609045 , chromium:631342 
TEST=emerge-link chromeos-factory-deps

Change-Id: I1d7a6e58b5f3d740eaf3acd0e84e19a0d1f25a6a
Reviewed-on: https://chromium-review.googlesource.com/363221
Commit-Ready: Hung-Te Lin <hungte@chromium.org>
Tested-by: Hung-Te Lin <hungte@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/6196935730d703d02ddf10f1a0c2c41fd9a605ac/virtual/target-chromium-os-sdk/target-chromium-os-sdk-1.ebuild
[rename] https://crrev.com/6196935730d703d02ddf10f1a0c2c41fd9a605ac/chromeos-base/chromeos-factory-deps/chromeos-factory-deps-1-r20.ebuild
[modify] https://crrev.com/6196935730d703d02ddf10f1a0c2c41fd9a605ac/chromeos-base/chromeos-factory-deps/chromeos-factory-deps-1.ebuild

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/factory/+/2d22dcb1cc65391b39083d57eaa9b2e86758453f

commit 2d22dcb1cc65391b39083d57eaa9b2e86758453f
Author: Joel Kitching <kitching@google.com>
Date: Wed Jul 27 23:20:49 2016

external: add symlink for enum34 library

Also add an entry to deps.conf.

Enum34 library can now be used as follows:

  from cros.factory.external import enum
  class MyColours(enum.Enum):
    RED = 'red'
    BLUE = 'blue
    GREEN = 'green

BUG= chromium:609045 , chromium:631342 
TEST=Manually checking import
CQ-DEPEND=CL:363221

Change-Id: Ibd975251b68349b8e6a4cde50a52b7d900609890
Reviewed-on: https://chromium-review.googlesource.com/364030
Commit-Ready: Joel Kitching <kitching@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/2d22dcb1cc65391b39083d57eaa9b2e86758453f/py/tools/deps.conf
[add] https://crrev.com/2d22dcb1cc65391b39083d57eaa9b2e86758453f/py/external/enum.py

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/factory/+/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191

commit 82ae7b1adaa0246ca9e2ee566c9f477aa07cd191
Author: Joel Kitching <kitching@google.com>
Date: Tue Jul 26 00:42:01 2016

power/battery: refactor status.battery and power module

Remove extraneous information from status.battery, since it
is only used by goofy.js.

Update battery_cycle pytest to point at power module instead
of status.battery.

Introduce power.GetChargeState, and update charge states enum
to use enum34 key-value style to allow for simple mapping to
human-readable string.

BUG= chromium:609045 
TEST=None
CQ-DEPEND=CL:363221,CL:363242

Change-Id: Iae3d21d519884fe31d7369be7fe0f3e25f5afae0
Reviewed-on: https://chromium-review.googlesource.com/363035
Commit-Ready: Joel Kitching <kitching@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Ting Shen <phoenixshen@chromium.org>

[modify] https://crrev.com/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191/py/device/power.py
[modify] https://crrev.com/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191/py/test/pytests/battery_cycle.py
[modify] https://crrev.com/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191/py/goofy/js/goofy.js
[modify] https://crrev.com/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191/py/device/status.py

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/factory/+/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191

commit 82ae7b1adaa0246ca9e2ee566c9f477aa07cd191
Author: Joel Kitching <kitching@google.com>
Date: Tue Jul 26 00:42:01 2016

power/battery: refactor status.battery and power module

Remove extraneous information from status.battery, since it
is only used by goofy.js.

Update battery_cycle pytest to point at power module instead
of status.battery.

Introduce power.GetChargeState, and update charge states enum
to use enum34 key-value style to allow for simple mapping to
human-readable string.

BUG= chromium:609045 
TEST=None
CQ-DEPEND=CL:363221,CL:363242

Change-Id: Iae3d21d519884fe31d7369be7fe0f3e25f5afae0
Reviewed-on: https://chromium-review.googlesource.com/363035
Commit-Ready: Joel Kitching <kitching@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Ting Shen <phoenixshen@chromium.org>

[modify] https://crrev.com/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191/py/device/power.py
[modify] https://crrev.com/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191/py/test/pytests/battery_cycle.py
[modify] https://crrev.com/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191/py/goofy/js/goofy.js
[modify] https://crrev.com/82ae7b1adaa0246ca9e2ee566c9f477aa07cd191/py/device/status.py

Status: Fixed (was: Started)
Owner: ----
Status: Available (was: Fixed)
Re-opening since Hung-Te would like to eventually remove the status.battery dict.  (Not certain what the optimal refactoring would be.)
No, no need to re-opene now. I'm happy with how it works currently.

I think we'll remove it by other refactoring works, for example during Earl's re-design of Goofy.
Owner: kitching@chromium.org
Status: Verified (was: Available)
Okay.  We'll consider this refactor 'fixed'.
Project Member

Comment 18 by bugdroid1@chromium.org, Aug 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/factory/+/3e8427f4df2c3b794deab61138d06cb7e2bab225

commit 3e8427f4df2c3b794deab61138d06cb7e2bab225
Author: Joel Kitching <kitching@google.com>
Date: Tue Aug 09 05:53:01 2016

power/battery: tweaks to fix unittests

Fix unittest issues as described by hungte@, in a
comment on CL:363035:

  py/device/status_unittest.py:

    dut.battery is not dict (because the power.* calls
    raised exception, due to _battery_path is None)

  py/test/utils/charge_manager_unittest.py:

    missing def of CHARGE and other enums

BUG= chromium:609045 
TEST=Locally
CQ-DEPEND=CL:363035

Change-Id: I91a77dc34410a0d0431c1fae8c102aa75ba0e327
Reviewed-on: https://chromium-review.googlesource.com/366944
Commit-Ready: Joel Kitching <kitching@chromium.org>
Tested-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/3e8427f4df2c3b794deab61138d06cb7e2bab225/py/device/power.py
[modify] https://crrev.com/3e8427f4df2c3b794deab61138d06cb7e2bab225/py/test/utils/charge_manager_unittest.py
[modify] https://crrev.com/3e8427f4df2c3b794deab61138d06cb7e2bab225/py/device/status.py

Sign in to add a comment