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

Issue 748540 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocked on:
issue 748475



Sign in to add a comment

factory: Clean unused, obsoleted and non-tests in py/test/pytests

Project Member Reported by hungte@chromium.org, Jul 25 2017

Issue description

Currently when you do "make doc", there will be few warnings:

WARNING:root:No test cases found in cros.factory.test.pytests.tablet_mode_ui
WARNING:root:No test cases found in cros.factory.test.pytests.wifi_throughput_host

wifi_throuput_host is a script to run on host side, and is not a real pytest. I think it should live in somewhere else, for example py/test/rf or py/tools, or move wifi_throuput to its own folder, and put wifi_throuput_host there - but I need RF experts to confirm how this file was deployed (kitching/akahuang/petershih?)

tablet_mode_ui is actually a shared UI template for few tablet tests (tablet_mode, tablet_rotation) to share. I think we should either move all tablet tests to a folder (tablet/) so test items can share files there, or move tablet_mode_ui to a new ui template in py/test/, for example py/test/tablet_mode_ui or merged into ui_templates.
 

Comment 1 by hungte@chromium.org, Jul 26 2017

Owner: petershih@chromium.org
Status: Assigned (was: Untriaged)
temporarily assign to petershih
In addition to non-test, I think maybe it's time to remove the unused/obsolete RF pytest. 

List of RF pytests that are active in core project:
- bluetooth_host:  station-based bluetooth scan/pairing test. 
- rf_graphyte:     conductive/radiated test using Graphyte.
- vswr:            VSWR test, need external instrument.
- wifi_throughput: station-based Wifi throughput test. Need external AP

List of RF pytests that are active in partner project:
- wireless_antenna
- wireless_radiotap

The list of pytests that I'm not sure if we will use in the future:
- cellular_switch_firmware
- probe_cellular_info
- lte_verify_config
- bluetooth: DUT-based bluetooth test. Need remote bluetooth device (e.g keyboard)

The list I think/guess obsolete:
- rf_radiated
- wifi_rf
- wifi_throughput_host

Comment 3 by hungte@chromium.org, Jul 26 2017

Summary: factory: Clean unused, deprecated and non-tests in py/test/pytests (was: Remove non-test py in py/test/pytests)

Comment 4 by hungte@chromium.org, Jul 26 2017

Summary: factory: Clean unused, obsoleted and non-tests in py/test/pytests (was: factory: Clean unused, deprecated and non-tests in py/test/pytests)

Comment 5 by hungte@chromium.org, Jul 26 2017

Blockedon: 748475
wireless_antenna and wireless_radiotap should be merged (issue:748475).

cellular_switch_firmware will need to be reviewed.

probe_cellular_info should be replaced by probe if possible, and 
lte_verify_config may be replaced by probe or verify_value,

