try to be safer when typing in X: slower, more checks
ClosedPublic

Authored by adamwill on Sep 5 2016, 6:44 AM.

Details

Summary

the main thing this does is try and type slower in X - this
should cover nearly everywhere we type anything in X, and make
it type slower. We also add a bit more safety checking to some
old tests which didn't have it (mainly _do_install_and_reboot)

  • wait_still_screen after typing to make sure all the keypresses

were registered before continuing.

This is an attempt to mitigate the problems we've seen where
the wrong text gets typed into the wrong places and the tests
break.

This branch is live on staging atm. It still has *some* issues,
but I do think it's an improvement.

Test Plan

run the tests (probably several times), compare to
runs without the change, see if it's better or worse...

Diff Detail

Repository
rOPENQATESTS os-autoinst-distri-fedora
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
adamwill retitled this revision from to try to be safer when typing in X: slower, more checks.Sep 5 2016, 6:44 AM
adamwill updated this object.
adamwill edited the test plan for this revision. (Show Details)
adamwill added reviewers: jskladan, garretraziel.
adamwill added a comment.EditedSep 5 2016, 6:55 AM

to elaborate a bit on the changes from multiple send_key calls to type_string with special characters: I think mutliple send_key calls in sequence with nothing between them are dangerous as they may lead to typing too fast, which can result in the keypresses getting lost or happening out of order. There is no kind of confirmation of send_key calls (or VNC events in general, really) - the function just fires off the appropriate command over the wire and returns immediately, it does not verify (and really cannot verify) that the keypress was actually received and applied before returning.

type_string also cannot actually *verify* that keypresses worked, but it does have some protection against typing too fast, and you can make it even slower with the argument, like we do all the time in this diff. So I think it's much safer to do type_string "\t\t\t", 20; than send_key "tab"; send_key "tab"; send_key "tab";, even if the latter is a bit more obvious to read.

in general this commit tries to avoid all cases where we stack up send_key events, or do type_string immediately after send_key with no kind of wait in the middle.

garretraziel added inline comments.Sep 5 2016, 1:11 PM
lib/freeipa.pm
23–24

type_string "\t\t\t\t", 20?

adamwill updated this revision to Diff 2533.Sep 7 2016, 5:37 AM

considerable rework with check_type_string

this is quite a lot further down the road from the previous
version, using a check_type_string function which checks for
screen changes while typing, and with nicer syntax and a few
other tweaks.

So a few more notes here. I have two main concerns about this:

  1. if we happen to hit a failure case where we're trying to type but we don't have the right interface element selected or whatever, so the keystrokes don't result in the screen changing at all, this will make the test take rather longer to fail (depending on the size value and the length of the string).
  1. all the wait_screen_change calls probably increase load on the worker box somewhat. I dunno what the cost of that call is exactly, but I don't think it's *outrageous* - I compared several test execution times between prod and staging and overall these changes don't really seem to make tests take longer.

It also does have some limitations. Most obviously if anything *else* that's going on causes the screen to change, it'll keep typing even if there are no screen changes actually caused by the typing. There's very little we can do about that, it's just kind of a fundamental limitation of this approach. There's one case where this problem renders the function totally useless - typing into cockpit when the constantly scrolling graph of system load is visible - so we don't use it there, we just type real slow. Note that the cockpit FreeIPA enrolment test, and the freeipa webUI test that runs after it, aren't actually working right now because for some reason the cockpit join domain screen doesn't work at all; I need to finally figure out what's going on there and fix it. If any of the blind changes to those tests don't quite work right, I'll fix it in the diff to fix the enrolment problem.

still, overall I think this is an improvement. We may run into little issues down the road and maybe have to tweak the implementations of the convenience functions and/or switch between safely and very_safely in a few places, or something, but I think this implementation gives us the tools to twiddle relatively easily. I've done quite a lot of runs with this code on staging and it seems pretty promising, it definitely reduces the number of typing fails.

oh, one more note - I've submitted check_type_string upstream: https://github.com/os-autoinst/os-autoinst/pull/584 . If that gets merged, we can simply drop it out of main_common.

I took the main_common name from SUSE, figured we may as well follow their conventions. It might better be called util (they have a util.pm too), I don't really know, doesn't seem like a big deal. Once this gets merged we can move several other things that aren't really class methods out of the classes and into this module (or make some other new ones, for specialized things).

adamwill updated this revision to Diff 2536.Sep 8 2016, 8:54 PM

rebase on current develop

adamwill updated this revision to Diff 2539.Sep 9 2016, 4:45 AM

rebase again

adamwill updated this revision to Diff 2541.Sep 9 2016, 8:24 PM

add handling of 'Finish configuration' state

with the slow typing, we're now more likely to hit a special case
in the install hub; if we don't finish User creation before anaconda
gets to post-install, it stops and waits, showing a 'Finish
configuration' button we have to click before the post-install steps
will run. So we just check for that button on return from the user
creation spoke, and click it if it's there. Include needles for all
languages, just on the offchance we somehow hit it with English or
French.

adamwill updated this revision to Diff 2542.Sep 9 2016, 9:53 PM

go back and type the user password again if we hit a warning bar

so even with all the slow typing and screen checking, a few times
I've seen a test somehow miss one character when typing the user
password for the second time. So let's call this the 'belt, braces,
superglue and constantly checking to see if your pants have fallen
down' approach: if there's a warning bar after we type the password
for the second time, go back and try typing it again.

I like this 'warning_bar' needle (rather than a specific 'passwords
don't match' warning needle) because it's not susceptible to font
changes or language differences, and we don't lose much by trying
to type the passwords again if the error happens to be something
other than 'passwords don't match'. And we can use the needle in
other cases in future if needed.

I can't recall if I've seen similar on the root password spoke,
but I'll probably add the same workaround there just in case.

garretraziel accepted this revision.Sep 12 2016, 4:47 PM
This revision is now accepted and ready to land.Sep 12 2016, 4:47 PM
This revision was automatically updated to reflect the committed changes.