Assert fullname mistype with setting #9808

SergioAtSUSE · 2020-03-17T10:27:06Z

Description

Use full name 'bernhard' by default.
Allow to check mistyped full name with setting ASSERT_BSC1122804=1

Related ticket

https://progress.opensuse.org/issues/46190

asmorodskyi · 2020-03-17T10:37:27Z

less affected maybe , but still it could mistype so it is not a solution , see #9807 . In my understanding it is bad idea to check everything everywhere . Why HPC test need to fail in case of this bug ? Why we need to cancel all HPC related check because of this bug ? Why we need more than one test to fail with same problem ? How it help to solve the problem ?

SergioAtSUSE · 2020-03-17T10:44:44Z

less affected maybe , but still it could mistype so it is not a solution, see #9807

Mistype is independent of this change, it is dependent on HOST/SUT performance.
I agree with #9807 as workaround for HPC.

In my understanding it is bad idea to check everything everywhere.

I agree.

Why HPC test need to fail in case of this bug? Why we need to cancel all HPC related check because of this bug? Why we need more than one test to fail with same problem?

They don't / we don't

How it help to solve the problem?

This PR helps by using a fullname that is less affected by https://bugzilla.opensuse.org/show_bug.cgi?id=1122804
So, HPC tests can use a different name and forget about mistyping.

asmorodskyi · 2020-03-17T10:45:29Z

less affected maybe , but still it could mistype so it is not a solution, see #9807

Mistype is independent of this change, it is dependent on HOST/SUT performance.
I agree with #9807 as workaround for HPC.

In my understanding it is bad idea to check everything everywhere.

I agree.

Why HPC test need to fail in case of this bug? Why we need to cancel all HPC related check because of this bug? Why we need more than one test to fail with same problem?

They don't / we don't

How it help to solve the problem?

This PR helps by using a fullname that is less affected by https://bugzilla.opensuse.org/show_bug.cgi?id=1122804
So, HPC tests can use a different name and forget about mistyping.

ok , LGTM

foursixnine · 2020-03-17T10:51:59Z

tests/installation/user_settings.pm

@@ -39,7 +39,7 @@ sub run {
    my $max_tries = 4;
    my $retry     = 0;
    do {
-        $self->enter_userinfo(max_interval => ($retry) ? utils::VERY_SLOW_TYPING_SPEED : undef);
+        $self->enter_userinfo(username => get_var('USER_FULLNAME', undef), max_interval => ($retry) ? utils::VERY_SLOW_TYPING_SPEED : undef);


this means tthat per product/jobgroup, we can end up with at least 1 needle right?... This is going to be a maintenance burden... But I'd vote for a less complicated name, as long as at least one test, has the full old name..

If they all choose the same name 'bernhard' it should only be one needle

Should then be IGNORE_BSC1122804=1?

if (get_var(IGNORE_BSC1122804)) { $self->enter_userinfo(username => 'bernhard'); assert_screen('inst-userinfostyped-just-bernhard'); } else { my $max_tries = 4; my $retry = 0; do { $self->enter_userinfo(max_interval => ($retry) ? utils::VERY_SLOW_TYPING_SPEED : undef); assert_screen([qw(inst-userinfostyped-ignore-full-name inst-userinfostyped-expected-typefaces)]); record_soft_failure('boo#1122804 - Typing issue with fullname') unless match_has_tag('inst-userinfostyped-expected-typefaces'); $retry++; } while (($retry < $max_tries) && !match_has_tag('inst-userinfostyped-expected-typefaces')); assert_screen('inst-userinfostyped-expected-typefaces'); # fail if mistyped }

can you please rebase your PR on latest master , since #9807 already merged and it will be more clear what you trying to achieve

b10n1k · 2020-03-17T12:39:33Z

Could you update also https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/variables.md with the new variable? I think the PR is fine so i will approve asap i see the VR.

SergioAtSUSE · 2020-03-17T13:19:40Z

Verification runs

Without setting, assert_screen done: http://openqa.slindomansilla-vm.qa.suse.de/tests/2312#step/user_settings/5
With setting, no assert_screen done: http://openqa.slindomansilla-vm.qa.suse.de/tests/2313#step/user_settings/3

b10n1k

LGTM

asmorodskyi · 2020-03-17T14:11:04Z

tests/installation/user_settings.pm

@@ -50,7 +50,7 @@ sub run {
            record_soft_failure('boo#1122804 - Typing issue with fullname') unless match_has_tag('inst-userinfostyped-expected-typefaces');
            $retry++;
        } while (($retry < $max_tries) && !match_has_tag('inst-userinfostyped-expected-typefaces'));
-        assert_screen('inst-userinfostyped-expected-typefaces');    # fail if mistyped
+        assert_screen('inst-userinfostyped-expected-typefaces') unless get_var('IGNORE_BSC1122804');    # fail if mistyped


what if you will replace my check_var('SLE_PRODUCT','hpc') with this 'IGNORE_BSC1122804' ? so we will get less soft failures in case people really want to ignore this problem ?

I am not convinced of having green when there is a mistype.

AFAICT @schlad is more into HPC, removing it, would at least deserve pinging him... no? :)

AFAICT @schlad is more into HPC, removing it, would at least deserve pinging him... no? :)

ehm , I would say there is no point until I did not convince @SergioAtSUSE :)