and I think bluetooth is still needed (I've seen partners testing this with external BT mouse).

Agree rf_radiated is deprecated by rf_graphyte.
Which test has deprecated wifi_rf? Still rf_graphyte?

sensor_movement can be removed. It is designed for Lucid to test gyro, and we didn't have a gyro test at that time. Should be deprecated by gyroscope and compass test.

hwmon_write is for Lucid too, just for simplify shell script logic in line_check_item. Not sure if we still need this.

Comment 7 Deleted

Comment 8 by hungte@chromium.org, Jul 26 2017

line_check_item is largely simplified by exec_shell. I'd prefer to remove hwmon_write if possible.

Comment 9 by hungte@chromium.org, Jul 26 2017

p.s. does that mean we can also remove hwmon from Device API?
What about py/test/pytests/hwmon_probe? (That should be merged to probe as well I think)
dut.hwmon is used to read humidity/voltage/temperature sensors in Lucid, it's better to create components for each of them if needed, not using a generic hwmon component.

hwmon_probe is more similar to thermal_sensor test, we can implement a hwmon thermal source and use thermal_sensor instead.
> hwmon_probe is more similar to thermal_sensor test, we can implement a hwmon thermal source and use thermal_sensor instead.

Agree. Meanwhile I've checked current test lists on ToT and no one is using that, so I think we can remove it until it's really needed by any real projects.

> dut.hwmon is used to read humidity/voltage/temperature sensors in Lucid, it's better to create components for each of them if needed, not using a generic hwmon component.

Since hwmon is actually not a real 'hardware component", I think maybe a better way is to move hwmon to cros.factory.test.utils.hwmon_utils, (or device.hwmon_utils, just like sensor_utils) and let individual sensors include hwmon.
I'm wondering if rf_radiated can be replaced by rf_graphyte under all circumstances?
rf_radiated measures signals via a power meter, I think this cannot be done by the graphyte framework? If this is the case, should we deprecate rf_radiated?
Can we extend Graphyte to support measuring signals via power meter?
Re #5,

wifi_rf mainly check if DUT can connect to Wifi AP and measure the signal strength. It can be replaced by wifi_throughput.

Re #12,

The current radiated test with Graphyte framework also measure the power level of the signal by the power meter. In addition, we can support multiple brand of power meters (we support Anritsu ML2437a and Keysight N1914a now). rf_radiated pytest only supports Keysight N1914a power meter. So it's fine to deprecate rf_radiated pytest.
Re #14,

Thanks for the clarify. Then I guess we can deprecate rf_radiated, and use graphyte for future builds.
also please set -x simple_storage_stress_test.py
More recommendations:
 - remove ectool_i2c_dev_id.py (inactive test from all tot  test lists)
 - rename ec_lightbar. It should be named lightbar if we really want, but
   since no OEM projects will be using it and it's very simple, highly 
   depends on particular board, we should have it in board overlay if needed.
 - I wonder if we should rename als_fixture and light_sensor in same style.
   Maybe als and als_fixture, or light_sensor and light_sensor_calibration.
... and maybe we should rename *_battery to battery_*. i.e., change simple_battey to battery_simple, and sysfs_battery to battery_sysfs.
Some conclusions from above discussions.

Following pytests will be removed:
1. [#2,#14] Remove rf_radiated, wifi_rf, wifi_throughput_host
2. [#5] Merge wireless_antenna and wireless_radiotap
3. [#6] Remove sensor_movement
4. [#6,#7] Remove hwmon_write
5. [#11] Remove hwmon_probe
6. [#17] Remove ectool_i2c_dev_id.py

Following pytests might need to be renamed:
1. [#17] als_fixture
2. [#17] light_sensor
3. [#18] *_battery to battery_*

Misc actions:
1. [#16] set -x simple_storage_stress_test.py

Question 1:
Comments #9, #10, #11 are around hwmon, thermal_sensor, and Device API. Should we create another issue on these topics?

Question 2:
From #17, if ec_lightbar should be in board overlay, I think we should remove it from ToT now?
Q1. thermal sensor is already addressed in https://chromium-review.googlesource.com/#/c/586215/

but yes, you can create another CL for hwmon changes.

Q2. yes I think we should remove it from ToT.
#17 you can move that test to samus' board overlay factory-board, since that should be the only device using it with tot factory-board (link also has lightbar but its tests were incomplete so no factory-board).
Project Member

Comment 22 by bugdroid1@chromium.org, Jul 27 2017

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

commit f1d20dcd86cb029b7b40a5f9115621302542017f
Author: Shen-En Shih <petershih@chromium.org>
Date: Thu Jul 27 09:42:53 2017

simple_storage_stress_test: remove execution bit

Remove the execution bit of pytest 'simple_storage_stress_test'

BUG= chromium:748540 
TEST=make test

Change-Id: Id513d006babc9ce9129361f2751383b13088b7c9
Reviewed-on: https://chromium-review.googlesource.com/588527
Commit-Ready: Shen-En Shih <petershih@chromium.org>
Tested-by: Shen-En Shih <petershih@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/f1d20dcd86cb029b7b40a5f9115621302542017f/py/test/pytests/simple_storage_stress_test.py

Project Member

Comment 23 by bugdroid1@chromium.org, Jul 27 2017

Project Member

Comment 24 by bugdroid1@chromium.org, Jul 28 2017

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

commit 162424670930cfe03b5c3348c878b130647ab382
Author: Shen-En Shih <petershih@chromium.org>
Date: Fri Jul 28 10:26:42 2017

pytests: Remove deprecated pytests.

Remove following pytests:
* ec_lightbar
* ectool_i2c_dev_id
* hwmon_probe
* hwmon_write
* rf_radiated
* sensor_movement
* wifi_rf
* wifi_throughput_host

BUG= chromium:748540 
TEST=make test

Change-Id: I439d364623fda81d8b33710bcfdbf19b9c02e690
Reviewed-on: https://chromium-review.googlesource.com/588872
Commit-Ready: Shen-En Shih <petershih@chromium.org>
Tested-by: Shen-En Shih <petershih@chromium.org>
Reviewed-by: Chih-Yu Huang <akahuang@chromium.org>
Reviewed-by: Ting Shen <phoenixshen@chromium.org>

[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/ec_lightbar.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/ectool_i2c_dev_id.py
[modify] https://crrev.com/162424670930cfe03b5c3348c878b130647ab382/py/test/test_lists/generic_smt.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/wifi_rf.py
[modify] https://crrev.com/162424670930cfe03b5c3348c878b130647ab382/po/zh-CN.po
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/rf_radiated/factory_common.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/rf_radiated/wifi_radiated_config.sample.yaml
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/sensor_movement.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/rf_radiated/rf_radiated.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/hwmon_write.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/hwmon_probe.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/wifi_throughput_host.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/rf_radiated/__init__.py
[delete] https://crrev.com/ffad996c87ce61a95e05cedc8c266506707dc1b8/py/test/pytests/rf_radiated/lte_radiated_config.sample.yaml

The rest part for this issue is:

Following pytests might need to be renamed:
1. [#17] als_fixture
2. [#17] light_sensor
3. [#18] *_battery to battery_*

I'd like to know is there any rule of thumb about the naming of the pytests?
And, any idea/decision on how to rename these pytests?
> any rule of thumb about the naming of the pytests?
 No, but I think it would be easier to read when converted to label on test list.

> And, any idea/decision on how to rename these pytests?
 My two cents:

1. It's easier to find "related test" if they have same prefix, i.e., battery_* are better than *_battery.

2. ALS (Ambient Light Sensor) is one kind of light sensor, but currently on laptops we usually only have ALS. So "light sensor" or "ambient light sensor" both sound ok to me, since our test code is actually more generic for any kinds of light sensors.

So maybe light_sensor and light_sensor_calibration (or light_sensor_fixture). I personally feel light_sensor_calibration better since that tells us what it does instead of how (fixture) it does.
Project Member

Comment 27 by bugdroid1@chromium.org, Aug 7 2017

Project Member

Comment 28 by bugdroid1@chromium.org, Aug 7 2017

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

commit 265677522acefac24bc7d10d655e9eabfecf3e0e
Author: Shen-En Shih <petershih@chromium.org>
Date: Mon Aug 07 18:45:04 2017

pytests: Rename 'sysfs_battery' to 'battery_sysfs'

Rename the pytest from 'sysfs_battery' to 'battery_sysfs'.
This groups the battery-related test together when sorting
alphabetically.

BUG= chromium:748540 
TEST=make test

Change-Id: I76678f3bb237b1d188574e4981ab62753d6c4029
Reviewed-on: https://chromium-review.googlesource.com/602106
Commit-Ready: Shen-En Shih <petershih@chromium.org>
Tested-by: Shen-En Shih <petershih@chromium.org>
Reviewed-by: Shen-En Shih <petershih@chromium.org>

[modify] https://crrev.com/265677522acefac24bc7d10d655e9eabfecf3e0e/po/zh-CN.po
[modify] https://crrev.com/265677522acefac24bc7d10d655e9eabfecf3e0e/py/test/test_lists/generic_smt.py
[rename] https://crrev.com/265677522acefac24bc7d10d655e9eabfecf3e0e/py/test/pytests/battery_sysfs.py

Project Member

Comment 29 by bugdroid1@chromium.org, Aug 9 2017

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

commit 4908034c20e2178a9dcd3159fb411b496cd9bd01
Author: Shen-En Shih <petershih@chromium.org>
Date: Wed Aug 09 09:23:17 2017

pytests: Rename 'simple_battery' to 'battery_basic'

Rename the pytest from 'simple_battery' to 'battery_basic'.
This will group the battery-related tests together, when
sorting alphabetically.

BUG= chromium:748540 
TEST=make test

Change-Id: Idbe5b601be3999eb1e6c62ca660cecd2c5861794
Reviewed-on: https://chromium-review.googlesource.com/603036
Commit-Ready: Shen-En Shih <petershih@chromium.org>
Tested-by: Shen-En Shih <petershih@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/4908034c20e2178a9dcd3159fb411b496cd9bd01/po/zh-CN.po
[rename] https://crrev.com/4908034c20e2178a9dcd3159fb411b496cd9bd01/py/test/pytests/battery_basic.py

Project Member

Comment 30 by bugdroid1@chromium.org, Aug 14 2017

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

commit f9ddb06fbca6707883626b4824ebb772c73d690d
Author: Peter Shih <pihsun@chromium.org>
Date: Mon Aug 14 12:54:36 2017

pytests: Remove mlb_version.

The board version doesn't starts with build phase anymore in newer
platform, and the expected_version check can be replaced using probe
pytest. So we can safely remove this test.

BUG= chromium:748540 
TEST=make doc
TEST=make test

Change-Id: I100175564e4c61523a1c699ece1c2d4370675008
Reviewed-on: https://chromium-review.googlesource.com/612925
Commit-Ready: Pi-Hsun Shih <pihsun@chromium.org>
Tested-by: Pi-Hsun Shih <pihsun@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/f9ddb06fbca6707883626b4824ebb772c73d690d/po/zh-CN.po
[delete] https://crrev.com/7113bc0f71cb85fd9103da1b29a1dadcbe845ced/py/test/pytests/mlb_version.py
[delete] https://crrev.com/7113bc0f71cb85fd9103da1b29a1dadcbe845ced/py/test/pytests/mlb_version_unittest.py

add one more - verify_touch_device_fw.py should be replaced by probe as well, I think.
Do we have anything left?
If not, Shen-en, feel free to close the issue.
Status: Fixed (was: Assigned)
#31,

verify_touch_device_fw.py should be replaced by the new probe framework as well. This issue can be tracked in this CL: https://bugs.chromium.org/p/chromium/issues/detail?id=677867#c11


#32,
Yes, all mentioned cleanups are done now.
Project Member

Comment 34 by bugdroid1@chromium.org, Oct 16 2017

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

commit 370e7ac4f0d1bb7bee8b4412c6ce15bc2e1fcf1f
Author: Peter Shih <pihsun@chromium.org>
Date: Mon Oct 16 04:55:55 2017

pytests: Merge display_idle with display.

The display_idle pytest is not used by anyone, and the code are most
duplicated from display pytest. To avoid double effort to maintain these
two tests, we can delete the display_idle, and add an argument for
display pytest to just show an image for given duration and pass.

BUG= chromium:748540 
TEST=make test

Change-Id: Ie9ca60068affaace5c91f1c034dc894f4527288e
Reviewed-on: https://chromium-review.googlesource.com/717997
Commit-Ready: Pi-Hsun Shih <pihsun@chromium.org>
Tested-by: Pi-Hsun Shih <pihsun@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>

[modify] https://crrev.com/370e7ac4f0d1bb7bee8b4412c6ce15bc2e1fcf1f/po/zh-CN.po
[delete] https://crrev.com/32d3316afb52b751db1733ee5857da406d01da9d/py/test/pytests/display_idle_static/display_idle.js
[modify] https://crrev.com/370e7ac4f0d1bb7bee8b4412c6ce15bc2e1fcf1f/py/test/pytests/display.py
[delete] https://crrev.com/32d3316afb52b751db1733ee5857da406d01da9d/py/test/pytests/display_idle_static/display_idle.css
[delete] https://crrev.com/32d3316afb52b751db1733ee5857da406d01da9d/py/test/pytests/display_idle.py
[modify] https://crrev.com/370e7ac4f0d1bb7bee8b4412c6ce15bc2e1fcf1f/py/test/pytests/display_static/display.js

Comment 35 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 36 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment