-
Notifications
You must be signed in to change notification settings - Fork 269
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
Kill PackageKit if it blocks YaST in 2 more occasions (poo#33517) #4818
Conversation
assert_screen \@tags; | ||
wait_screen_change { send_key 'alt-n' } if match_has_tag('yast2_control-center_update-repo-dialogue'); | ||
# Let it kill PackageKit, in case it is running. | ||
wait_screen_change { send_key 'alt-y' } if match_has_tag('yast2_control-center-ask_packagekit_to_quit'); |
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.
these wait_screen_change should not be necessary because we call an assert_screen afterwards anyway, right?
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.
In case the pop-up takes a bit longer to disappear that would mean the same needle would apply again and it would press the same key combination again on the next loop iteration.
Not sure if that would hurt.
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'm with @cwh42 here, we saw similar issue already. I would rather think how many iterations should we conduct, as if pop-up is there, we will timeout after 2 hours only (the risk of such scenario is rather low).
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 see, makes sense
do { | ||
assert_screen \@tags; | ||
# Let it kill PackageKit, in case it is running. | ||
wait_screen_change { send_key 'alt-y' } if match_has_tag('yast2_control-center-ask_packagekit_to_quit'); |
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.
as above
@@ -341,7 +350,7 @@ sub run { | |||
start_security_center; | |||
start_sudo; | |||
start_user_and_group_management; | |||
start_hypervisor; | |||
#start_hypervisor; |
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.
Don't add disabled code as nobody but you will understand why this is not active. Better leave it out and keep in your local git repository, either in git stash
or a temporary "WIP"-commit.
Also, we want to keep this on products/versions where it still works.
https://progress.opensuse.org/issues/34405 seems to be the related issue. Maybe we just need to explicitly install the package within start_hypervisor
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.
@cwh42 could you please revert this change or put logical condition to schedule it when required only? I guess point with wait_screen_change is clarified.
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 still miss a kind of agreement of what I think about the wait_screen_change
.
According to poo#34405 start_hypervisor
only gets installed on SLE - so I'll wrap it in an appropriateif
.
Moved |
Handle the PackageKit dialog in two more parts of that test.
Also I commented out
start_hypervisor
, since it apparently is not part of a TW default installation any more.