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. _______________________________________________ qa-devel mailing list qa-devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/qa-devel