I am not convinced of having green when there is a mistype.

do you know "Test One Thing at a Time in Isolation" testing principle ? I am not saying that you "should have green" . I just saying that when you have 100 tests make all of them red or yellow because of single problem is also wrong and does not show real state of the product. there are two type of installation scenarios currently in openQA , one meant to check installation itself and they do need to become red/yellow in case any troubles but second type is for delivering images to other tests. This second type should be more error tolerant because they not meant to answer question "Does installation contain bugs?" but they answering another question "Is it possible to finish installation?" . And it is not exclusively about HPC product there are plenty of other cases when people actually get annoyed by failures like this one and even soft-failure is just a noise for them which does not change anything for state of their product

I am with Anton. Installation should detect any possible issue, but other jobs that create hdd should not care about this as long as the installation performs well.

that is the theory and then there is the practice, where people make wrappers around wrappers (grub, first_boot, reboot, opensuse-welcome, etc)

Let the test go green with a mistyped user and someone will implement another workaround in another part of the test suite to solve the same problem we are already solving in one part.

that is the theory and then there is the practice, where people make wrappers around wrappers (grub, first_boot, reboot, opensuse-welcome, etc)

Let the test go green with a mistyped user and someone will implement another workaround in another part of the test suite to solve the same problem we are already solving in one part.

mmm not sure that I can follow you here , my point was that we having check which not related to most of the jobs where we having it. And you mentioning something about bad coding practices which we have in this repo . How one related to another ?

that is the theory and then there is the practice, where people make wrappers around wrappers (grub, first_boot, reboot, opensuse-welcome, etc)

I personally fully see your point!

"Is it possible to finish installation?" . And it is not exclusively about HPC product there are plenty of other cases when people actually get annoyed by failures like this one and even soft-failure is just a noise for them which does not change anything for state of their product

And they can set IGNORE_BSC1122804=1 in the jobgroup and be happy with their lives. What we can do is that IGNORE_BSC1122804=1 also removes the soft-falure and paints the test green for those who like to paint non critical errors green, be happy.

OleksandrOrlov · 2020-03-17T16:26:56Z

@SergioAtSUSE There are 45 failed test runs in current build due to user_settings module. Also, this is sporadic fail.

Does it mean that we should add this IGNORE_BSC1122804=1 to all of the test suites, where the issue may potentially occur (except several ones, where we want to continue highlighting the issue)? Maybe it is better to not make assert_screen but instead use check_screen with record_soft_failure combination? WDYT?

asmorodskyi · 2020-03-17T16:35:31Z

@SergioAtSUSE There are 45 failed test runs in current build due to user_settings module. Also, this is sporadic fail.

Does it mean that we should add this IGNORE_BSC1122804=1 to all of them (except several ones, where we want to continue highlighting the issue)? Maybe it is better to not make assert_screen but instead use check_screen with record_soft_failure combination? WDYT?

Well if you will add IGNORE_BSC1122804=1 to 45 tests you will get 45 soft-failures which would bring you to another problem ( smaller one but still ) - too many soft-failures and trend from everyone to ignore soft-failure state . So actually your question proving my idea which I mention above ( well at least for me :D ). Also you comment brings me to idea that maybe we need to revert the logic to FAIL_ON_BSC1122804 ?

