Project

General

Profile

Actions

action #116554

closed

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

Added by okurz over 2 years ago. Updated about 2 years ago.

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

0%

Estimated time:

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 2 (1 open1 closed)

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

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

Actions
Actions #1

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
Actions #2

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).

Actions #3

Updated by mkittler over 2 years ago

  • Description updated (diff)
Actions #4

Updated by mkittler over 2 years ago

  • Description updated (diff)
Actions #5

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
Actions #6

Updated by mkittler over 2 years ago

  • Assignee set to mkittler
Actions #7

Updated by mkittler over 2 years ago

  • Status changed from Workable to In Progress
Actions #8

Updated by okurz over 2 years ago

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

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

Actions #10

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.

Actions #11

Updated by mkittler over 2 years ago

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

Actions #12

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.

Actions

Also available in: Atom PDF