Fix firewall activation warning in yast2_lan_restart.pm #3956

Zaoliang · 2017-11-28T14:08:16Z

Fix firewall activation warning, add softfail for yast2_lan_restart.pm

fix issue with warning shows up at:
https://openqa.suse.de/tests/1274792#step/yast2_lan_restart/30
add record_soft_failure bsc#1070578
see poo#25676 and poo#28501 for details
verification run:
http://e13.suse.de/tests/318
needles PR:
Fix issue with warning about firewall activation for yast2_lan_restart.pm os-autoinst-needles-opensuse#292

ldevulder · 2017-11-28T15:59:06Z

tests/x11/yast2_lan_restart.pm

+        send_key 'alt-o';
+    }
+    else {
+        assert_screen 'yast2_closed_xterm_visible', 120;


It's not really a comment on the code but more a question: is assert_screen 'yast2_closed_xterm_visible', 120; really useful?
As if the this code is executed it should has been already did with assert_screen ([qw(yast2-lan-restart_firewall_active_warning yast2_closed_xterm_visible)], 120);, no?

And so it could be executed two times. But maybe I'm wrong :)

I don't know. I just try to fix the code here ;)
Actually we want to re-work the code according to changes from sles 15/leap 15.0.
yast2_closed_xterm_visible', 120; -- is needed here so we can be sure that lan got closed correctly and it is ready for following tests.

assert_screen checks only needles and has nothing to do with closing xterm.

Could you try to remove the else { assert_screen ... } and test if it still works?

well, yast2_closed_xterm_visible is needed anyway, for sles 12 and leap lower than 15.0
Just want to avoid to use check_screen here like if (check_screen 'yast2-lan-restart_firewall_active_warning') {...}

Agreed that it is needed for older version, but assert_screen([qw(yast2-lan-restart_firewall_active_warning yast2_closed_xterm_visible)], 120); is not intended to test both? As I understand the code, it tests if one of yast2-lan-restart_firewall_active_warning or yast2_closed_xterm_visible is found. If yast2-lan-restart_firewall_active_warning is found it does specific stuff, and it it's the other it does assert_screen again, which may not be needed for me, as it has already been checked (on all sle/opensuse version).

But again, it's more for me to understand better how openQA works :-) This is why I asked if a test without it was possible or not.

it is needed for both cases. so I added this yast2_closed_xterm_visible now. It looks better:
http://e13.suse.de/tests/26#step/yast2_lan_restart/39

okurz · 2017-11-29T15:12:20Z

tests/x11/yast2_lan_restart.pm

@@ -42,7 +42,20 @@ sub check_network {
    $status //= 'no_restart';
    wait_still_screen;
    send_key 'alt-o';                     # OK
-    assert_screen 'yast2_closed_xterm_visible', 120;
+                                          # new: warning pops up for firewall, alt-y for assign it to zone


Better add a blank line in before so that the comment is not placed in this weird location

okurz · 2017-11-29T15:12:50Z

tests/x11/yast2_lan_restart.pm

+    assert_screen([qw(yast2-lan-restart_firewall_active_warning yast2_closed_xterm_visible)], 120);
+    if (match_has_tag 'yast2-lan-restart_firewall_active_warning') {
+        send_key 'alt-y';
+        wait_still_screen;


That means at least 7 seconds of waiting, better use something else, at least a reduced stilltime

okurz · 2017-11-29T15:13:45Z

tests/x11/yast2_lan_restart.pm

+        assert_screen 'yast2_closed_xterm_visible', 120;
+    }
+    #  for sure that it comes back to root console
+    wait_still_screen;


The above assert_screen 'yast2_closed_xterm_visible', 120; outside of the if-else should do that job. I don't see the need for this wait_still_screen

@okurz it's more for my personal knowledge, but could you explain me why the assert_screen 'yast2_closed_xterm_visible', 120; after the else is needed? The assert_screen ([qw(yast2-lan-restart_firewall_active_warning yast2_closed_xterm_visible)], 120);1 should not do the job already?

I don't want to insist ;-) I just want to understand :)

Zaoliang · 2017-11-29T15:36:31Z

updated again.
@okurz let's talk about this offline.

Zaoliang · 2017-12-01T08:52:37Z

@okurz record_soft_failure added, please check, thanks!

nicksinger · 2017-12-01T08:53:23Z

Could you please rerun your verification run so we can see the soft-fail working? The rest looks good to me.

Zaoliang · 2017-12-01T08:54:44Z

@nicksinger there is verification run:

http://e13.suse.de/tests/latest?version=15.0&arch=x86_64&machine=64bit&test=extra_tests_on_gnome&distri=opensuse&flavor=DVD#step/yast2_lan_restart/319

Zaoliang · 2017-12-01T08:57:49Z

or this is better: http://e13.suse.de/tests/52

Zaoliang · 2017-12-01T09:10:58Z

why my commit message didn't get updated with my update of commit ?

okurz · 2017-12-03T07:20:54Z

code change looks ok but I think you introduced a new needle check and therefore we are missing the corresponding needle PRs/MRs. Also regarding your last question: It seems like your commit was updated but maybe you mean your PR description? That is not the same so you need to update your PR description within github.

Zaoliang · 2017-12-04T09:13:27Z

needle PR:
os-autoinst/os-autoinst-needles-opensuse#292

okurz · 2017-12-04T09:59:00Z

Please rebase your test code
Fix the travis reported test failures: https://travis-ci.org/os-autoinst/os-autoinst-distri-opensuse/builds/311203534?utm_source=github_status&utm_medium=notification
Update your PR description with the needle PR and make sure it does not show like a "code block". Probably you just have to remove some whitespace before the text so that it does not show up as this "grey box".

Zaoliang · 2017-12-04T15:33:04Z

@okurz PR updated, please check and merge, thanks!

okurz · 2017-12-05T12:37:02Z

tests/x11/yast2_lan_restart.pm

    check_network('restart');
 }

 sub run {
    select_console 'x11';
    x11_start_program("xterm -geometry 155x50+5+5", target_match => 'xterm');
    become_root;
+    # make sure that firewalld is stopped, or we have later pops for firewall activation warning
+    # or timeout for command 'ip a' later
+    script_run "systemctl stop firewalld";


Please don't add a script_run without checking the return value. Can you simply call assert_script_run or can the "stop" command fail here?

It was assert_script_run, then I change it into script_run. But it is fine with assert_script_run. Please check, thanks!

- fix issue with warning shows up at: https://openqa.suse.de/tests/1274792#step/yast2_lan_restart/30 - add record_soft_failure bsc#1070578 - see poo#25676 and poo#28501 for details - verification run: http://e13.suse.de/tests/52 - needles PR: os-autoinst/os-autoinst-needles-opensuse#292

DimStar77 · 2017-12-07T07:43:39Z

Causes failure on TW (not on firewalld yet):

https://openqa.opensuse.org/tests/552697#step/yast2_lan_restart/14

See os-autoinst#3956 (comment)

ldevulder reviewed Nov 28, 2017

View reviewed changes

okurz reviewed Nov 29, 2017

View reviewed changes

okurz reviewed Dec 5, 2017

View reviewed changes

okurz merged commit 227c8a6 into os-autoinst:master Dec 6, 2017

okurz added a commit to okurz/os-autoinst-distri-opensuse that referenced this pull request Dec 7, 2017

yast2_lan_restart: Prevent trying to stop non-existing firewalld

b7e4f13

See os-autoinst#3956 (comment)

okurz mentioned this pull request Dec 7, 2017

yast2_lan_restart: Prevent trying to stop non-existing firewalld #4027

Merged

okurz added a commit to okurz/os-autoinst-distri-opensuse that referenced this pull request Dec 7, 2017

yast2_lan_restart: Prevent trying to stop non-existing firewalld

2b2ad74

See os-autoinst#3956 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix firewall activation warning in yast2_lan_restart.pm #3956

Fix firewall activation warning in yast2_lan_restart.pm #3956

Zaoliang commented Nov 28, 2017 •

edited

Loading

ldevulder Nov 28, 2017

Zaoliang Nov 28, 2017

ldevulder Nov 28, 2017

Zaoliang Nov 29, 2017

ldevulder Nov 29, 2017

Zaoliang Nov 29, 2017

okurz Nov 29, 2017

okurz Nov 29, 2017

okurz Nov 29, 2017

ldevulder Dec 1, 2017

Zaoliang commented Nov 29, 2017

Zaoliang commented Dec 1, 2017

nicksinger commented Dec 1, 2017

Zaoliang commented Dec 1, 2017

Zaoliang commented Dec 1, 2017

Zaoliang commented Dec 1, 2017

okurz commented Dec 3, 2017

Zaoliang commented Dec 4, 2017

okurz commented Dec 4, 2017

Zaoliang commented Dec 4, 2017

okurz Dec 5, 2017 •

edited

Loading

Zaoliang Dec 6, 2017

DimStar77 commented Dec 7, 2017

Fix firewall activation warning in yast2_lan_restart.pm #3956

Fix firewall activation warning in yast2_lan_restart.pm #3956

Conversation

Zaoliang commented Nov 28, 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

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 29, 2017

Zaoliang commented Dec 1, 2017

nicksinger commented Dec 1, 2017

Zaoliang commented Dec 1, 2017

Zaoliang commented Dec 1, 2017

Zaoliang commented Dec 1, 2017

okurz commented Dec 3, 2017

Zaoliang commented Dec 4, 2017

okurz commented Dec 4, 2017

Zaoliang commented Dec 4, 2017

okurz Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimStar77 commented Dec 7, 2017

Zaoliang commented Nov 28, 2017 •

edited

Loading

okurz Dec 5, 2017 •

edited

Loading