Complete workarounds for yast_proxy.pm and yast2_samba.pm #3881

Zaoliang · 2017-11-07T14:14:21Z

Complete workarounds for yast_proxy.pm and yast2_samba.pm

workaround with susefirewall2 for proxy server
workaround with susefirewall2 for samba server
record softfail for samba server, add new workflow for changes
add record_soft_failure "bsc#1068900" for samba-server
proxy server: add record_soft_failure bsc#1069458
verification run TW: http://e13.suse.de/tests/31
TW samba-server failed at early stage because of boo#1068938
verification run Leap 15.0: http://e13.suse.de/tests/4593
proxy failed because squid is missing
verification run SLES 15: http://e13.suse.de/tests/30
verification run SLES 12 SP3: http://e13.suse.de/tests/33
add record_soft_failure "bsc#1068900" for samba-server
Related ticket: https://progress.opensuse.org/issues/26998

rwx788 · 2017-11-08T13:35:18Z

tests/console/yast2_proxy.pm

-    send_key 'alt-a';
-    wait_screen_change { send_key 'alt-o' };
-
+    if (!is_sle && !sle_version_at_least('15')) {


Which distri are we aiming here? I believe we can use is_tumbleweed here

will re-write this: using sub ti splitt into: setup_ldap, samba_current, samba_new so that to have a clear structure :) thanks for review!

ops, this is about samba-server, not about proxy. will take a look at code for proxy.

