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.

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

Adjust yast control center for sle15 #3916

Merged
merged 2 commits into from
Nov 23, 2017
Merged

Conversation

rwx788
Copy link
Member

@rwx788 rwx788 commented Nov 21, 2017

Originally we thought we just need to create new set of needles, but
some of modules do not behave as expected or are not pre-installed. This
commit includes additional soft-failures and some minor improvements.

@rwx788 rwx788 added WIP Work in progress and removed WIP Work in progress labels Nov 21, 2017
@@ -34,6 +34,7 @@ sub search {
}
else {
send_key 'alt-s';
send_key 'backspace';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part doesn't work as expected, as when this method called with empty string, we expect that field is emptied then. But with old code we just navigate to the search field.

@@ -136,13 +154,13 @@ sub start_authentication_server {
assert_and_click 'yast2_control-center_authentication-server';
assert_screen [qw(yast2_control-center-authentication-server_install yast2_control-center-authentication-server_configuration)], 90;
if (match_has_tag('yast2_control-center-authentication-server_install')) {
send_key 'alt-i';
wait_still_screen { send_key 'alt-i' };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why calling wait_still_screen when there is an assert_screen just afterwards?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

assert_screen 'yast2_control-center-authentication-server_configuration';
send_key 'alt-c';
wait_screen_change { send_key 'alt-c' };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, why wait_screen_change when there is an assert_screen just afterwards?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

Originally we thought we just need to create new set of needles, but
some of modules do not behave as expected or are not pre-installed. This
commit includes additional soft-failures and some minor improvements.

See [poo#25716](https://progress.opensuse.org/issues/25716).
@rwx788
Copy link
Member Author

rwx788 commented Nov 22, 2017

NOTE: please merge needles for bootloader as well. Needles for control center already merged.

@okurz
Copy link
Member

okurz commented Nov 22, 2017

needles merged. @Zaoliang can you take a look and review the code?

assert_screen 'yast2_control-center-partitioner_expert', timeout => 60;
send_key 'alt-f';
if (is_storage_ng) {
assert_screen 'expert-partitioner-modify-confirmation';
wait_screen_change { send_key 'alt-o' }; #Continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that wait_screen_change again shouldn't be needed. We have a clear assert_screen afterwards. That's enough, isn't it?

@@ -273,6 +291,12 @@ sub run {
if (is_sle && sle_version_at_least '15') {
#see bsc#1062331, sound is not added to the yast2 pattern
ensure_installed('in yast2-sound');
#kdump is disabled by default, so ensure that it's installed
ensure_installed('in yast2-kdump');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is "yast2-kdump" related to the comment that "kdump is disabled by default"? Btw, it shouldn't be disabled by default because as far as I remember at least on a default SLES installation it will enable kdump during installation, isn't it?

Copy link
Member Author

@rwx788 rwx788 Nov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kdump is disabled in installation by default, hence nothing is installed and this is expected. In the qcow image used here it's disabled and we lack defined flow how to be sure if it's there or not in given image.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, it's disabled by default. I also checked SLE 12-SP3, nevermind

@@ -273,6 +291,12 @@ sub run {
if (is_sle && sle_version_at_least '15') {
#see bsc#1062331, sound is not added to the yast2 pattern
ensure_installed('in yast2-sound');
#kdump is disabled by default, so ensure that it's installed
ensure_installed('in yast2-kdump');
record_soft_failure 'bsc#1059569';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work to check for the error condition first? E.g. with multi-tag assert_screen and match_has_tag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which error condition? We can verify that package is not installed, then soft-fail and install, or see that module is not there -> install -> restart control center, but that looks like an overhead for this case. We can also just fail on the bug and see which decision is made.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fine then

}
else {
start_wake_on_lan;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it looks cleaner if that is handled within the start_wake_on_lan method. Also this should allow to check for the error condition first as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, here we can do proper soft-fail without workaround: https://gitlab.suse.de/openqa/os-autoinst-needles-sles/merge_requests/579 Please, merge new needle.

send_key 'alt-y';
assert_screen [qw(yast2_control-center-partitioner_warning yast2_control-center-partitioner_expert)], 180;
# Define if storage-ng
set_var('STORAGE_NG', 1) if match_has_tag 'storage-ng';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure ... I would prefer to keep decision maker as a human for this variable. It's too important to leave it like this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the same approach in the installer, so for this test suite variable is not set. Of course we can also set it on all mediums, but this line of code doesn't hurt anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmorodskyi there is a long history behind that variable. Simply stated: We won't go back anytime soon :)

@rwx788
Copy link
Member Author

rwx788 commented Nov 23, 2017

With latest enhancement to wake on lan we need one needle for sle:
https://gitlab.suse.de/openqa/os-autoinst-needles-sles/merge_requests/579
Here is verification run: http://g226.suse.de/tests/459#

In the test we check value in dropdown, but then it overlaps other
controls. Pressing esc key closes it.
@okurz
Copy link
Member

okurz commented Nov 23, 2017

all needles PRs/MRs merged

@okurz okurz merged commit 385ccde into os-autoinst:master Nov 23, 2017
@rwx788 rwx788 deleted the 27074_yast2gui branch January 10, 2018 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants