Apply new structure for yast modules #7317

b10n1k · 2019-04-23T07:15:16Z

Following the proposed diagram we try to apply a structure that will be more tiny and clean for the test modules. The old y2_common, y2x11test, console_yasttest and y2logsstep have been renamed to present their purpose.
the new y2_common now is the base class of the old y2x11test and console_yasttest. In additional the y2logsstep has been removed as base class and it is use Exporter to be imported into the corresponding modules.

With decooupling in mind there is a new NetworkHelper module with code extracted by y2x11test.

Unfortunately, the post_fail_hook is defined in many installation modules. This is not fixed for now as the changes are already kind of big and we can work on them in a follow up task after this has been approved. Also the new lib/y2_logs_helper.pm might need some kind of cleanup to have delicate functions to logs as i dont know how related are some functions defined in it(see select_conflict_resolution)

Related ticket: https://progress.opensuse.org/issues/46676
Needles: N/A
Verification run:
lvm-encrypt-separate-boot@64bit

yast2_gui@64bit

autoyast_gnome@64bit

ext4@64bit

autoyast_error_dialog@64bit

asmorodskyi · 2019-04-25T07:20:33Z

your PR having conflicts so you need to rebase , taking into account how many files it touches it will have conflicts unless it will be merged 2 hours after rebasing :) So I suggest you pick up one person with merge rights seat together in conference tool of your choice and do review and merge it immediately ( in case it will pass review of course :) ) otherwise you will spent a lot of time on rebasing ....
taking into account quantity of touched files I also recommend not to merge it before SLE/openSUSE 15 SP1 release

lib/y2_guitest.pm

rwx788 · 2019-04-25T10:53:56Z

tests/caasp/oci_register.pm

@@ -10,9 +10,10 @@
 # Summary: Type in registration information
 # Maintainer: Martin Kravec <mkravec@suse.com>

+use base 'installbasetest';
+use y2_logs_helper;


I'm not sure if that is right, as y2_logs_helper is new y2logsstep, but with your change postfail_hooks, etc. will be called from installbasetest. y2logsstep mainly contains things for the YaST installer. As per proposal we should have gotten y2_install_base_test instead and have it as a parent class instead of y2logsstep.

i am not sure why caasp jobs needs to use the post_fail_hook. the y2_install_base_test meant for yast tests. althought as it is now it simplify the logic and can be modified accordingly

Point is, we don't want to change behavior of the code, we only change the structure. Previously, post_fail_hook would be executed from lib/y2logsstep.pm, with this change, it will be executed from installbasetest. So it's change in the behavior. From the proposal we should have y2_installbasetest (not sure about naming), which should include relevant parts from current y2logsstep. And common parts can be moved somewhere else, like to the helper you have created.
Hope it's more clear now.

clear enough. let me take care of it

lib/Utils/NetworkHelper.pm

b10n1k

@rwx788 i moved the %setup_nis_nfs_x11 back to the y2_guitest and the setup_static_mm_network to the mm_network and i adjusted the imports accordingly

lib/Utils/NetworkHelper.pm

b10n1k · 2019-04-25T15:47:17Z

tests/caasp/oci_register.pm

@@ -10,9 +10,10 @@
 # Summary: Type in registration information
 # Maintainer: Martin Kravec <mkravec@suse.com>

+use base 'installbasetest';
+use y2_logs_helper;


i am not sure why caasp jobs needs to use the post_fail_hook. the y2_install_base_test meant for yast tests. althought as it is now it simplify the logic and can be modified accordingly

lib/y2_installbase.pm

rwx788 · 2019-04-30T09:22:09Z

tests/autoyast/installation.pm

@@ -18,7 +18,8 @@

 use strict;
 use warnings;
-use base 'y2logsstep';
+use base 'opensusebasetest';
+use y2_logs_helper;


Same as before, this changes behavior, we should use lib/y2_installbase here as parent, in general for most of the files that should be the only change, one parent class to another, other changes should not be required.

b10n1k · 2019-05-10T15:18:21Z

Some VR:
https://openqa.suse.de/tests/2882680
https://openqa.suse.de/tests/2883102#step/accept_license/3
https://openqa.suse.de/tests/2883105
https://openqa.suse.de/tests/2883106

OleksandrOrlov · 2019-05-03T12:44:40Z

lib/y2_module_basetest.pm


 use strict;
 use warnings;
 use testapi;
 use version_utils qw(is_opensuse is_leap is_tumbleweed);
+use y2_logs_helper;