SergioAtSUSE · 2020-03-17T16:35:57Z

@SergioAtSUSE There are 45 failed test runs in current build due to user_settings module. Also, this is sporadic fail.

Does it mean that we should add this IGNORE_BSC1122804=1 to all of them (except several ones, where we want to continue highlighting the issue). Maybe it is better to not make assert_screen but instead use check_screen with record_soft_failure combination? WDYT?

No, because that was the case before. And we want it to fail when the workaround fails.
What we need to find is a way of having both, people ignoring the failed workaround and people not wanting to.

asmorodskyi · 2020-03-17T16:40:13Z

What we need to find is a way of having both, people ignoring the failed workaround and people not wanting to.

I can not speak about others but I am not "person who want to ignore this failure" . I just think that single test failed with this is enough to track the problem and all other 44 failures just creating noise which not allow to clearly see other problems

SergioAtSUSE · 2020-03-17T16:42:51Z

I can not speak about others but I am not "person who want to ignore this failure" . I just think that single test failed with this is enough to track the problem and all other 44 failures just creating noise which not allow to clearly see other problems

It is not so easy. Any mistyped fullname is a new needle to match login screen on display manager and causing matches on the wrong place which also causes login problems.

SergioAtSUSE · 2020-03-17T16:52:04Z

I will update this PR tomorrow to:

use fullname: 'bernhard' as default
- addresing @foursixnine's concern about people creating one needle per fullname (only two fullnames 'bernhard' and 'Bernhard M. Wiedemann'
- Addresing @asmorodskyi's concern about more people needing to use a setting to ignore a test that is only interesting for functional
use setting ASSERT_BSC1122804=1 so that only functional team has to add "something" to their test to get that part covered.

