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

Assure focused password textbox #6596

Merged

Conversation

lib/utils.pm Outdated
@@ -756,7 +756,7 @@ sub handle_login {
select_user_gnome($myuser);
}
}
assert_screen 'displaymanager-password-prompt', no_wait => 1;
assert_and_click 'displaymanager-password-prompt';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the password prompt be always selected? This worked in before so either we have a bug that we do not want to hide or something else is broken but I would like to not just silently accept faulty product behaviour.

@SergioAtSUSE SergioAtSUSE changed the title Assure focused password textbox [WIP] Assure focused password textbox Jan 21, 2019
@SergioAtSUSE SergioAtSUSE added WIP Work in progress and removed WIP Work in progress labels Jan 21, 2019
@SergioAtSUSE SergioAtSUSE changed the title [WIP] Assure focused password textbox Assure focused password textbox Jan 22, 2019
@SergioAtSUSE SergioAtSUSE force-pushed the assure_focused_password_textbox branch from 8908ab2 to 89d9b02 Compare January 22, 2019 12:35
lib/utils.pm Outdated
@@ -761,7 +761,8 @@ sub handle_login {
select_user_gnome($myuser);
}
}
assert_screen 'displaymanager-password-prompt', no_wait => 1;
record_soft_failure('bsc#1122664 - password prompt is not focused') if (check_screen 'displaymanager-unfocused-password-prompt');
Copy link
Member

Choose a reason for hiding this comment

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

and again this can not work reliably because check_screen has a 0 timeout, meaning there is no waiting between the former selection of user and the password prompt.
Please use a multi-tag assert_screen with match_has_tag instead of check_screen with non-zero timeout to prevent introducing any timing dependant behaviour, to save test execution time as well as state more explicitly from the testers point of view what are the expected alternatives. For example:

assert_screen([qw(yast2_console-finished yast2_missing_package)]);
if (match_has_tag('yast2_missing_package')) {
    send_key 'alt-o';  # confirm package installation
    assert_screen 'yast2_console-finished';
}

Copy link
Member Author

@SergioAtSUSE SergioAtSUSE Jan 22, 2019

Choose a reason for hiding this comment

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

With a blinking cursor, using multi-tag assertion you have 50% of chances of getting false positives. But, anyway I just realized that asserting an unfocused textbox is also wrong. Because the unfocused textbox looks the same as a focused textbox at an instant when the cursor is off.

Copy link
Member

Choose a reason for hiding this comment

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

  1. There is the option "no_wait" on e.g. assert_screen to cover "blinking cursor" if you must
  2. Better avoid the blinking cursor, e.g. with a proper needle design, could be an exclude area
  3. Are you sure unfocussed looks the same as focussed but cursor off? If you mean by "cursor off", while it's blinking I remember in gdm that there is still a small white border around the box

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. assert_screen will break the test on unfocused textbox
  2. We are exactly trying to match this area, we cannot exclude it.
  3. Yes, see: https://progress.opensuse.org/issues/46223#note-6

@SergioAtSUSE SergioAtSUSE changed the title Assure focused password textbox [WIP] Assure focused password textbox Jan 22, 2019
@SergioAtSUSE SergioAtSUSE added the WIP Work in progress label Jan 22, 2019
@SergioAtSUSE SergioAtSUSE force-pushed the assure_focused_password_textbox branch from 5229a1a to 9c570ef Compare January 22, 2019 17:11
@SergioAtSUSE SergioAtSUSE removed the WIP Work in progress label Jan 22, 2019
@SergioAtSUSE SergioAtSUSE changed the title [WIP] Assure focused password textbox Assure focused password textbox Jan 22, 2019
@SergioAtSUSE
Copy link
Member Author

@okurz, what do you think now?

