Fix some issues related with the test case power-management/lid_close_suspend_open (Bugfix) #1093
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1093 +/- ##
==========================================
- Coverage 43.38% 41.48% -1.91%
==========================================
Files 357 341 -16
Lines 38686 37754 -932
Branches 6561 6412 -149
==========================================
- Hits 16784 15661 -1123
- Misses 21238 21451 +213
+ Partials 664 642 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
eugene-yujinwu
changed the title
eugene-yujinwu
requested review from
weizhenwu,
seankingyang,
linjuiche,
clairlin53,
zongminl,
pieqq,
scotthu27 and
stanley31huang
and removed request for
weizhenwu
| (Please wait at least 30 seconds after resuming from sleep before running this test. In most cases, | ||
| systemd will hold off on reacting to lid events in a period of time after system startup or resume.) | ||
| 1. Press "Enter" to start the test. | ||
| 2. Close the lid (Please close the lid within 10 sec). | ||
| 3. Wait 5 seconds with the lid closed. |
There was a problem hiding this comment.
it's a bit confuse to me that the waiting time is not match in step 3 and the above notes.
There was a problem hiding this comment.
@stanley31huang Thanks for comment. They are different time, the time mentioned in the notes is the time should wait to run this test if system just resume from suspend. The time in step 3 is the time to close the lid. It's maybe really a bit confusing, I'm not sure how to make it a little more clear. Any suggestion are welcome.
There was a problem hiding this comment.
It seems to me that we could simply write a scripts to check if current time is too close to previous suspend process.
If previous suspend process within 30 seconds, Ask user to wait a period of time and then close the lid.
There was a problem hiding this comment.
I think this is good idea. But I don't know how to get the how long the last suspend time to now. Do you have any idea about that? Thanks!
There was a problem hiding this comment.
If the scripts will make sure the system is ready for suspend test, we need to modify the steps here.
|
@eugene-yujinwu please remove the latest white space character from the title for the validate_title check. |
eugene-yujinwu
changed the title
Done. Thanks! |
|
@eugene-yujinwu |
@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. |
|
Add the power-management/lid test back per Clair's comments. |
|
Reopen it. Thank @vincent! |
|
@eugene-yujinwu I am thinking about that the 30 seconds is the default value defined in the |
| if [ "$holdoff_timeout_sec" -ne 0 ]; then | ||
| while (journalctl --since "$holdoff_timeout_sec seconds ago" -b 0 -r | grep "suspend exit" >/dev/null 2>&1) | ||
| do | ||
| echo "The system just resume from suspend, please wait for $holdoff_timeout_sec seconds and continue the test" |
There was a problem hiding this comment.
The $holdoff_timeout_sec is never changed during the loop, it would be better to echo just once at beginning.
There was a problem hiding this comment.
@hanhsuan A new commit was pushed, please kindly help review it. Many thanks!
| do | ||
| echo "waiting... " | ||
| sleep 3 | ||
| done |
There was a problem hiding this comment.
instead of having a while loop to polling the journal logs, I would suggest we simply sleep within a period of time if needed, likes
previous_sleep_log=$(journalctl --output=short-unix --since "$holdoff_timeout_sec seconds ago" -b 0 -r | grep "suspend exit")
if [ "$holdoff_timeout_sec" -ne 0 and "$previous_sleep_log" -ne "" ]; then
# set the previous_sleep_time
previous_sleep_time=$(echo $previous_sleep_log | awk -F'.' '{ print $1 }')
# sleep a period of time
sleep_time=$(( $(date +"%s") - "$previous_sleep_time" ))
echo "sleep for $sleep_time seconds ..."
sleep "$sleep_time"
fi
echo "System is ready for suspend test"
There was a problem hiding this comment.
@stanley31huang a new commit was pushed per this advise. Just a little change. Please have a look when you have time. thanks a lot.
…_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
Co-authored-by: stanley31huang <stanley.huang@canonical.com>
eugene-yujinwu
force-pushed
the
add-notes-for-lid-close-test
branch
from
7edb633
to
ab7d95c
Compare
Fix some issues related with the test case power-management/lid_close_suspend_open (bugfix) (#1080)
Description
Resolved issues
Documentation
Tests