I guess we do not need the import of 'y2_logs_helper' here, as no functions from it are used.

OleksandrOrlov · 2019-05-03T13:04:52Z

lib/y2lan_restart_common.pm

@@ -19,7 +19,8 @@ use Exporter 'import';
 use testapi;
 use utils 'systemctl';
 use version_utils qw(is_sle is_leap);
-use y2_common 'accept_warning_network_manager_default';
+use y2_module_basetest 'accept_warning_network_manager_default';
+use y2_module_consoletest;


y2_module_consoletest is inherited from y2_module_basetest. Maybe importing of y2_module_consoletest will be enough?

Same question for the following modules:

tests/console/lvm_thin_check.pm

tests/console/validate_lvm_encrypt.pm

i agree with the logic. y2_module_consoletest is not needed anyway. so i have removed it

OleksandrOrlov · 2019-05-13T10:11:15Z

tests/iscsi/iscsi_server.pm

@@ -35,7 +35,7 @@ sub run {
    configure_static_dns(get_host_resolv_conf());
    zypper_call 'in yast2-iscsi-lio-server';
    assert_script_run 'dd if=/dev/zero of=/root/iscsi-disk seek=1M bs=8192 count=1';    # create iscsi LUN
-    my $module_name = y2logsstep::yast2_console_exec(yast2_module => 'iscsi-lio-server');
+    my $module_name = y2_module_consoletest::yast2_console_exec(yast2_module => 'iscsi-server');


I guess you've changed the needle here to fail post_fail_hook. Please do not forget to revert it back.

lib/y2_module_consoletest.pm

OleksandrOrlov · 2019-05-13T10:26:04Z

lib/y2_logs_helper.pm

+use Exporter 'import';
+
+
+our @EXPORT_OK = qw(select_conflict_resolution workaround_dependency_issues break_dependency process_unsigned_files deal_with_dependency_issues verify_license_has_to_be_accepted accept_license verify_license_translations get_available_compression);


The functions:

process_unsigned_files

deal_with_dependency_issues

do no exist in y2_logs_helper anymore. Please, remove them from the @EXPORT_OK

nice catch. fixed

OleksandrOrlov · 2019-05-13T10:55:02Z

lib/y2_installbase.pm

+use y2_logs_helper qw(get_available_compression);
+use Exporter 'import';
+
+our @EXPORT_OK = qw(save_upload_y2logs save_system_logs save_strace_gdb_output process_unsigned_files deal_with_dependency_issues);


I guess there is no need to export the functions as static ones as all of them are class methods and require $self for the call.

OleksandrOrlov · 2019-05-13T11:06:19Z

lib/y2_module_consoletest.pm

+    save_upload_y2logs;
+    upload_logs('/var/log/zypper.log', failok => 1);
+    save_system_logs;
+    save_strace_gdb_output('yast');


The functions called without $self:

save_upload_y2logs

save_system_logs

save_strace_gdb_output

It will cause an error. Please, search for:
post_fail_hook failed: Can't call method "investigate_yast2_failure" on an undefined value at /var/lib/openqa/pool/10/os-autoinst-distri-opensuse/lib/y2_installbase.pm line 190.

to see the example in the verification run logs: https://openqa.suse.de/tests/2886984/file/autoinst-log.txt

OleksandrOrlov · 2019-05-13T11:13:50Z

tests/console/yast2_lan_hostname.pm

-    y2logsstep::yast2_console_exec(yast2_module => 'lan');
-    accept_warning_network_manager_default;
+    y2_module_consoletest::yast2_console_exec(yast2_module => 'lan');
+    type_string "Y2DEBUG=1 ZYPP_MEDIA_CURL_DEBUG=1 yast2 \n";


Correct me if I'm wrong but it seems like the line
type_string "Y2DEBUG=1 ZYPP_MEDIA_CURL_DEBUG=1 yast2 \n";

came to your branch after you made a merge. I guess we do not need it here to avoid conflicts while merging.

OleksandrOrlov · 2019-05-13T11:27:10Z

tests/installation/welcome.pm

+            verify_license_translations unless is_sle('15+');
+        }
+        accept_license;
+    }


Please correct me if I'm wrong, but it seems like the changes also came from the merge and should not be presented in your PR.

very likely. i will try to fix it

tests/console/yast2_lan_hostname.pm

DimStar77 · 2019-05-20T07:54:18Z

Seems to cause an error in Tumbleweed:

e.g. https://openqa.opensuse.org/tests/938053#step/yast2_lan_restart/24

