action #114412
closedAdd support for "wait_screen_change" with "no_wait" option to allow to use on cases like "wait for every character to be typed" size:M
Description
Motivation¶
As proposed in #109737 there are cases where we could use "wait_screen_change" but by default it waits for 0.5s which is wasteful if done too often in a row, e.g. in cases like "wait for every character to be typed". So same as in "check_screen/assert_screen" we should add support for the "no_wait" option or rethink how much time needs to be waited at all if not continuously checking.
Acceptance criteria¶
- AC1: "wait_screen_change" on a repeatedly updated screen waits significantly less than the current 0.5s
Suggestions¶
- Continue with https://github.com/os-autoinst/os-autoinst/pull/2109
- Optional: Make sure it works with https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/15173
Updated by okurz over 2 years ago
- Copied from action #109737: [opensuse][sporadic] test fails in chromium due to lost characters when typing in the address bar size:M added
Updated by livdywan over 2 years ago
- Subject changed from Add support for "wait_screen_change" with "no_wait" option to allow to use on cases like "wait for every character to be typed" to Add support for "wait_screen_change" with "no_wait" option to allow to use on cases like "wait for every character to be typed" size:M
- Description updated (diff)
- Status changed from New to Workable
Updated by mkittler over 2 years ago
- Assignee set to mkittler
there are cases where we could use "wait_screen_change" but by default it waits for 0.5s which is wasteful if done too often in a row
Note that it only waits 0.5 s if there's no immediate change. Simply decreasing this sleep won't help unless we decrease the frequency the backend requests a screen updated and enqueues a new screenshot. So as a first step I need to determine how the backend behaves in that regard and possibly add a way to enforce an immediate update (similar to no_wait for screen checks).
Updated by mkittler over 2 years ago
It looks like the isotovideo normally aims for a 1 second update interval. (The function _calc_check_delta in isotovideo checks how much time has passed since the last update and sets the timeout for a 1 second interval.) In case a screen is to be asserted with no_wait option the timeout is reduced to 0.1 seconds. However, all of this is only the interval for calling the backend's check_asserted_screen function. That function will only request a screen update (via VNC) in certain cases, though.
The backend's interval for requesting VNC updates is actually only influenced indirectly via $bmwqemu::vars{SCREENSHOTINTERVAL} which is by default 0.5 seconds. (The update interval is actually half of it, so by default it is 0.25 seconds.)
So assuming defaults are in place, reducing the sleep in wait_screen_change will not make it faster because only when a new screenshot is enqueued the the last image is updated and that's used for this check.
Considering the code in do_capture of baseclass.pm it should be easy to add another variable (that could be set from autotest functions) to either change the screenshot (and possibly VNC update) interval dynamically or to enforce the next VNC update and screenshot immediately.
It would also be possible to decouple wait_screen_change from the screenshot creation and compare with the VNC framebuffer directly. I suppose then we'd also make sure that the VNC socket is drained (as this so far only seems to happen when capturing a screenshot or before an update request).
Updated by mkittler over 2 years ago
So far autotest needs to poll the backend in wait_screen_change (with that 0.5 seconds sleep). Maybe it would be nicer if the backend would provide a synchronous call instead. So the backend would simply postpone the reply to the call until the screen changes or a timeout is reached. That would eliminate this one sleep altogether. The call would also be able to lower the screenshot interval to waste less time.
Updated by mkittler over 2 years ago
- Status changed from Workable to In Progress
Updated by openqa_review over 2 years ago
- Due date set to 2022-09-16
Setting due date based on mean cycle time of SUSE QE Tools
Updated by mkittler over 2 years ago
This PR shows how removing the sleep-loop in the autotest completely would look like: https://github.com/os-autoinst/os-autoinst/pull/2158
Then I could go ahead and add an option to make the backend reduce the time before enqueuing the next screenshot to waste less time on wait_screen_change calls.
Before that, I'll first give the refactoring a verification run. I'll try the scenario https://openqa.opensuse.org/tests/latest?arch=x86_64&distri=opensuse&flavor=DVD-Updates&machine=64bit-2G&test=gnome&version=15.3 from the original ticket about chromium typing issues (#109737).
Updated by mkittler over 2 years ago
PR for actually implementing no_wait (based on the first PR): https://github.com/os-autoinst/os-autoinst/pull/2164
Updated by mkittler over 2 years ago
- Status changed from In Progress to Feedback
The change for moving the loop to the backend was merged last week and has been deployed on o3 since 2022-09-08 (e.g. 2022-09-08 10:52:57|install|os-autoinst|4.6.1662625276.a9cb3fd-lp154.1384.1|aarch64||devel_openQA|9bf1d9106e4e769ae20319bc456ef5657e11051f78e634c6b4f398b10f45e71f|). It was also deployed on OSD today. So far it doesn't look like the refactoring caused any problems.
The change for implmenting no_wait still needs to be merged. I did some performance testing today and it looks like it really makes things faster (see https://github.com/os-autoinst/os-autoinst/pull/2164#issuecomment-1243772718).
Updated by livdywan over 2 years ago
- Status changed from Feedback to Resolved
This ticket is done, and we briefly confirmed our expectations here meaning #109737 is unblocked now and can be verified for the improvements.