Hey, folks. So I've been tweaking and testing my openQA live image test stuff today, and I think it's probably about ready for merging.
I've got branches of both openqa_fedora and openqa_fedora_tools with relevant changes:
https://www.happyassassin.net/cgit/openqa_fedora/log/?h=live https://www.happyassassin.net/cgit/openqa_fedora_tools/log/?h=live
on the openQA side there's all the expected work to add new needles and test cases and modify existing ones where appropriate to handle both live and non-live cases - quite a lot of change and this will get very long if I try to summarize it, so just poke me with questions about any specific bits that aren't obvious. I might go back through and sprinkle some comment-fu on there today.
On the fedora_tools side various tweaks were needed. image downloading is tweaked so the filenames for different images should always be unique (we can't use sub-directories, I asked :<).
One of the most awkward bits was making sure we run all the tests that aren't particularly image-specific *exactly once* for all composes - not zero times, and not twice or more. This is problematic because nightly composes have a 'generic boot.iso' image (but no 'server boot.iso' or 'server DVD' or 'server netinst' or anything), while TC/RC composes have a 'server boot.iso', 'server DVD', and 'server netinst' (which I think is always 100% identical to 'server boot.iso', but fedfind has to consider them two separate things, really), but no 'generic boot.iso'.
So I introduced a new openQA flavor called 'universal', and added a bit of logic to openqa_trigger which makes it effectively 'nominate' one of the images from the compose it's running on as the one that will have the 'universal' tests run against it. It *also* then schedules every image downloaded - including the one nominated as 'universal' - under its 'natural' flavor name (which is now payload_imagetype). To match this, on the openQA side, all the tests which can run with any non-live image are now associated with the 'universal' flavor/product, and only image-specific tests (which currently means just 'default boot and install') are associated with the image-specific flavors/products.
The upshot of that is that when you schedule a run against a compose you get most of the tests run just once against one of the non-live images, and you also get one instance of default_boot_install per image, which is I think exactly what we want.
The other cute hack I added is a way for the result reporting stuff to be able to figure out how to report the default_boot_install results to the wiki correctly. I added an optional key for the per-testcase dicts in the TESTCASES dict-of-dicts; if a testcase has a 'name_cb' key, its value should be a callback function which will provide the correct testcase name when called with the openQA job's 'flavor' as the sole argument. report_job_results.py checks for the callback and calls it if it's there, and uses the return value as the --testcase parameter it passes to relval. Aaaand that results in us reporting the result against the correct 'test instance' (row in the results table) for the image the test was run against. SIMPLES! Actually I kinda like that approach, and the general idea should be extensible in other cases where we need to do something like this. (We'll probably need a section callback for the Desktop page, for instance.)
anyhow, it's a tad tricky to test this with just a single run ATM because we have netinsts (but no Workstation lives) for F22 composes and Workstation lives (but no netinsts) for Rawhide, but I tested it out quite a bit with various different composes and it seems to be working pretty well. You can of course see all the various test runs at https://openqa.happyassassin.net .
Adam,
please set these up for review in Phabricator. I strongly suspect (given the time that I spent looking at the changes so far) that some discussion will be required, and Phab is _the_ place to do it. Also, please make sure to rebase your repos to their current state, before creating the phab reviews.
For further development, I'd suggest creating an account on Bitbucket, and using the "core" repos - all the FedoraQA Devs can write to the repos, and all the "admins" and administer it. Once you have the accound, I'll add you to the Dev group, and having a "feature" branch in the core repo seems quite better, given the development work-flows we currently adhere to.
Thanks,
joza
Some preliminary feedback:
= openqa_fedora =
== _do_install_and_reboot.pm ==
Please delete the "anaconda_install_finish" needle, if it is unused.
anaconda_install_done needle: * Why is only a part of the button selected? * 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?
== 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...) * merge _live_run_anaconda.pm and _anaconda_select_language.pm into one file, and call it something like "_get_to_anaconda_main_hub.pm"
This will keep the idea of having things split (so the "unless Kickstart" clause is just in one place), and will join the pieces, that IMHO should be together anyway.
== Needle changes ==
=== 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.
=== 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.
=== 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?
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".
= openqa_fedora_tools =
== conf_test_suites.py ==
I'm fairly certain that both default_install and package_set_minimal cover QA:Testcase_install_to_VirtIO.
== 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.
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`.
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).
I hope I'm not being too harsh, it is most certainly not my intent to come around that way,
J.
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!
On 2015-03-12 02:50, Josef Skladanka wrote:
Adam,
please set these up for review in Phabricator. I strongly suspect (given the time that I spent looking at the changes so far) that some discussion will be required, and Phab is _the_ place to do it. Also, please make sure to rebase your repos to their current state, before creating the phab reviews.
For further development, I'd suggest creating an account on Bitbucket, and using the "core" repos - all the FedoraQA Devs can write to the repos, and all the "admins" and administer it. Once you have the accound, I'll add you to the Dev group, and having a "feature" branch in the core repo seems quite better, given the development work-flows we currently adhere to.
Sure, will do. IIRC, when I sent my last round, you didn't have the openQA stuff set up for review in Phab so you asked for email, so I just figured to carry on that way.
On Thu, 2015-03-12 at 07:57 -0700, Adam Williamson wrote:
On 2015-03-12 02:50, Josef Skladanka wrote:
Adam,
please set these up for review in Phabricator. I strongly suspect (given the time that I spent looking at the changes so far) that some discussion will be required, and Phab is _the_ place to do it. Also, please make sure to rebase your repos to their current state, before creating the phab reviews.
For further development, I'd suggest creating an account on Bitbucket, and using the "core" repos - all the FedoraQA Devs can write to the repos, and all the "admins" and administer it. Once you have the accound, I'll add you to the Dev group, and having a "feature" branch in the core repo seems quite better, given the development work-flows we currently adhere to.
Sure, will do. IIRC, when I sent my last round, you didn't have the openQA stuff set up for review in Phab so you asked for email, so I just figured to carry on that way.
I've now pushed my 'live' branches into Bitbucket. Haven't yet filed review requests, I want to look over the 'check_condition' stuff carefully first, but I've addressed all the other review notes, I think.
On Thu, 2015-03-12 at 08:34 -0400, Josef Skladanka wrote:
== 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.
So after coming back to this I didn't really like it either, and even with your cleanups it seemed a bit fiddly. So instead I just threw it away and came up with a new approach. I'm writing this from a bus so I haven't tested it yet - if there are any stupid thinkos in the code feel free to go ahead and commit a fix.
This way winds up being about the same line count, but it's a lot clearer (I think) what's going on, and it should be fairly easy to extend in future when we inevitably wind up with different images for testing or whatever.
I've pushed the new approach out to the live branch (one big commit then two follow-ups to fix thinkos I spotted myself :>), let me know if it looks better to you.
qa-devel@lists.fedoraproject.org
-
Adam Williamson -
Josef Skladanka