# Test died: Undefined subroutine &y2_module_consoletest::yast2_console_exec called at /var/lib/openqa/cache/openqa1-opensuse/tests/opensuse/lib/y2lan_restart_common.pm line 53.

ldevulder · 2019-05-24T07:23:34Z

taking into account quantity of touched files I also recommend not to merge it before SLE/openSUSE 15 SP1 release

Weird, it has been merged before release of 15-SP1... I hope that there will be no impact :)

b10n1k force-pushed the poo46676 branch 7 times, most recently from 272d1a1 to d9d3eb9 Compare April 23, 2019 11:56

rwx788 reviewed Apr 25, 2019

View reviewed changes

lib/y2_guitest.pm Outdated Show resolved Hide resolved

rwx788 reviewed Apr 25, 2019

View reviewed changes

lib/Utils/NetworkHelper.pm Outdated Show resolved Hide resolved

rwx788 reviewed Apr 25, 2019

View reviewed changes

lib/Utils/NetworkHelper.pm Outdated Show resolved Hide resolved

b10n1k force-pushed the poo46676 branch 3 times, most recently from e97fbb6 to 1cb5ee5 Compare April 26, 2019 06:11

b10n1k commented Apr 26, 2019

View reviewed changes

b10n1k force-pushed the poo46676 branch 12 times, most recently from 27ecec8 to 473e5ff Compare April 30, 2019 06:31

rwx788 reviewed Apr 30, 2019

View reviewed changes

lib/y2_installbase.pm Outdated Show resolved Hide resolved

rwx788 reviewed Apr 30, 2019

View reviewed changes

b10n1k force-pushed the poo46676 branch 11 times, most recently from ca630f3 to 004b15d Compare May 10, 2019 11:44

OleksandrOrlov reviewed May 13, 2019

View reviewed changes

b10n1k force-pushed the poo46676 branch 4 times, most recently from 8917969 to 009b685 Compare May 13, 2019 16:54

OleksandrOrlov reviewed May 13, 2019

View reviewed changes

tests/console/yast2_lan_hostname.pm Show resolved Hide resolved

b10n1k force-pushed the poo46676 branch 6 times, most recently from 9ced051 to 4df02ac Compare May 14, 2019 13:08

Apply new structure for yast modules

04a874b

b10n1k force-pushed the poo46676 branch from 4df02ac to 04a874b Compare May 14, 2019 13:10

OleksandrOrlov merged commit 90dddc1 into os-autoinst:master May 16, 2019

b10n1k deleted the poo46676 branch August 24, 2023 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply new structure for yast modules #7317

Apply new structure for yast modules #7317

b10n1k commented Apr 23, 2019

asmorodskyi commented Apr 25, 2019 •

edited

Loading

rwx788 Apr 25, 2019

b10n1k Apr 25, 2019

rwx788 Apr 26, 2019

b10n1k Apr 26, 2019

b10n1k left a comment

b10n1k Apr 25, 2019

rwx788 Apr 30, 2019

b10n1k Apr 30, 2019

b10n1k commented May 10, 2019

OleksandrOrlov May 3, 2019

b10n1k May 13, 2019

OleksandrOrlov May 3, 2019

b10n1k May 13, 2019

OleksandrOrlov May 13, 2019

b10n1k May 13, 2019

OleksandrOrlov May 13, 2019

b10n1k May 13, 2019

OleksandrOrlov May 13, 2019

b10n1k May 13, 2019

OleksandrOrlov May 13, 2019

b10n1k May 13, 2019

OleksandrOrlov May 13, 2019

b10n1k May 13, 2019

OleksandrOrlov May 13, 2019

b10n1k May 13, 2019

DimStar77 commented May 20, 2019 •

edited

Loading

ldevulder commented May 24, 2019

		use Exporter 'import';


		our @EXPORT_OK = qw(select_conflict_resolution workaround_dependency_issues break_dependency process_unsigned_files deal_with_dependency_issues verify_license_has_to_be_accepted accept_license verify_license_translations get_available_compression);

Apply new structure for yast modules #7317

Apply new structure for yast modules #7317

Conversation

b10n1k commented Apr 23, 2019

asmorodskyi commented Apr 25, 2019 • 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

b10n1k left a 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

b10n1k commented May 10, 2019

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

DimStar77 commented May 20, 2019 • edited Loading

ldevulder commented May 24, 2019

asmorodskyi commented Apr 25, 2019 •

edited

Loading

DimStar77 commented May 20, 2019 •

edited

Loading