openQA live image testing: ready for merge?

[Adam Williamson]

On Thu, 2015-03-12 at 08:34 -0400, Josef Skladanka wrote:
> Some preliminary feedback:
> 
> = openqa_fedora =
> 
> == _do_install_and_reboot.pm ==
> 
> Please delete the "anaconda_install_finish" needle, if it is unused.

Don't recall that one off the top of my head (I'm on a bus, on my
laptop) - I'll look.

> anaconda_install_done needle: 
>   * Why is only a part of the button selected?

Because the button says 'Reboot' on non-lives but 'Done' or 'Quit' or
something on lives (because it just exits the installer, it doesn't
reboot). I wanted to avoid duplicating the needle. The text is the same
for both (as long as you're in English), and we need to click the button
so we need to capture a bit of it.

>   * What is the logic behind "assert_and_click" for multiple areas in one needle? Seems like the "click" is done on the last of the areas (judging from the contents of the needle) - is this _always_ true?

The documentation states that it will click the centre of the *smallest*
area.

> == main.pm ==
> 
> ad the contents of:
>   _boot_to_default.pm
>   _live_run_anaconda.pm
>   _anaconda_select_language.pm
> 
> I'm absolutely for splitting this up a bit, but I'd rather have it done in a slightly different way:
>   * rename _boot_to_default.pm to something in the likes of "_handle_bootloader.pm" (/me is bad with names, but it really just handles the grub options...)

My thinking there was that we are likely to want to do *other* stuff
from the bootloader; we have test cases for the media checks, for
booting from the local disk, for running memtest, etc. So I didn't want
to name it too generically.

Now, if you think the best design for those would be to add them into
this needle with conditionals, then the more generic name would make
sense for sure.

>   * merge _live_run_anaconda.pm and _anaconda_select_language.pm into one file, and call it something like "_get_to_anaconda_main_hub.pm"

Yeah, sorry, I just remembered after sending the email that you'd
suggested that - it still seems like a good idea, I'll get to it soon.

> === anaconda_spoke_done.json ===
> 
> Why change the needle, and why in this particular manner? The change looks unnecessary. If there is no particular reason for it, please revert to previous version.

Sorry, indeed I should've explained that. It's quite subtle and it
stumped me for a while. The existing needle is only a 95% match for at
least one point where it's used on the live image, because for some
reason when running inside GNOME, the button has a faint white dashed
rectangle within it; I stared at screenshots for like half an hour
before realizing what was happening.

So I could either lower the needle's tolerance, or tweak the match area
to try and get it over the threshold. I went into GIMP, zoomed in real
close, and selected as much of the button as is not susceptible to
color-dependent anti-aliasing; that was enough to get the match above
96%. We can go with lowering the tolerance instead, if you like.

> === bootloader_bios_live.json ===
> 
> The black area (last "match" area in the needle) is IMHO quite useless - I suspect it is a remain of the original bootloader needle. If there isn't a reason for having it there, please remove the area from the needle.

I'll take a look. I actually wanted to use the same needle for both, but
unfortunately the background of the bootloader is different colors on
live vs. non-live :(

> === gdm.json ===
> 
> I'm not sure why you selected the particular bit of the screen, but it does not really make much sense to me. Why did you not select any of the more distinct areas of the gdm screen?

Because I believe that area is most likely to remain consistent and
unique. For the user name, well, we'll likely have test cases with
different user names and even more than one user (for the 'desktop
login' test). Also, it's a selectable widget, which presents a couple of
problems. GNOME changes the theme quite often, so the selection
highlight color might change. And it's susceptible to being selected or
not selected.

The 'Fedora' logo seems like an 'obvious' choice, except it's been
pretty changeable lately. It was only added in, like, 20 or 21, I think,
and its size has changed too, I think.

As a meta note: I'm taking kind of a general approach of trying to make
generic needles for a given screen for recognition purposes, with the
expectation that if you need to do something specific on the screen that
you can't do from the 'recognition' needle, you'll create another.

> Also, I'd really like for the needle files to be named as close to the "tag" (i.e. "graphical_login" ) as possible, I know that you probably made this with other login managers in mind, but please use "graphical_login_gdm" as the name of the file, instead of plain "gdm".

Sure, can do.

> == conf_test_suites.py ==
> 
> I'm fairly certain that both default_install and package_set_minimal cover QA:Testcase_install_to_VirtIO.

Sure, makes sense. I was aiming for minimal change from the existing
suites, if I removed it inadvertently that was a mistake.

> == openqa_trigger.py ==
> 
> I really don't like the whole check_condition() thing. The name of the function does no correspond to what it does, which is quite unpleasant together with its side-effects (scheduling the jobs, and changing value of the jobs variable), and using variables from out of its scope.

Well, the thing is you need to check condition1 then *only* do
condition2 if condition1 wasn't good enough. At first they were just two
blocks, but they were very similar, so I wanted to split them out.

I'm not sure I agree with 'using variables outside its scope'. It's a
nested function, and I thought it was basically part of Python design
for those to be able to use variables from the parent function.
https://docs.python.org/2/reference/compound_stmts.html states:

"Free variables used in the nested function can access the local
variables of the function containing the def."

> Also, it seems that you forgot to actually fill the uni_done variable, resulting in `if condition and image.arch not in uni_done:` being effectively reduced to `if condition`, and `if not all(arch in uni_done for arch in arches):` reduced to `if True`.

As you can probably tell I was rejigging this little block rapidly on
the fly, so not populating uni_done sounds like just a mistake, I'll
check that and fix it. Obviously the intent is that once we've found one
'universal' image for each arch, we stop and don't replace them.

> So please:
>  * find a more appropriate name for "check_condition()"
>  * pass all necessary variables in arguments
>  * make sure the uni_done variable is filled with the right data, and ideally rename it to something more descriptive of it's purpose.
> 
> I've spent an hour or so tackling it, so please consider this as an example: http://fpaste.org/197044/63062142/ but note that I have not ran the code (so typos are probably present).

Thanks, I'll take a look at it.

> I hope I'm not being too harsh, it is most certainly not my intent to come around that way,

Not at all! I'm all about the feedback :) thanks!
-- 
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | identi.ca: adamwfedora
http://www.happyassassin.net