Then it would be the functional team who decides if we only use that setting for one scenario (I would say gnome, because it doesn't create qcow2 for other tests)

asmorodskyi · 2020-03-17T16:54:32Z

It is not so easy. Any mistyped fullname is a new needle to match login screen on display manager and causing matches on the wrong place with also causes login problems.

Yes and such case we really need to fail , totally agree ! But your comment clearly exclude soft-failure option here . So we suppose to have two option - if next job in call chain rely on username we should fail , if not we may ignore it . This is why I suggest to move your IGNORE_BSC1122804 to if which I introduce and add variable to each test which actually will keep working if there was typo during username typing . But after @OleksandrOrlov comment we should also check how many job actually need this check and how many may ignore it so maybe we really need to reverse logic ...

okurz · 2020-03-18T06:58:20Z

@schlad when you don't want to test the ISO I recommend to use jeos images or built your own using kiwi, for bare metal use autoyast when you can't deploy full installation images to machines. This is all avoiding any interaction with the installer.

The interactive GUI installer is a different but still important test target for two reasons: It is used as "showcase" when SLE is evaluated on customer site, by reviewers and reporters. And second: The installer provides default settings that would provide a sane default that needs to be tested as well which is more than just individual rpms, e.g. the settings from control.xml, partitioning proposal, kernel boot parameters, default patterns installed, services enabled. You can assume rpms to be at least tested a bit by package maintainers, the whole system is not, people rely on QA for this so far.

SeroSun · 2020-03-18T07:03:36Z

LGTM

schlad · 2020-03-18T07:08:33Z

@schlad when you don't want to test the ISO I recommend to use jeos images or built your own using kiwi, for bare metal use autoyast when you can't deploy full installation images to machines. This is all avoiding any interaction with the installer.

The interactive GUI installer is a different but still important test target for two reasons: It is used as "showcase" when SLE is evaluated on customer site, by reviewers and reporters. And second: The installer provides default settings that would provide a sane default that needs to be tested as well which is more than just individual rpms, e.g. the settings from control.xml, partitioning proposal, kernel boot parameters, default patterns installed, services enabled. You can assume rpms to be at least tested a bit by package maintainers, the whole system is not, people rely on QA for this so far.

Thanks for the reminder.

SeroSun · 2020-03-18T09:44:03Z

I triggered a VR for your new push.
http://openqa.suse.de/tests/4004332
This case was failed without this PR.
https://openqa.nue.suse.com/tests/3998083

But this seems blocked by hostname_inst issue in #9795

asmorodskyi · 2020-03-18T10:02:00Z

tests/installation/user_settings.pm

        # retry if not typed correctly
        my $max_tries = 4;
        my $retry     = 0;
        do {
-            $self->enter_userinfo(max_interval => ($retry) ? utils::VERY_SLOW_TYPING_SPEED : undef);
+            $self->enter_userinfo(max_interval => utils::VERY_SLOW_TYPING_SPEED);


I think this line should be reverted , you need to use $retry variable here

Why?

The reason to have 'undef' (normal speed) is for the scenarios not affected by the bug.
Now that this if-branch is only to hit it, we want to always type slow. This will only happen to functional tests that use the setting.

ah ok I see , make sense . please ignore my comment above than

asmorodskyi · 2020-03-18T10:03:51Z

tests/installation/user_settings.pm

-        assert_screen([qw(inst-userinfostyped-ignore-full-name inst-userinfostyped-expected-typefaces)]);
-    }
-    else {
+    if (get_var('ASSERT_BSC1122804')) {


+1 , I think it is best option which would make everyone happy

Could you please update PR's description? You have old name there.

asmorodskyi · 2020-03-18T10:04:40Z

@SergioAtSUSE - LGTM for your last version !

asmorodskyi · 2020-03-18T10:05:27Z

@SergioAtSUSE - LGTM for your last version !

of course with revert of line which I commented on :)

asmorodskyi · 2020-03-18T10:32:44Z

@SergioAtSUSE - LGTM for your last version !

of course with revert of line which I commented on :)

solved

- Use full name 'bernhard' by default. - Allow to check mistyped full name with setting ASSERT_BSC1122804=1

SergioAtSUSE · 2020-03-18T12:16:08Z

New verification runs:

jlausuch · 2020-03-19T00:21:12Z

new failures:
https://openqa.suse.de/tests/4007070#step/user_settings/4
https://openqa.suse.de/tests/4006730#step/user_settings/4

asmorodskyi · 2020-03-19T07:53:00Z

new failures:
https://openqa.suse.de/tests/4007070#step/user_settings/4
https://openqa.suse.de/tests/4006730#step/user_settings/4

just to not confuse people who will read this - failure mentioned by Jose is obvious one - no one created needles with new username after merging this patch

juadk · 2020-03-19T08:01:32Z

new failures:
https://openqa.suse.de/tests/4007070#step/user_settings/4
https://openqa.suse.de/tests/4006730#step/user_settings/4

just to not confuse people who will read this - failure mentioned by Jose is obvious one - no one created needles with new username after merging this patch

Yes it's obvious but thanks for the information because we have some failed tests about that regarding HA. Shame not to have been warned before RC1.

SergioAtSUSE · 2020-03-19T08:24:28Z

new failures:
https://openqa.suse.de/tests/4007070#step/user_settings/4
https://openqa.suse.de/tests/4006730#step/user_settings/4

just to not confuse people who will read this - failure mentioned by Jose is obvious one - no one created needles with new username after merging this patch

Wrong, the title of the panel changed.

Local Users -> Local User

Nothing to do with this PR.
There is no assert_screen if you didn't use ASSERT_BSC1122804=1!

SergioAtSUSE · 2020-03-19T08:25:56Z

ignore-fullname should have matched and therefore continue. No need to create a new expected-typefaces

juadk · 2020-03-19T08:58:15Z

ignore-fullname should have matched and therefore continue. No need to create a new expected-typefaces

@SergioAtSUSE Yes, I just got it!
We must create a new needle because the current one (user_settings-inst-userinfostyped-ignore-full-name-sle-15.2-20200225) do not match due to Local Users change. So I agree it's not related to this PR. It's a coincidence, sorry for the confusion.

SergioAtSUSE requested review from foursixnine, Soulofdestiny, asmorodskyi and rwx788 March 17, 2020 10:27

This was referenced Mar 17, 2020

Remove extra screen_assertion for user_settings #9793

Closed

Only fail for fullname mistype on functional #9803

Closed

asmorodskyi approved these changes Mar 17, 2020

View reviewed changes

foursixnine reviewed Mar 17, 2020

View reviewed changes

SergioAtSUSE mentioned this pull request Mar 17, 2020

user_settings: Not fail on mistyped user name for HPC #9807

Merged

SergioAtSUSE force-pushed the override_fullname_with_setting branch from d20ff1c to b5cf9d6 Compare March 17, 2020 13:16

SergioAtSUSE marked this pull request as ready for review March 17, 2020 13:19

b10n1k approved these changes Mar 17, 2020

View reviewed changes

SergioAtSUSE changed the title ~~Override fullname with USER_FULLNAME~~ Ignore fullname mistype with setting Mar 17, 2020

foursixnine approved these changes Mar 17, 2020

View reviewed changes

asmorodskyi reviewed Mar 17, 2020

View reviewed changes

SergioAtSUSE added the notready label Mar 17, 2020

SergioAtSUSE changed the title ~~Ignore fullname mistype with setting~~ [WIP] Ignore fullname mistype with setting Mar 17, 2020

SergioAtSUSE force-pushed the override_fullname_with_setting branch from b5cf9d6 to 4d14740 Compare March 18, 2020 08:36

asmorodskyi reviewed Mar 18, 2020

View reviewed changes

SergioAtSUSE force-pushed the override_fullname_with_setting branch from 4d14740 to 6b5249b Compare March 18, 2020 11:30

Assert fullname mistype with setting

f40aabc

- Use full name 'bernhard' by default. - Allow to check mistyped full name with setting ASSERT_BSC1122804=1

SergioAtSUSE force-pushed the override_fullname_with_setting branch from 6b5249b to f40aabc Compare March 18, 2020 11:55

SergioAtSUSE removed the notready label Mar 18, 2020

SergioAtSUSE changed the title ~~[WIP] Ignore fullname mistype with setting~~ Ignore fullname mistype with setting Mar 18, 2020

SergioAtSUSE merged commit 017ae93 into os-autoinst:master Mar 18, 2020

SergioAtSUSE deleted the override_fullname_with_setting branch March 18, 2020 12:58

SergioAtSUSE changed the title ~~Ignore fullname mistype with setting~~ Assert fullname mistype with setting Mar 18, 2020

foursixnine mentioned this pull request Mar 18, 2020

Decrease max_interval for re-typing fullname #9804

Closed

OleksandrOrlov mentioned this pull request Jan 20, 2022

Add local user and root user setting up with LibyuiClient #12831

Merged

Assert fullname mistype with setting #9808

Assert fullname mistype with setting #9808

Conversation

SergioAtSUSE commented Mar 17, 2020 • edited Loading

Description

Related ticket

asmorodskyi commented Mar 17, 2020

SergioAtSUSE commented Mar 17, 2020

asmorodskyi commented Mar 17, 2020

foursixnine Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioAtSUSE Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b10n1k commented Mar 17, 2020

SergioAtSUSE commented Mar 17, 2020

Verification runs

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioAtSUSE Mar 17, 2020 • 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

OleksandrOrlov commented Mar 17, 2020 • edited Loading

asmorodskyi commented Mar 17, 2020

SergioAtSUSE commented Mar 17, 2020

asmorodskyi commented Mar 17, 2020

SergioAtSUSE commented Mar 17, 2020 • edited Loading

SergioAtSUSE commented Mar 17, 2020 • edited Loading

asmorodskyi commented Mar 17, 2020

okurz commented Mar 18, 2020

SeroSun commented Mar 18, 2020

schlad commented Mar 18, 2020

SeroSun commented Mar 18, 2020

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

asmorodskyi commented Mar 18, 2020

asmorodskyi commented Mar 18, 2020

asmorodskyi commented Mar 18, 2020

SergioAtSUSE commented Mar 18, 2020

jlausuch commented Mar 19, 2020

asmorodskyi commented Mar 19, 2020

juadk commented Mar 19, 2020

SergioAtSUSE commented Mar 19, 2020

SergioAtSUSE commented Mar 19, 2020

juadk commented Mar 19, 2020

SergioAtSUSE commented Mar 17, 2020 •

edited

Loading

foursixnine Mar 17, 2020 •

edited

Loading

SergioAtSUSE Mar 17, 2020 •

edited

Loading

SergioAtSUSE Mar 17, 2020 •

edited

Loading

OleksandrOrlov commented Mar 17, 2020 •

edited

Loading

SergioAtSUSE commented Mar 17, 2020 •

edited

Loading

SergioAtSUSE commented Mar 17, 2020 •

edited

Loading