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

Add test module to disable screensaver #8503

Merged
merged 1 commit into from Sep 25, 2019

Conversation

asdil12
Copy link
Member

@asdil12 asdil12 commented Sep 23, 2019

New try with fix for some issues regarding maint:

diff --git a/lib/main_common.pm b/lib/main_common.pm
index 286ee0952..1c0f094ed 100644
--- a/lib/main_common.pm
+++ b/lib/main_common.pm
@@ -2434,7 +2434,6 @@ sub load_systemd_patches_tests {

 sub load_system_prepare_tests {
     loadtest 'ses/install_ses' if check_var_array('ADDONS', 'ses') || check_var_array('SCC_ADDONS', 'ses');
-    loadtest 'qa_automation/patch_and_reboot' if is_updates_tests;
     # temporary adding test modules which applies hacks for missing parts in sle15
     loadtest 'console/sle15_workarounds' if is_sle('15+');
     loadtest 'console/integration_services' if is_hyperv || is_vmware;
@@ -2451,6 +2450,7 @@ sub load_system_prepare_tests {
 sub load_create_hdd_tests {
     return unless get_var('INSTALLONLY');
     # install SES packages and deepsea testsuites
+    loadtest 'qa_automation/patch_and_reboot' if is_updates_tests;
     load_shutdown_tests;
     if (check_var('BACKEND', 'svirt')) {
         if (is_hyperv) {

Verification:
http://artemis.suse.de/tests/1669
http://artemis.suse.de/tests/1667

@asdil12 asdil12 requested a review from dzedro September 23, 2019 11:23
@dzedro
Copy link
Contributor

dzedro commented Sep 23, 2019

@asdil12
Copy link
Member Author

asdil12 commented Sep 23, 2019

* Serialdev group change doesn't work on 15+
  https://openqa.suse.de/tests/3393478#step/consoletest_setup/32
  https://openqa.suse.de/tests/3393493#step/consoletest_setup/32

Because the parent job that created the HDD_1 image (see job settings) of those two jobs was not ran with my code.
It works when I also run the parent job with my PR code.

@asdil12
Copy link
Member Author

asdil12 commented Sep 23, 2019

* Can you move `loadtest 'qa_automation/patch_and_reboot' if is_updates_tests;` in front of `load_system_prepare_tests();` ?

I can't really follow you on this one - could you send me a patch?

* I think `loadtest 'shutdown/grub_set_bootargs';` should be part of `load_shutdown_tests` as it just appearead in e.g. [qam-minimal+base](https://openqa.suse.de/tests/3393480#step/grub_set_bootargs/1)

Is it a problem that it appeared? I would rather not touch it as I don't know what the side effects would be.

@asdil12
Copy link
Member Author

asdil12 commented Sep 23, 2019

I just saw that resolve_dependency_issues is scheduled twice:
https://openqa.suse.de/tests/3393480#step/resolve_dependency_issues#1/1
It this due to my changes?

@dzedro
Copy link
Contributor

dzedro commented Sep 24, 2019

ok serialdev, makes sense.
Why are you doing all this main.pm changes ? Can't you just add loadtest "x11/disable_screensaver" if any_desktop_is_applicable(); ? It is causing some changes in who knows what tests.

@asdil12 asdil12 marked this pull request as ready for review September 24, 2019 13:23
@asdil12
Copy link
Member Author

asdil12 commented Sep 24, 2019

ok serialdev, makes sense.
Why are you doing all this main.pm changes ? Can't you just add loadtest "x11/disable_screensaver" if any_desktop_is_applicable(); ? It is causing some changes in who knows what tests.

I need to do it like this so that the test module will get scheduled in the default scenarios as well as in the create_hdd scenarios on both opensuse and sle without duplicating code.

It is really time that we switch to YAML scheduling...

@SergioAtSUSE
Copy link
Member

SergioAtSUSE commented Sep 24, 2019

I just saw that resolve_dependency_issues is scheduled twice:
https://openqa.suse.de/tests/3393480#step/resolve_dependency_issues#1/1
It this due to my changes?

All the load_modules I see are in the function load_inst_tests. I dont't see any of your changes causing it.

# First schedule
996         if (get_var('PATTERNS') || get_var('PACKAGES')) {
997             loadtest "installation/resolve_dependency_issues";

# Second schedule
616 sub installyaststep_is_applicable {                                                                                                                                                                                                                                                                                      
617     return !get_var("NOINSTALL") && !get_var("RESCUECD") && !get_var("ZDUP");
618 }
#...
1014     if (installyaststep_is_applicable()) {
1015         loadtest "installation/resolve_dependency_issues" unless get_var("DEPENDENCY_RESOLVER_FLAG");

@SergioAtSUSE
Copy link
Member

@dzedro, waiting for your approval 😉

@SergioAtSUSE SergioAtSUSE merged commit 8bddbd8 into os-autoinst:master Sep 25, 2019
@ldevulder
Copy link
Member

This PR broke your SLES4SAP test with gnome, see #8542 for the fix/workaround.

@ldevulder
Copy link
Member

Hmm I also don't see the execution of disable_screensaver in the verification runs, that's not so good (or maybe it's just that I need another coffe?).

@SergioAtSUSE
Copy link
Member

@ldevulder, I suppose you refer to the verification runs in the description. They were outdated and don't reflect the last changes. You are right, the test module disable_screensaver was not scheduled.

The last verification runs were in a following commit, although no explicit: It works when I also run the parent job with my PR code. where parent job was a link to the verification run on comment #8503 (comment)

Also, there was https://openqa.suse.de/tests/3393480#step/resolve_dependency_issues#1/1 in comment #8503 (comment)

Thanks for bringing it up!

@coolo
Copy link
Contributor

coolo commented Sep 26, 2019

This also broke openSUSE maintenance tests - please fix ASAP: https://openqa.opensuse.org/tests/1040716#next_previous

@coolo
Copy link
Contributor

coolo commented Sep 26, 2019

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

I prepared a revert in #8548 as many Tumbleweed tests as well as more, e.g. openSUSE maintenance, fail in "disable_screensaver". The issues I see in this PR on top:

  • The screensaver test module is a heavy waste of time in many test modules. Can you do the test for the screensaver in a dedicated test module which you run in only individual scenarios and simply disable in all other scenarios?
  • There seem to be some changes in the single commit which do not have a direct relation to screensaver en-/disabling, please create specific commits for the specific changes

turn_off_xfce_screensaver;
}
turn_off_x11_dpms();
sleep 10;
Copy link
Member

Choose a reason for hiding this comment

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

what is this time-waste for?

@okurz
Copy link
Member

okurz commented Sep 26, 2019

I just saw that even in a staging test, e.g. https://openqa.opensuse.org/tests/1041042/file/autoinst-log.txt the test module is now adding 4 minutes (!) to the test runtime, please consider the time invested for test runs.

@asdil12
Copy link
Member Author

asdil12 commented Sep 26, 2019

* The screensaver test module is a heavy waste of time in many test modules. Can you do the test for the screensaver in a dedicated test module which you run in only individual scenarios and simply disable in all other scenarios?

The whole idea of this ticket is to disable the screensaver everywhere.
So only doing so for one scenario makes this pretty useless and we can simply drop this ticket.
@SergioAtSUSE I'm not sure, how to proceed here - wdyt?

* There seem to be some changes in the single commit which do not have a direct relation to screensaver en-/disabling, please create specific commits for the specific changes

They do - but due to our horrible scheduling code the changes are related.
Don't ask me for details though as I forgot about them.

@okurz
Copy link
Member

okurz commented Sep 26, 2019

The whole idea of this ticket is to disable the screensaver everywhere.
So only doing so for one scenario makes this pretty useless and we can simply drop this ticket.
@SergioAtSUSE I'm not sure, how to proceed here - wdyt?

You misunderstood. I suggest: Test the screensaver in a single scenario, disable the screensaver in all scenarios.

[…] due to our horrible scheduling code the changes are related.

Don't blame any scheduling code for convoluted changes which you did not even mention in the commit message. You can always propose to change the schedule code or you can use YAML schedules.

@@ -29,6 +29,7 @@ sub run {
my ($self) = @_;
select_console 'root-console';

disable_serial_getty;
Copy link
Member

Choose a reason for hiding this comment

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

This addition also breaks test on IPMI HW: https://openqa.suse.de/tests/3402340 (tried multiple time). Fixed since the revert of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any relation to this though.
It looks like openQA screws up entering the SSH password.
I have no idea how this could have been broken by my changes.

@SergioAtSUSE
Copy link
Member

@asdil12, I understood what @okurz says in #8503 (comment), I agree with that proposal.

Good point from @ldevulder, we need to consider far more scenarios for the verification runs.

@ldevulder
Copy link
Member

we need to consider far more scenarios for the verification runs.

Clearly :) and also more details, because I don't understand why this disable_serial_getty is now needed (but it's not because I don't understand that this change is not needed ;-)).

@ldevulder
Copy link
Member

Another weird side effect: https://openqa.suse.de/tests/3402666#step/patterns/1.

We can see that scc_deregistration is called to early in the test, and so patterns installation fails later.
Should be more like this test: https://openqa.suse.de/tests/3410947.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants