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.

Sign up for GitHub

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

Jump to bottom

Fix some issues related with the test case power-management/lid_close_suspend_open (Bugfix) #1093

Merged
merged 7 commits into from
Jul 4, 2024
Merged

Fix some issues related with the test case power-management/lid_close_suspend_open (Bugfix) #1093

merged 7 commits into from
Jul 4, 2024

Conversation

eugene-yujinwu
Copy link
Contributor

Fix some issues related with the test case power-management/lid_close_suspend_open (bugfix) (#1080)

  • Include a note to ensure the test is executed at the appropriate time
  • Fix the typo in the test order number
  • Remove functionally similar test cases power-management/lid from the test plan
  • Adjust the place of test case to avoid running it just after the suspend case

Description

Resolved issues

Documentation

Tests

@codecov Codecov
Copy link

codecov bot commented Mar 20, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.48%. Comparing base (8a9c908) to head (f1f03f9).
Report is 10 commits behind head on main.

❗ Current head f1f03f9 differs from pull request most recent head 90ccfd7

Please upload reports for the commit 90ccfd7 to get more accurate results.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1093      +/-   ##
==========================================
- Coverage   44.17%   41.48%   -2.70%     
==========================================
  Files         359      341      -18     
  Lines       38812    37754    -1058     
  Branches     6581     6412     -169     
==========================================
- Hits        17147    15661    -1486     
- Misses      21003    21451     +448     
+ Partials      662      642      -20
Flag Coverage Δ
provider-base 15.26% <ø> (-3.30%) ⬇️
provider-genio ?

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.

@eugene-yujinwu eugene-yujinwu changed the title Fix some issues related with the test case power-management/lid_close_suspend_open (bugfix) (#1080) Fix some issues related with the test case power-management/lid_close_suspend_open (Bugfix) Mar 20, 2024
@stanley31huang
Copy link
Collaborator

@eugene-yujinwu please remove the latest white space character from the title for the validate_title check.

@eugene-yujinwu eugene-yujinwu changed the title Fix some issues related with the test case power-management/lid_close_suspend_open (Bugfix) Fix some issues related with the test case power-management/lid_close_suspend_open (Bugfix) Mar 21, 2024
@eugene-yujinwu
Copy link
Contributor Author

@eugene-yujinwu please remove the latest white space character from the title for the validate_title check.

Done. Thanks!

@clairlin53
Copy link
Contributor

@eugene-yujinwu
I recommend keeping the second times with lid test, there were some issues observed with lid close after second times and there is an issue when closing the lid over 10 seconds, please refer to the following bugs for more details.
https://bugs.launchpad.net/stella/+bug/1974120
https://bugs.launchpad.net/stella/+bug/1979038
https://bugs.launchpad.net/stella/+bug/1962826

@eugene-yujinwu
Copy link
Contributor Author

eugene-yujinwu commented Mar 22, 2024 •
edited
Loading

@eugene-yujinwu I recommend keeping the second times with lid test, there were some issues observed with lid close after second times and there is an issue when closing the lid over 10 seconds, please refer to the following bugs for more details. https://bugs.launchpad.net/stella/+bug/1974120 https://bugs.launchpad.net/stella/+bug/1979038 https://bugs.launchpad.net/stella/+bug/1962826

@clairlin53 Thank you for your comment. I think it makes sense to keep this case, as shown by the bugs you mentioned , we have indeed encountered situations in the past where the first close lid worked fine, but the second close lid had a problem.
I will add it back. Thank you.

@eugene-yujinwu
Copy link
Contributor Author

Add the power-management/lid test back per Clair's comments.

@eugene-yujinwu
Copy link
Contributor Author

Reopen it. Thank @vincent!

@hanhsuan
Copy link
Contributor

@eugene-yujinwu I am thinking about that the 30 seconds is the default value defined in the logind.conf by HoldoffTimeoutSec. Is that possible to show the real value while this test case at beginning ? Then the user don't have to guess how long they have to wait for next test.

@eugene-yujinwu eugene-yujinwu marked this pull request as draft April 17, 2024 06:29
@stanley31huang
Copy link
Collaborator

@eugene-yujinwu please change the status of this PR to Ready for review when you think it's ready. And please make sure all unit tests passed with your changes.

@eugene-yujinwu
Fix some issues related with the test case power-management/lid_close…
f8d173e 
…_suspend_open (bugfix) (canonical#1080)

- Include a note to ensure the test is executed at the appropriate time
- Fix the typo in the test order number
- Remove functionally similar test cases power-management/lid from the test plan
- Adjust the place of test case to avoid running it just after the suspend case
@eugene-yujinwu
Add test power-management/lid back per Clair's comments
886464d
@eugene-yujinwu eugene-yujinwu marked this pull request as ready for review June 21, 2024 04:43
@eugene-yujinwu
Copy link
Contributor Author

@stanley31huang just rebase the commit and reopen the review. But I cannot figure out why some of the unit tests failed. Do you have any idea or hints? Thanks!

@eugene-yujinwu
Copy link
Contributor Author

@stanley31huang , finally I make the unit tests passed:). could you kindly review this, thanks!

pieqq
pieqq requested changes Jun 27, 2024
View reviewed changes
Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

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

Nice quality of life improvement on this job!

I feel like the Shell script is becoming really big and hard to maintain, and it is not tested... I think it would be worthwhile to convert it into a Python script with unit tests.

That said, I've made a few comments and added some suggestions.

providers/base/units/power-management/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/graphics/test-plan.pxu Show resolved Hide resolved
providers/base/bin/lid_close_suspend_open.sh Outdated Show resolved Hide resolved
providers/base/bin/lid_close_suspend_open.sh Outdated Show resolved Hide resolved
@eugene-yujinwu
Copy link
Contributor Author

@pieqq Thanks for review. A new commit was pushed per your suggestion. Could you kindly review it. Thanks!

@eugene-yujinwu
Copy link
Contributor Author

Nice quality of life improvement on this job!

I feel like the Shell script is becoming really big and hard to maintain, and it is not tested... I think it would be worthwhile to convert it into a Python script with unit tests.

That said, I've made a few comments and added some suggestions.

@pieqq Do you think I should drop this PR, and rewrite it with python and submit a new PR?

pieqq
pieqq approved these changes Jul 4, 2024
View reviewed changes
Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

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

+1, thanks!

@pieqq
Copy link
Collaborator

pieqq commented Jul 4, 2024

Nice quality of life improvement on this job!
I feel like the Shell script is becoming really big and hard to maintain, and it is not tested... I think it would be worthwhile to convert it into a Python script with unit tests.
That said, I've made a few comments and added some suggestions.

@pieqq Do you think I should drop this PR, and rewrite it with python and submit a new PR?

Let'd land this. You can keep a note to improve it later (switch to Python + add unit tests).

@pieqq pieqq merged commit 9409c50 into canonical:main Jul 4, 2024
9 checks passed
@eugene-yujinwu
Copy link
Contributor Author

Nice quality of life improvement on this job!
I feel like the Shell script is becoming really big and hard to maintain, and it is not tested... I think it would be worthwhile to convert it into a Python script with unit tests.
That said, I've made a few comments and added some suggestions.

@pieqq Do you think I should drop this PR, and rewrite it with python and submit a new PR?

Let'd land this. You can keep a note to improve it later (switch to Python + add unit tests).

Thank you @pieqq . OK, let's do it later.

@zongminl zongminl mentioned this pull request Aug 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stanley31huang stanley31huang stanley31huang left review comments

@hanhsuan hanhsuan hanhsuan left review comments

@pieqq pieqq pieqq approved these changes

@seankingyang seankingyang Awaiting requested review from seankingyang

@weizhenwu weizhenwu Awaiting requested review from weizhenwu

@linjuiche linjuiche Awaiting requested review from linjuiche

@clairlin53 clairlin53 Awaiting requested review from clairlin53

@zongminl zongminl Awaiting requested review from zongminl

@scotthu27 scotthu27 Awaiting requested review from scotthu27

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eugene-yujinwu @stanley31huang @clairlin53 @hanhsuan @pieqq