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.
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
Improve handling bsc#1008493 in yast2_tftp module #4823
Conversation
tests/console/yast2_tftp.pm
Outdated
if (match_has_tag('yast2_tftp_view_log_error')) { | ||
# softfail for opensuse when error for view log throws out | ||
record_soft_failure "bsc#1008493"; | ||
send_key 'alt-o'; # confirm the error message | ||
wait_screen_change { send_key 'alt-o' }; # confirm the error message | ||
assert_screen('yast2_tftp_view_log_show'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original intention was to solve it by a cool needle only, not any code change. A wait_screen_change before an assert_screen does not look something that we want to have in general. Why do you think this will help here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually thought that in this case code executed in this order :
wait_screen_change
send_key
assert_screen
@okurz ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okurz I can tell you what is the problem if we don't do so. I checked your theory, but it won't help as you've suggested, it's not possible to create a needle which cannot match before the error pop-up. So either we need wait_still_screen call before in old code, or like it is here. I was not able to reproduce an issue in 5 runs, and also doesn't happen too much on production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asmorodskyi no, you got it wrong. wait_screen_change
ensures to wait for a screen change after the body was executed. I crosschecked the docs in http://open.qa/api/testapi/#_wait_screen_change and it is not exactly clear from that. I will propose a doc change -> os-autoinst/os-autoinst#943
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwx788 Well, I would understand a wait_stil_screen
better. Could we do that instead? And please put an explanatory comment in the code because I understand now the test+behaviour but it is not intuitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okurz fine.
LGTM |
Once in a while we may match log needle, if error is not shown immediately. So, move soft-failure further, so we actually try to close dialog, and if it didn't work, we check if error is shown. Be aware that this issue doesn't appear often, and wait_still_screen would also do the trick, but we don't to wait for no reason. See [poo#34072](https://progress.opensuse.org/issues/34072).
Once in a while we may match log needle, if error is not shown
immediately. So, move soft-failure further, so we actually try to close
dialog, and if it didn't work, we check if error is shown.
Be aware that this issue doesn't appear often, and wait_still_screen
would also do the trick, but we don't to wait for no reason.
See poo#34072.