action #116554
closedMake sleeping time in "no_wait" scenarios consistent size:M
Description
Motivation¶
We have multiple places in os-autoinst where we consider a "no_wait" scenario but use different sleeping times, e.g. 0.01s in https://github.com/os-autoinst/os-autoinst/pull/2109/files vs. 0.1s in https://github.com/os-autoinst/os-autoinst/pull/2164/files#diff-ee84fe6541d1e2e971e9e0d4bf0187272df8fa4ac878d033b1c8c600325e9f6dR185 . We should better avoid magic numbers with inconsistent usage for the same
Acceptance criteria¶
- AC1: In all cases of check_screen/assert_screen/wait_screen_change/wait_still_screen only a fraction of a second is spent when the expected screen content is already showing up (either in general or only if "no_wait" is used)
- AC2: We only use one common definition of a sleep interval (or timeout)
Suggestions¶
Review the history when "no_wait" was introduced. "no_wait" was first introduced in
commit e2b9c835, Author: Oliver Kurz okurz@suse.de, Date: Thu Jan 26 14:59:26 2017 +0100 "Add optional 'no_wait' parameter for check/assert_screen". The "no_wait" in "wait_still_screen" with 0.01s was introduced in f2a31c64Review occurences from
$ git grep '\(sleep\|interval\).*\.0\?1'
related to sleeping between test API checks
backend/baseclass.pm: # note: Still keeping the interval at 0.1 s to avoid wasting too much CPU (corresponding to what check_screen/assert_screen
backend/baseclass.pm: my @additional_intervals = $wait_screen_change && $wait_screen_change->{no_wait} ? (0.1) : ();
…
testapi.pm: sleep($args{no_wait} ? 0.01 : 0.5);
there might be an additional 0.1s somewhere in isotovideo that mkittler mentioned. At best we can define the number with 0.01 consistently in a single place and use from there following up with what https://github.com/os-autoinst/os-autoinst/pull/2109/files#diff-3420c7e01f73caee7d97053af39990e3297daf42d06e0fc8848f78af1a5e009eR565 tried to do already.
- It might be the best solution to get rid of sleep calls completely if that is not required. Please check the impact on CPU usage when removing the sleep call
- Keep in mind that in one case we have an actual sleep call, in the other case it's an interval/timeout for something slightly different
Updated by okurz over 2 years ago
- Related to action #109737: [opensuse][sporadic] test fails in chromium due to lost characters when typing in the address bar size:M added
Updated by mkittler over 2 years ago
The ticket description makes it sound like my PR would be inconsistent with what we have in the codebase. However, https://github.com/os-autoinst/os-autoinst/pull/2109/files has never been merged yet. So I don't see that we have multiple places in os-autoinst with inconsistent intervals. Note that these intervals are also not used in any literal sleep
call; they're used as timeouts when waiting on sockets.
There is indeed another place where we specify a very similar interval: https://github.com/os-autoinst/os-autoinst/blob/f206b29cea0413102e98b887e71a0d45b5698192/isotovideo#L331
However, it is consistent. Maybe it would make sense to introduce a constant and use it in both places. Both timeouts are used for similar purposes (no_wait
handling) but likely we should still think twice whether it actually makes sense to keep them in-sync (as they are after all used in quite different places).
I also highly doubt that all instances of 0.01/0.1 intervals should generally/blindly be treated the same (as the grep suggestion suggests).
It would also be good if it was tested whether 0.01 or 0.1 is faster and how the CPU usage behaves. Please refrain from just blindly reducing/increasing intervals (e.g. in my last change I've did some performance testing: https://github.com/os-autoinst/os-autoinst/pull/2164#issuecomment-1243772718).
Updated by okurz over 2 years ago
- Subject changed from Make sleeping time in "no_wait" scenarios consistent to Make sleeping time in "no_wait" scenarios consistent size:M
- Description updated (diff)
- Status changed from New to Workable
Updated by mkittler over 2 years ago
- Status changed from Workable to In Progress
Updated by okurz over 2 years ago
- Related to action #116608: Support no_wait in send_key_until_needlematch as well added
Updated by openqa_review over 2 years ago
- Due date set to 2022-09-30
Setting due date based on mean cycle time of SUSE QE Tools
Updated by mkittler over 2 years ago
- Status changed from In Progress to Feedback
I tested various scenarios from o3 locally. I suppose my changes ready to be merged.
Updated by mkittler over 2 years ago
The PR has been merged and deployed on OSD and most o3 workers.
Updated by okurz about 2 years ago
- Due date deleted (
2022-09-30) - Status changed from Feedback to Resolved
That looks all quite good. IMHO we can resolve.