Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LED manifest entries for interactive LED checks (BugFix) #1385

Merged
merged 5 commits into from
Aug 23, 2024

Conversation

pieqq
Copy link
Collaborator

@pieqq pieqq commented Aug 3, 2024

Description

In order to prevent tester from having to manually skip tests that check LEDs that might not be present on a device, this PR introduces manifest entries for all the LED checks that are currently used, so that the tester can select what LED indicators the DUT has:

image

Before this PR, there was only one generic has_led_indicator that originally came from the CE-OEM provider to test some IoT-specific LEDs. This manifest entry is renamed to a more specific has_led_gpio_sysfs and grouped with the other LED manifest entries using the same prompt.

Please check individual commit messages for more information.

Resolved issues

https://warthogs.atlassian.net/browse/CHECKBOX-1322

Documentation

Tests

Running a Desktop Cert test plan and selecting the LED tests, the new manifest entries are displayed and can be tailored to match the LEDs available in the DUT.

pieqq added 3 commits August 3, 2024 22:12
Instead of asking the tester to manually skip tests when a device does
not have a given LED (for instance no Microphone Mute LED), this commit
introduces manifest entries for all the related LED jobs.

This way, the tester defines what LEDs the device has before running the
tests, and tests that are not required due to a lack of specific LED are
automatically skipped.
The generic "has_led_indicator" manifest entry, which originally came
from the CE-OEM provider for IoT testing, is renamed as
"has_led_gpio_sysfs" to better reflect the need for IoT projects.

It is grouped with other LED-related manifest entries by using the same
prompt.
led/wlan, led/wlan-disabled and led/bluetooth are all testing a subset
of what led/wireless is testing. This commit

- removes these jobs
- replaces their calls with led/wireless in related test plans
Copy link

codecov bot commented Aug 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.14%. Comparing base (b3df727) to head (cea6439).
Report is 148 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1385      +/-   ##
==========================================
+ Coverage   45.12%   45.14%   +0.02%     
==========================================
  Files         366      367       +1     
  Lines       39058    39079      +21     
  Branches     6607     6611       +4     
==========================================
+ Hits        17626    17644      +18     
- Misses      20758    20760       +2     
- Partials      674      675       +1     
Flag Coverage Δ
provider-base 18.71% <ø> (+0.09%) ⬆️
provider-certification-client 57.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@rickwu666666 rickwu666666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This modification makes sense to IoT platforms. Thanks!

@hanhsuan
Copy link
Contributor

hanhsuan commented Aug 8, 2024

@GabrielChenCC Do you have any platform with separate suspend LED ?

@GabrielChenCC
Copy link
Contributor

@GabrielChenCC Do you have any platform with separate suspend LED ?

@hanhsuan For ThinkPad prototypes, they all have a separate suspend LED; for ThinkCentre and ThinkStation prototypes, they do not have suspend LED.

Copy link
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the new manifest entries, I have a doubt about the job removal, Are we sure that the wireless is a superset of removed bt+wifi tests? The comment seems to hint to the opposite actually.

providers/base/units/led/jobs.pxu Show resolved Hide resolved
providers/base/units/led/test-plan.pxu Show resolved Hide resolved
@pieqq
Copy link
Collaborator Author

pieqq commented Aug 22, 2024

@GabrielChenCC Do you have any platform with separate suspend LED ?

@hanhsuan For ThinkPad prototypes, they all have a separate suspend LED; for ThinkCentre and ThinkStation prototypes, they do not have suspend LED.

@hanhsuan @GabrielChenCC the led/suspend job is not part of any test plan right now. Are you suggesting I should add another manifest for suspend led and add the test to a test plan?

@GabrielChenCC
Copy link
Contributor

@GabrielChenCC Do you have any platform with separate suspend LED ?

@hanhsuan For ThinkPad prototypes, they all have a separate suspend LED; for ThinkCentre and ThinkStation prototypes, they do not have suspend LED.

@hanhsuan @GabrielChenCC the led/suspend job is not part of any test plan right now. Are you suggesting I should add another manifest for suspend led and add the test to a test plan?

@pieqq I think so, for the led/suspend, every ThinkPad has a suspend LED. It is indeed a test item. If we don't add a test case to verify it, it is very likely to be missed during testing. This gap should be remedied. So I guess we should add another manifest for the suspend led and add the test to a test plan.

@hanhsuan
Copy link
Contributor

@GabrielChenCC Do you have any platform with separate suspend LED ?

@hanhsuan For ThinkPad prototypes, they all have a separate suspend LED; for ThinkCentre and ThinkStation prototypes, they do not have suspend LED.

@hanhsuan @GabrielChenCC the led/suspend job is not part of any test plan right now. Are you suggesting I should add another manifest for suspend led and add the test to a test plan?

This submission shows led/suspend is included in the manual test plan and I skip it manually. led/suspendf is included in client-cert-desktop-24-04.pxu -> suspend-key-led-oops-check-cert

pieqq added 2 commits August 22, 2024 11:34
Based on feedback from QA team, add a manifest for the suspend LED and
use it in the led/suspend job.
@pieqq
Copy link
Collaborator Author

pieqq commented Aug 22, 2024

@hanhsuan @GabrielChenCC I've made the modification discussed in d5bbe5f.

I also found that the led/power-blink-suspend test required the device to have a Power LED, so I added that in cea6439.

Please review the change :)

@hanhsuan
Copy link
Contributor

LGTM

@GabrielChenCC
Copy link
Contributor

GabrielChenCC commented Aug 23, 2024

Look good to me. Thanks.

@Hook25 Hook25 merged commit d725523 into main Aug 23, 2024
45 checks passed
@Hook25 Hook25 deleted the 1322-led-manifest branch August 23, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants