-
Notifications
You must be signed in to change notification settings - Fork 266
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
Avoid sporadic failure clicking on reboot #2869
Avoid sporadic failure clicking on reboot #2869
Conversation
Verification run ok, can be reviewed / merged |
I'm not sure if this will help, problem is that for some reason pressing 'logoutdialog-reboot-highlighted' didn't work, after that is assert_screen, 30 seconds of waiting should be enough. |
Oh, right, I thought that wait_screen_change was making a loop with the callback and it should press the button until the box is closed. |
63375a5
to
b61926f
Compare
New changes and new verification run: |
tests/x11/reboot_gnome.pm
Outdated
|
||
for (my $i = 0; $i < $repeat; $i++) { | ||
assert_and_click 'logoutdialog-reboot-highlighted'; | ||
if (!check_screen('logoutdialog-reboot-highlighted', $wait_change)) { |
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.
you are not using $mustmatch
here but hardcoding the string.
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.
It's certainly a question of style but you could use
last unless check_screen($mustmatch, $wait_change);
But I would assume the following to work better for the overall loop content:
wait_screen_change { assert_and_click 'logoutdialog-reboot-highlighted' };
last unless check_screen($mustmatch, 0);
The wait_screen_change
should ensure that either the screen changed or we waited for some time so that also the timeout on check_screen
would not be necessary.
Anyway, I don't think we should just accept the dialog to not accept a mouse press without a bug report and record_soft_failure
b61926f
to
f838962
Compare
Ok, new changes pushed and new verification run: http://copland.arch.suse.de/tests/481 |
tests/x11/reboot_gnome.pm
Outdated
@@ -16,11 +16,26 @@ use strict; | |||
use testapi; | |||
use utils; | |||
|
|||
sub assert_and_click_until_screen_change { | |||
my ($mustmatch, $wait_change, $repeat) = @_; | |||
$wait_change //= 1; |
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 think 1 second wait is very optimistic
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.
and also it will most likely not do what is expected because @SergioAtSUSE uses it as parameter on wait_screen_change
, not assert_and_click
. Just delete that parameter because you don't need it anymore.
- poo#19082
f838962
to
66be570
Compare
New verification run: http://copland.arch.suse.de/tests/482 |
my $i = 0; | ||
|
||
for (; $i < $repeat; $i++) { | ||
wait_screen_change { assert_and_click $mustmatch; }, $wait_change; |
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.
honestly I don't know why but this gives me Useless use of private variable in void context at /var/lib/openqa/cache/lord.arch.suse.de/tests/sle/tests/x11/reboot_gnome.pm line 26.
in logfiles. I guess you can see this in other logfiles as well.
Ticket
Verification run