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
Stabilize KDE live installation on aarch64 #10474
Stabilize KDE live installation on aarch64 #10474
Conversation
The commit fixes check_screen method call to pass arrayref there instead of actual array. Related ticket: poo#66355
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved but you have forgot one small thing In L51
@@ -173,7 +169,7 @@ sub run { | |||
send_key 'alt-o'; # ok | |||
next; | |||
} | |||
if (get_var('LIVECD') and (match_has_tag('screenlock') || match_has_tag('blackscreen'))) { | |||
if (get_var('LIVECD') and (check_screen('screenlock') || check_screen('blackscreen'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering how is it better of multi tag assert screen we did previously? Does it help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this line it does not actually matters. But it matters on the previous steps:
Previously 'screenlock' and 'blackscreen' were part of @tags array. Then this array was used in while loop on line 132: my $ret = check_screen \@tags, 30;
. This check was designed to define if installation is finished ('rebootnow' needle) or interrupted with 'yast_error', 'yast2_wrong_digest' or other popups. For some reason 'screenlock' and just recently 'blackscreen' were added to this @tags array. Maybe it made sense in the past, but now this leads to the incorrect detecting of the installation state.
I removed these tags from @tags array. And on this line I just changed to check_screen
, because obviously it will not work with match_has_tag
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the code I don't see any difference. As we have this condition in the end of the list. So to me those look as equivalent (except timing), so if you say that helps, we can keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a broken change to me. In the screenlock
or blackscreen
case this code isn't reached due to the next;
in line 143, is it?
181ef5e
to
4c16182
Compare
The commit changes the behavior of the test module to move mouse, cursor while installation is performed with Live CD. That is done to prevent screenlock to be appeared. Change is done in scope of work to stabilize the installation test suite with Live CD installation on aarch64. Related ticket: poo#66355
4c16182
to
7e79403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks man. Approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see problems with this PR:
- You are invalidating the test. The idea is not to call a low-level script but test from user perspective. Clicking the icon to start the installer is the correct way.
- I am not convinced that just a handful of verification runs can prove stability in this case. And I don't trust that the additional non-efficient
check_screen
is better than the multi-tag assert screen
I would very much prefer if you revert your change and find a better way.
@Vogtinator you might also want to comment on this.
@@ -48,7 +48,7 @@ sub run { | |||
assert_and_click 'live-upgrade'; | |||
} | |||
else { | |||
assert_and_click 'live-installation'; | |||
x11_start_program('xdg-su -c "/usr/sbin/start-install.sh"', target_match => 'maximize'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is invalidating the test case as the point is to simulate what the user does without relying on internals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should definitely be reverted. If assert_and_click
does not work for the simple case of starting an application, something is really broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with you. This is another way how user could do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vogtinator This is not a problem of a product. You'll not be able to reproduce the issue manually, unless you are holding mouse click for a second or more. This is sporadic issue related to connection/vnc. When we'll revert the change we'll have the test sporadically failing and it will be false fails, as the product still works correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're practically suggesting to replace all assert_and_click
by x11_start_program
here because of some weird VNC issue here, which I don't think is adequate.
The VNC issue has to be fixed in any case. That a single 0.15s press is suddenly ~7x longer is really broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, that's exactly what I have proposed to @OleksandrOrlov just now, as on 64bit scenario works just fine.
@OleksandrOrlov WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but do you need the change at all as you already bumped RAM and CPUs for the test scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oliver, you will not believe, but I tried to bump RAM and CPU first before introducing any new changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, good.
Yes, I would also prefer a revert here. |
On all architectures, except aarch64, start Installer Wizard by clicking on the appropriate icon. On aarch64 use x11_start_program instead due to sporadic issue when mouse is hold too long, so that context pop-up is appeared. The decision to revert the click on icon for other architectures was made during discussion in PR os-autoinst#10474. Related ticket: https://progress.opensuse.org/issues/66355
The PR adds the required steps to stabilize Tumbleweed installation using Live CD on aarch64.
Changes introduced:
x11_start_program
, which also allows to open the app but without relying on click;sub handle_livecd_screenlock
seems to be not effective with the current product, but it is still left (maybe temporary), just to not broke existing tests that potentially may use it (e.g. on x64). Though, the way howcheck_screen
is used here was changed. As previously it used array in function arguments instead of arrayref, that is required by function definition;QEMURAM=4096
QEMUCPUS=4
TIMEOUT_SCALE=4
were added to Test Suite settings.https://openqa.opensuse.org/tests/1293429
https://openqa.opensuse.org/tests/1293430
https://openqa.opensuse.org/tests/1293431
https://openqa.opensuse.org/tests/1293432