it should be:
if ((is_tumbleweed) || (is_sle && !sle_version_at_least('15'))

rwx788 · 2017-11-08T13:37:26Z

Please, provide verification run on TW to prove that we don't break anything there. Ideally also one on SP3

okurz · 2017-11-12T11:54:54Z

tests/console/yast2_samba.pm

+    if (is_sle && sle_version_at_least('15')) {
+        assert_screen 'yast2_samba-workgroup';
+        send_key 'alt-n';
+        if (check_screen 'yast2_still_susefirewall2') {


Please avoid check_screen with non-zero timeout

okurz · 2017-11-12T11:55:22Z

tests/console/yast2_samba.pm

-    send_key 'alt-o';
-
-    wait_serial('yast2-samba-server-status-0', 60) || die "'yast2 samba-server' didn't finish";
+    # continue current workflow for sle 13 and current TW


There is no SLE 13

okurz · 2017-11-12T11:56:01Z

tests/console/yast2_samba.pm

+
+        # check samba server status
+        assert_script_run("systemctl show -p ActiveState smb.service | grep ActiveState=active");
+    }
 }


Way too long for an else branch. Please extract into method

rwx788 · 2017-11-20T17:01:10Z

tests/console/yast2_proxy.pm

-    send_key 'alt-a';
-    wait_screen_change { send_key 'alt-o' };
-
+    if ((is_tumbleweed) || (is_sle && !sle_version_at_least('15'))) {


What's about leap? I believe we need even more ugly check here with "|| !leap_version_at_least('15')"

leap 15.0 has been covered now: http://e13.suse.de/tests/4570#step/yast2_samba/32

Please try to avoid the explicit statement of is_tumbleweed as it should be considered our reference which we never need to check for. Rather turn the check around. Also you are not yet stating the Leap changes. Please make sure your code is always pushed in the most recent version before commenting back that it has been solved so that we know what to check for. On top I don't understand why (new) tumbleweed should behave the same as old-SLE? Is there a package submission gone missing for openSUSE:Factory?

this comment still applies

changed now.

rwx788 · 2017-11-20T17:06:07Z

tests/console/yast2_samba.pm

-    if (match_has_tag 'yast2_still_susefirewall2') {
-        record_soft_failure 'bsc#1064405';
-        send_key 'alt-e';
+    if (!is_sle && !sle_version_at_least('15')) {


I'm not sure if this is correct condition. sle_version_at_least always returns true if it's not sle distri (that's confusing part), so it will return false only if sle version is lower than 15, meaning, this condition will be never true. It's easier to get it if you flip it: !(is_sle || sle_version_at_least('15')). If it's not sle second condition gives true, if it's sle, then first one is already true. After negation, it's always false.

okay, !(is_sle || sle_version_at_least('15')) is clear to me as well.

rwx788 · 2017-11-20T17:07:28Z

tests/console/yast2_samba.pm

+    # start samba server configuration
+    script_run("yast2 samba-server; echo yast2-samba-server-status-\$? > /dev/$serialdev", 0);
+    # check Samba-Server Configuration got started
+    if ((is_tumbleweed) || (is_sle && sle_version_at_least('15'))) {


We use this ugly condition twice here, let's move it to the lib, and again we need to make it work on sle 15.

we do care new TW and sles 15, leap is not important atm. the whole yast2 test group is not activated for leap.

That's not true, we have leap tests running on o3. They are not in best shape, but it's mainly because we had no time to fix them. So I believe we have to at least define expected behavior, so only new set of needles is required in best case.

atm o3 is not testing leap for yast2 at all. do we have check var like "is_leap"?
if yes, I can add this however..

okurz · 2017-11-21T22:37:38Z

tests/console/yast2_proxy.pm

@@ -83,19 +83,23 @@ sub run {
        if (check_screen 'yast2_proxy_firewall_enabled') {
            send_key 'alt-p';
            wait_still_screen 3;
+            # now we have a new popup to workaround for question about firewall settings
+            if (check_screen 'yast2_proxy_squid_cannot-open-interface') {


As you know we want to avoid check_screens with non-zero timeout

okay, add 10 sec then

okurz · 2017-11-21T22:38:29Z

tests/console/yast2_samba.pm

+sub gui_new {
+    assert_screen 'yast2_samba-workgroup';
+    send_key 'alt-n';
+    if (check_screen 'yast2_still_susefirewall2') {


change this as well

- see os-autoinst/os-autoinst-distri-opensuse#3881 - see poo#7360 for more details - verification run: http://e13.suse.de/tests/4570#step/yast2_samba/32

okurz · 2017-11-22T12:04:14Z

tests/console/yast2_proxy.pm

+            # now we have a new popup to workaround for question about firewall settings
+            if (check_screen 'yast2_proxy_squid_cannot-open-interface') {
+                send_key 'alt-y';
+            }


You already have experience with better ways, multi-tag assert_screen with match_has_tag. I don't see a reason why we should not use that here

this part is quite old, of course I can re-work this now

checked this again, I don't think it is better to keep this because we don't need to check more than 2 needles here:
http://e13.suse.de/tests/4551#step/yast2_proxy/12

okurz · 2017-11-22T12:04:51Z

tests/console/yast2_samba.pm

+sub gui_new {
+    assert_screen 'yast2_samba-workgroup';
+    send_key 'alt-n';
+    if (check_screen 'yast2_still_susefirewall2', 10) {


okurz · 2017-11-22T12:05:08Z

tests/console/yast2_samba.pm

+        record_soft_failure 'bsc#1064405';
+        send_key 'alt-c';
+    }
+    wait_still_screen;


the default stilltime of 7 seconds is a big high, don't you think?

reduce to 3

okurz · 2017-11-22T12:06:29Z

tests/console/yast2_samba.pm

+sub run {
+    setup_ldap;
+
+    # install samba stuffs at first


the comment is a bit redundant. Everyone should be able to understand the next line just as well

okay, removed

okurz · 2017-11-22T12:06:33Z

tests/console/yast2_samba.pm

+    # install samba stuffs at first
+    zypper_call("in samba yast2-samba-server");
+
+    # start samba server configuration


here as well

removed now

okurz · 2017-11-22T12:07:14Z

tests/console/yast2_samba.pm

+    # continue current workflow for sle 12
+    else {
+        gui_current;
+    }


Better use the same approach that you did further up by only looking for the specific older versions. This allows to not explicitly repeat the is_tumbleweed check

okay, will change like:

if (((is_leap) && !(leap_version_at_least('15.0'))) || (is_sle && !(sle_version_at_least('15')))) {
gui_current;
}
# continue current workflow for sle 12
else {
gui_new;
}

leap_version_at_least returns false in case it's not leap, so is_leap part is not really required

rwx788 · 2017-11-22T13:52:41Z

tests/console/yast2_samba.pm

-        record_soft_failure 'bsc#1064405';
-        send_key 'alt-e';
+    # only older version like sles 12, tw, leap should still check the needle
+    if ((is_sle && !(sle_version_at_least('15'))) || (is_leap && !(leap_version_at_least('15.0')))) {


same here, do not need is_leap

okurz

Please incorporate all suggestions including the one about check_screen. I think we should care about the sleep time because even if we only check for two needles a standard check_screen will waste 30 seconds when there is nothing to much. We know much better ways how to handle that.

It should be easier to separate the PRs for yast2_proxy and yast2_samba

Zaoliang · 2017-11-23T09:06:23Z

checked this again for check_screen in proxy and samba. Only one place is not defined timeout.

Now I changed this as well.
if (check_screen 'yast2_proxy_squid_cannot-open-interface', 3) {
record_soft_failure 'bsc#1069458';
send_key 'alt-y';
}

Zaoliang · 2017-11-23T09:24:56Z

again, for

if (check_screen 'yast2_proxy_squid_cannot-open-interface', 3) {
record_soft_failure 'bsc#1069458';
send_key 'alt-y';
}

there is atm no other option to workaround. Without check_screen here I don't think there is another way to handle it.

rwx788 · 2017-11-23T13:02:29Z

tests/console/yast2_samba.pm

-        record_soft_failure 'bsc#1064405';
-        send_key 'alt-e';
+    # only older version like sles 12, tw, leap should still check the needle
+    if ((is_sle && !sle_version_at_least('15')) || is_leap && !leap_version_at_least('15.0')) {


Please, either remove wrapping parenthesis for both && conditions, or wrap is_leap && ....
Code will work, as logical AND has precedence on OR, but it's inconsistent from style perspective.

In the comment you mention "tw" but Tumbleweed should not activate that branch. I think that comment is wrong

okurz · 2017-11-23T13:37:17Z

tests/console/yast2_proxy.pm

            send_key 'alt-p';
            wait_still_screen 3;
+            # now we have a new popup to workaround for question about firewall settings
+            if (check_screen 'yast2_proxy_squid_cannot-open-interface', 3) {


I don't think that "3" seconds is a good choice. It should be either 0 seconds when the previous "wait_still_screen" made sure that the dialog shows up correctly in this case or the default when we can not ensure anything. But the best approach should be to use a multi-tag assert_screen with match_has_tag. Can you point me to an openQA test that detected this screen and recorded the following soft-failure?

okurz · 2017-11-23T13:40:48Z

tests/console/yast2_proxy.pm

-    send_key 'alt-a';
-    wait_screen_change { send_key 'alt-o' };
-
+    if ((is_tumbleweed) || (is_sle && !sle_version_at_least('15'))) {


Please try to avoid the explicit statement of is_tumbleweed as it should be considered our reference which we never need to check for. Rather turn the check around. Also you are not yet stating the Leap changes. Please make sure your code is always pushed in the most recent version before commenting back that it has been solved so that we know what to check for. On top I don't understand why (new) tumbleweed should behave the same as old-SLE? Is there a package submission gone missing for openSUSE:Factory?

okurz · 2017-11-23T13:41:37Z

tests/console/yast2_samba.pm

-        record_soft_failure 'bsc#1064405';
-        send_key 'alt-e';
+    # only older version like sles 12, tw, leap should still check the needle
+    if ((is_sle && !sle_version_at_least('15')) || is_leap && !leap_version_at_least('15.0')) {


In the comment you mention "tw" but Tumbleweed should not activate that branch. I think that comment is wrong

okurz · 2017-11-23T13:42:30Z

tests/console/yast2_samba.pm

-    if (match_has_tag 'yast2_still_susefirewall2') {
-        record_soft_failure 'bsc#1064405';
-        send_key 'alt-e';
+    # only older version like sles 12, tw, leap should still check the needle


s/sles/SLE/

okurz · 2017-11-23T13:43:53Z

tests/console/yast2_samba.pm

+    zypper_call("in samba yast2-samba-server");
+    script_run("yast2 samba-server; echo yast2-samba-server-status-\$? > /dev/$serialdev", 0);
+    # check Samba-Server Configuration got started
+    if (is_leap && !leap_version_at_least('15.0') || (is_sle && !sle_version_at_least('15'))) {


I strongly recommend to surround both leap-checks with parentheses, i.e. write if ((is_leap && !leap_version_at_least('15.0')) || (is_sle && !sle_version_at_least('15'))) {

- add workaround needles for firewall port and needles for firewall port in samba server - add workaround needle for open interface in proxy server - see related ticket: https://progress.opensuse.org/issues/25478 - see PR: os-autoinst/os-autoinst-distri-opensuse#3881 - verification run (Leap): http://e13.suse.de/tests/4593

Zaoliang · 2017-11-24T17:17:57Z

@okurz please check my updated PR if you have time, thanks!

- workaround with susefirewall2 for proxy server - workaround with susefirewall2 for samba server - record softfail for samba server, add new workflow for changes - add record_soft_failure "bsc#1068900" for samba-server - proxy server: add record_soft_failure bsc#1069458 - verification run TW: http://e13.suse.de/tests/31 TW samba-server failed at early stage because of boo#1068938 - verification run Leap 15.0: http://e13.suse.de/tests/4593 proxy failed because squid is missing - verification run SLES 15: http://e13.suse.de/tests/30 - verification run SLES 12 SP3: http://e13.suse.de/tests/33 - add record_soft_failure "bsc#1068900" for samba-server - Related ticket: https://progress.opensuse.org/issues/26998

Zaoliang · 2017-12-01T09:17:05Z

@okurz please check and merge, thanks!

rwx788 reviewed Nov 8, 2017

View reviewed changes

okurz reviewed Nov 12, 2017

View reviewed changes

rwx788 reviewed Nov 20, 2017

View reviewed changes

okurz reviewed Nov 21, 2017

View reviewed changes

Zaoliang mentioned this pull request Nov 22, 2017

Add needle for openSUSE Leap because of changed workflow os-autoinst/os-autoinst-needles-opensuse#288

Merged

okurz reviewed Nov 22, 2017

View reviewed changes

rwx788 reviewed Nov 22, 2017

View reviewed changes

okurz requested changes Nov 23, 2017

View reviewed changes

rwx788 reviewed Nov 23, 2017

View reviewed changes

okurz reviewed Nov 23, 2017

View reviewed changes

Zaoliang mentioned this pull request Nov 24, 2017

Add new needles for workaround for proxy and samba server os-autoinst/os-autoinst-needles-opensuse#291

Merged

okurz approved these changes Dec 3, 2017

View reviewed changes

okurz merged commit f425b48 into os-autoinst:master Dec 3, 2017

Complete workarounds for yast_proxy.pm and yast2_samba.pm #3881

Complete workarounds for yast_proxy.pm and yast2_samba.pm #3881

Conversation

Zaoliang commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwx788 commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okurz left a comment

Choose a reason for hiding this comment

Zaoliang commented Nov 23, 2017

Zaoliang commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zaoliang commented Nov 24, 2017

Zaoliang commented Dec 1, 2017

Zaoliang commented Nov 7, 2017 •

edited

Loading