Project

General

Profile

action #116554

Make sleeping time in "no_wait" scenarios consistent size:M

Added by okurz 2 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Feature requests
Target version:
Start date:
2022-09-14
Due date:
% Done:

0%

Estimated time:
Difficulty:

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 f2a31c64

  • Review 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

Related issues

Related to openQA Tests - action #109737: [opensuse][sporadic] test fails in chromium due to lost characters when typing in the address bar size:MResolved2022-04-09

Related to openQA Project - action #116608: Support no_wait in send_key_until_needlematch as wellNew2022-09-15

History

#1 Updated by okurz 2 months ago

  • Related to action #109737: [opensuse][sporadic] test fails in chromium due to lost characters when typing in the address bar size:M added

#2 Updated by mkittler 2 months 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).

#3 Updated by mkittler 2 months ago

  • Description updated (diff)

#4 Updated by mkittler 2 months ago

  • Description updated (diff)

#5 Updated by okurz 2 months 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

#6 Updated by mkittler 2 months ago

  • Assignee set to mkittler

#7 Updated by mkittler 2 months ago

  • Status changed from Workable to In Progress

#8 Updated by okurz 2 months ago

  • Related to action #116608: Support no_wait in send_key_until_needlematch as well added

#9 Updated by openqa_review 2 months ago

  • Due date set to 2022-09-30

Setting due date based on mean cycle time of SUSE QE Tools

#10 Updated by mkittler 2 months ago

  • Status changed from In Progress to Feedback

I tested various scenarios from o3 locally. I suppose my changes ready to be merged.

#11 Updated by mkittler 2 months ago

The PR has been merged and deployed on OSD and most o3 workers.

#12 Updated by okurz about 2 months ago

  • Due date deleted (2022-09-30)
  • Status changed from Feedback to Resolved

That looks all quite good. IMHO we can resolve.

Also available in: Atom PDF