Issue metadata
Sign in to add a comment
|
Refactor code for power / battery in DUT testing |
||||||||||||||||||||||
Issue descriptionEncountered 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.
,
May 4 2016
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'
,
May 4 2016
I think that's because you removed battery_sysfs_path.
,
May 4 2016
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?
,
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
,
Jun 2 2016
,
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
,
Jul 25 2016
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)
,
Jul 26 2016
Doesn't look like it is ever possible for status.battery.status to contain 'Idle'.
,
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
,
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
,
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
,
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
,
Aug 3 2016
,
Aug 9 2016
Re-opening since Hung-Te would like to eventually remove the status.battery dict. (Not certain what the optimal refactoring would be.)
,
Aug 9 2016
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.
,
Aug 9 2016
Okay. We'll consider this refactor 'fixed'.
,
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 |
|||||||||||||||||||||||
Comment 1 by kitching@chromium.org
, May 4 2016Components: Factory
Labels: -Type-Bug Type-Bug-Regression
Owner: kitching@chromium.org
Status: Started (was: Unconfirmed)