lib/utils.pm Outdated
@@ -761,7 +761,10 @@ sub handle_login {
select_user_gnome($myuser);
}
}
assert_screen 'displaymanager-password-prompt', no_wait => 1;
if (!check_screen('displaymanager-password-prompt', 30)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why not to use multitag assert here or that's not possible to catch unfocused password prompt?
Advantage will be that if password prompt is unfocused, we won't wait for 30 seconds before selecting it.

Copy link
Member

Choose a reason for hiding this comment

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

+1 what @rwx788 says, the check_screen is a no-go for me here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is not possible.

As I stated here: #6596 (comment)

And I explained it here: https://progress.opensuse.org/issues/46223#note-6

Comparing this job where it worked with a failing one:

There is no difference between a focused textbox from a non-focused textbox. If you try to verify if it is not focused, you get false positives, since there is a 50% chance (it really depends on the blinking frecuency, but you get the point) of getting an focused textbox without cursor.
The only thing we can be sure is that if we see the cursor, it is focused, and to avoid the test to fail, I need check_screen.

Copy link
Member

Choose a reason for hiding this comment

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

But the two screens look quite different. Maybe only very subtle a one-pixel border around the entryfield which looks like black-on-black but the other logos are bright in one case and dim in the other case. Can't we use that? If it is really not possible to detect if the password field is unfocussed then this is a pretty messed up bug in sddm that should definitely be reported and fixed by adjusting the design.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I can agree with an UX bug. There should be a unambiguous difference between a focused and an unfocused textbox.

Creating the bug and adapting the test.

Copy link
Member

Choose a reason for hiding this comment

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

That should be most likely an upstream bug. Have you looked on bugs.kde.org if maybe there is one already? But honestly I am not yet sure if that is really the problem. As I stated: IMHO the two screens do look quite different

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 didn't.

I have found a list of bugs that could be related. bugs.kde.org redirected me to https://github.com/sddm/sddm for sddm bugs.

I put here the list to carefully check them tomorrow (I just did a quick look):

At least openQA and in my monitors I don't see any difference in the textbox. I will try to use it manually to see if there is another element in the screen that can give a hint about a focused password textbox

Copy link
Member

Choose a reason for hiding this comment

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

Does

focussed_unfocussed

convince you? The upper half is "focussed", the lower is "unfocussed", e.g. by manually selecting the key below the textfield. The "focussed" field has a blurry blue-ish border

@rwx788
Copy link
Member

rwx788 commented Jan 24, 2019

Don't we need needles for SLE? Or this issue doesn't occur there?

@SergioAtSUSE
Copy link
Member Author

Don't we need needles for SLE? Or this issue doesn't occur there?

Good point.

I as can see, the test module is only used by opensuse:

sergio@sergio-latitude:~/github/os-autoinst-distri-opensuse$ git --no-pager  grep user_gui_login
lib/main_common.pm:    loadtest "x11/user_gui_login" if is_opensuse && !get_var("LIVETEST") && !get_var("NOAUTOLOGIN");

@SergioAtSUSE SergioAtSUSE changed the title Assure focused password textbox [WIP] Assure focused password textbox Jan 24, 2019
@SergioAtSUSE SergioAtSUSE added the WIP Work in progress label Jan 24, 2019
@SergioAtSUSE SergioAtSUSE changed the title [WIP] Assure focused password textbox Assure focused password textbox Jan 29, 2019
@SergioAtSUSE SergioAtSUSE removed the WIP Work in progress label Jan 29, 2019
@SergioAtSUSE SergioAtSUSE changed the title Assure focused password textbox [WIP] Assure focused password textbox Jan 29, 2019
@SergioAtSUSE
Copy link
Member Author

I still need to rebase the commits

@SergioAtSUSE SergioAtSUSE force-pushed the assure_focused_password_textbox branch from 6775998 to 7d56b3b Compare January 29, 2019 16:55
@SergioAtSUSE SergioAtSUSE changed the title [WIP] Assure focused password textbox Assure focused password textbox Jan 29, 2019
@SergioAtSUSE
Copy link
Member Author

Ready to be reviewed (and merged)

@rwx788 rwx788 merged commit 10fa5f7 into os-autoinst:master Jan 29, 2019
@fcrozat
Copy link
Contributor

fcrozat commented Jan 30, 2019

Please revert this NOW, this is breaking all openQA tests on SLE15 SP1.

@nilxam
Copy link
Member

nilxam commented Jan 30, 2019

and all leap tests.

@SergioAtSUSE
Copy link
Member Author

Fixing it in: #6669

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

Successfully merging this pull request may close these issues.

6 participants