Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump $count in calls to send_key_until_needlematch #15404

Merged
merged 1 commit into from
Aug 30, 2022
Merged

Bump $count in calls to send_key_until_needlematch #15404

merged 1 commit into from
Aug 30, 2022

Conversation

ilmanzo
Copy link
Contributor

@ilmanzo ilmanzo commented Aug 23, 2022 •
edited
Loading

The function send_key_until_needlematch() had an off-by-one error.
This error has been recently fixed, so we need to increment +1 the $count argument (3rd positional) in all test suites calling it.

@github-actions
Copy link

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

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files

@ilmanzo ilmanzo changed the title Bump $count in calls to send_key_until_needlematch [WIP] Bump $count in calls to send_key_until_needlematch Aug 23, 2022
@ge0r ge0r added the qe-yast label Aug 23, 2022
@ilmanzo ilmanzo mentioned this pull request Aug 24, 2022
Merged
@ilmanzo ilmanzo added the WIP Work in progress label Aug 25, 2022
@ilmanzo ilmanzo force-pushed the poo115619_Adjust_counter_argument_in_testsuites_to_match_recent_changes branch from 32d1977 to b7a04e2 Compare August 25, 2022 07:30
@ilmanzo ilmanzo changed the title [WIP] Bump $count in calls to send_key_until_needlematch Bump $count in calls to send_key_until_needlematch Aug 26, 2022
@ilmanzo ilmanzo force-pushed the poo115619_Adjust_counter_argument_in_testsuites_to_match_recent_changes branch from b7a04e2 to f107f2c Compare August 26, 2022 11:31
@ilmanzo ilmanzo removed the WIP Work in progress label Aug 26, 2022
@ilmanzo
Bump $count in calls to send_key_until_needlematch
5bda96c 
The function send_key_until_needlematch() had an off-by-one error.
This error has been fixed recently, so we need to increment +1 the
$count argument (3rd) in all test suite calling it.
@ilmanzo ilmanzo force-pushed the poo115619_Adjust_counter_argument_in_testsuites_to_match_recent_changes branch from f107f2c to 5bda96c Compare August 29, 2022 12:06
@Martchus
Copy link
Contributor

Have all of these calls really been made under the assumption that send_key_until_needlematch works like it did before os-autoinst/os-autoinst#2151? To me that PR seems to "fix" something in the sense that most people would have expected that behavior anyways. However, if that is not true, I'm wondering about the usefulness of os-autoinst/os-autoinst#2151.

@ilmanzo
Copy link
Contributor Author

ilmanzo commented Aug 30, 2022 •
edited
Loading

Have all of these calls really been made under the assumption that send_key_until_needlematch works like it did before os-autoinst/os-autoinst#2151? To me that PR seems to "fix" something in the sense that most people would have expected that behavior anyways. However, if that is not true, I'm wondering about the usefulness of os-autoinst/os-autoinst#2151.

I guess nobody can tell if ALL the calls are made with some expectation or the other. Most of them are likely to be "N-1 bug aware", where you can see some key are sent 9 times or 19 times example1, example2 when an EVEN number of keys are required.

Starting motivation of os-autoinst/os-autoinst#2151 and related POO ticket is to make the testapi function behave as user expects, e.g. you ask to send a key up to 3 times, it sends the key up to 3 times, and not 4. Once we got that fixed (and merged), we should update accordingly the calling code. What do you think?

@Martchus
Copy link
Contributor

Ok, and more a decision of test writers anyways.

@ge0r ge0r merged commit 9845ccf into os-autoinst:master Aug 30, 2022
@ilmanzo ilmanzo deleted the poo115619_Adjust_counter_argument_in_testsuites_to_match_recent_changes branch August 30, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jknphy jknphy jknphy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ilmanzo @Martchus @jknphy @ge0r