Fix nr. of keys sent in send_key_until_needlematch

[ilmanzo]

Related ticket: https://progress.opensuse.org/issues/107749

the function send_key_until_needlematch() is documented to send the key at maximum n times, but is sending the key n+1 times before failing. By looking at the test 03-testapi.t , 'esc' key is sent two times and then checked for three times.

This is an issue for some scenarios, for example, if an even number of keypresses is required, as is the case of maximizing and unmaximizing a window, by sending an even number of alt-f10.

This PR aims to correct both the unit test and the API behavior.

[github-actions]

Great PR! Please pay attention to the following items before merging:

Files matching testapi.pm:

• Consider bumping the version number in OpenQA/Isotovideo/Interface.pm for changes in behaviour

This is an automatically generated QA checklist based on modified files

[Martchus]

I suppose this change is in line with what is already documented so I guess it looks good to merge.

[codecov]

Codecov Report

Merging #2151 (1d51f69) into master (d8c6a21) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2151      +/-   ##
==========================================
- Coverage   90.25%   90.25%   -0.01%     
==========================================
  Files         153      153              
  Lines       14313    14310       -3     
==========================================
- Hits        12918    12915       -3     
  Misses       1395     1395              

Impacted Files	Coverage Δ
OpenQA/Isotovideo/Interface.pm	100.00% <ø> (ø)
t/03-testapi.t	100.00% <100.00%> (ø)
testapi.pm	71.73% <100.00%> (ø)
t/23-baseclass.t	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ilmanzo]

to avoid breaking existing test suites, we are going to fix all $count parameter in the callers ; I'd suggest to merge / release both changes in the same timeframe . Related PR: os-autoinst/os-autoinst-distri-opensuse#15404

[Martchus]

Ok, then I'm making it as not-ready for now to prevent the auto-merge.

[jknphy]

Hi @Martchus how we can sync the deployment in openQA of this change with the other PR for the use the function?
I'm thinking to merge it on Monday for example (not on Friday :) I guess) Should we ping tools squad for manual deployment? otherwise there could be many test failing during the gap, wdyt?

[Martchus]

Since packages need to be rebuilt and an out-of-schedule deployment needed to be triggered manually, there might be some time between merging a PR here and its actual deployment. Normally we solve this by avoiding breaking changes in the first place. I haven't thought of this change as one which would affect many tests as I assumed the new behavior is what people expected to happen anyways.

I suppose we could still do it but then the other way around. This change is deployed first and once you get the deployment e-mail on openqa AT suse… you can merge the test PR (which should be deployed rather quickly).

[jknphy]

een merging a PR here and its actual deployme

what deployment email? is there any slack channel?

[perlpunk]

Normally we could check $OpenQA::Isotovideo::Interface::version >= 28, right? But I guess that's not very nice for a change with over 200 lines

[perlpunk]

@jknphy there will be an email sent to the openqa mailing list after deployment.

[perlpunk]

I retriggered the Tumbleweed OBS test, there is already a ticket about the failing 27-consoles-vmware.t test

[b10n1k]

+1

[jknphy]

can this be merged without that OBS check? is it just about to remove the label, right? it is not "required", I don't see any merge button (but maybe I don't have permission in this repo, not sure or that is not visible with the automerge in place).
Please proceed if so :) better on Monday than at the end of the week.

[Martchus]

We could merge it without the OBS check. However, it would be cleaner if you'd rebase against master. (The problem with the test should hopefully already fixed by another PR that has been merged meanwhile.)

[ilmanzo]

Hello, seems approved and checks have passed. If we can we remove the label and merge, I'd ask to do the same on the parallel pull request . Thanks

[Martchus]

Yes, we can remove labels and merge it.

[ilmanzo]

So... I'd ask whoever has permission to remove the label and merge; many thanks

[ge0r]

So... I'd ask whoever has permission to remove the label and merge; many thanks

Since it was @Martchus that added the label, could you merge this please?