GNOME Bugzilla – Bug 744852
test sending system key combos to guest
Last modified: 2016-03-31 13:22:07 UTC
new test for system key combos
Created attachment 297390 [details] [review] tests: capture system key combos Capture all 4 supported keycombos Ctrl+Alt+{Backspace, F1, F2, F7} via showkey utility. Succesfull capture is indicated by network downing and VM unpingable.
Review of attachment 297390 [details] [review]: * Please put empty lines before paragraphs. :) * Could you please explain whats going on with package installations in this patch?
Review of attachment 297390 [details] [review]: * Could you please explain whats going on with package installations in this patch? There is a package system in core linux so these installs just pull in (tce-load) some dependencies like that showkey (key press recorder) and curl as base system is really a core only.
Review of attachment 297390 [details] [review]: ::: tests/steps/general.py @@ +69,3 @@ +@step('Install "{pkg}" package and wait "{time}" seconds') +def install_package(context, pkg, time): This is very specific to tc linux so i'd expect name to reflect that. @@ +215,3 @@ + +@step('Verify recorded signals') +def verify_signals(context): The name and arguments make this sound like a very generic function but seems it is not. @@ +221,3 @@ + typeText("wget fpaste.org/187744/37188914/raw\n") + sleep(2) + typeText("sh raw\n") So you first downlaod a script that only downloads another script and than you run that? Why the very indirect approach and why on two different locations?
Review of attachment 297390 [details] [review]: ::: tests/steps/general.py @@ +221,3 @@ + typeText("wget fpaste.org/187744/37188914/raw\n") + sleep(2) + typeText("sh raw\n") Oh and why does one use wget and the other use curl? If wget is readily available on tclinux, I'd just use that and then we don't need to install curl.
Review of attachment 297390 [details] [review]: there are several reason for this very indirect approach 1. bug in typeText not being able to write shifted chars like > into running VMs (not sure if a11y, dogtail, or gnome-boxes bug) 2. wget is translated without ssl support so no chance to use it on https pages (like fedorapeople.org) 3. fpaste using some weird line breaks that I was unable to get rid of 1. this can be solved by using different tool xdotool but that would bring another dependency. Now I instead use very simple commands like wget fpaste, sh raw, etc 2. If we have some location that we can store files to w/o need of ssl then we may get rid of curl. not sure if anonymous ftp would help 3. I am just using this script location and single line fpastes I will change these names.
Review of attachment 297390 [details] [review]: Well I'd be fine with adding a small dep (at least on fedora the xdotool is mere 51k download) over writing spaghetti code (no offence, I understand your reason to write it this way).
Created attachment 301832 [details] [review] tests: capture system key combos Capture all 4 supported keycombos Ctrl+Alt+{Backspace, F1, F2, F7} via showkey utility. Succesfull capture is indicated by network downing and VM unpingable.
Created attachment 301833 [details] [review] tests: capture system key combos Capture all 4 supported keycombos Ctrl+Alt+{Backspace, F1, F2, F7} via showkey utility. Succesfull capture is indicated by network downing and VM unpingable.
Created attachment 301837 [details] [review] tests: capture system key combos Capture all 4 supported keycombos Ctrl+Alt+{Backspace, F1, F2, F7} via showkey utility. Succesfull capture is indicated by network downing and VM unpingable.
Created attachment 301838 [details] [review] tests: capture system key combos Capture all 4 supported keycombos Ctrl+Alt+{Backspace, F1, F2, F7} via showkey utility. Succesfull capture is indicated by network downing and VM unpingable.
Review of attachment 301838 [details] [review]: * Empty line before a new paragraph please. * I don't think "downing" is a verb. "network going down" would be correct. * Not all of those shortcuts leads to network down and/or VM unpingable and thats what the log seems to be saying. * You probably also want to tell that you are adding xdotool dep and why in the log. ::: tests/environment.py @@ +169,3 @@ def before_tag(context, tag): + if 'send_keycodes' in tag: + if not os.path.isfile('/usr/bin/xdotool'): I don't think this needs to be optional really. Its fine to require a bunch of really small packages for tests only. @@ -220,3 @@ os.system('virsh -q -c qemu:///system undefine Core-5.3-2 > /dev/null 2>&1') - unrelated change. ::: tests/general.feature @@ +131,3 @@ + * Hit "Enter" + * Save IP for machine "Core-5" + * Install TC Linux package "distro.ibiblio.org/tinycorelinux/3.x/tcz/showkey.tcz" and wait "1" seconds Does this fit 120 chars? If not, i'd find a way to divide this a bit. @@ +144,3 @@ + * Focus VM in Boxes + Then Verify previously recorded signals + Then Cannot ping "Core-5" Why wouldn't you be able to ping the box after putting it on 7th VT and then awaiting 10 seconds? ::: tests/steps/general.py @@ +54,3 @@ +@step('Focus VM in Boxes') +def focus_vm(context): Lets add this in a separate patch. @@ +77,3 @@ + +@step('Install TC Linux package "{pkg}" and wait "{time}" seconds') +def install_tc_linux_package(context, pkg, time): Should also go into a separate patch. @@ +79,3 @@ +def install_tc_linux_package(context, pkg, time): + if "/" in pkg: + call("xdotool type --delay 50 'wget %s\n'" %pkg, shell=True) Would be nice it add a comment here telling why we are using xdotool. Also please don't forget to add the new dep to README. @@ -183,3 @@ -@step('Start box name "{box}"') -def start_boxes_via_vm(context, box): Why is this disapearing? ::: tests/steps/utils.py @@ +80,3 @@ +@step('XType "{text}"') +def xdo_text(context, text): I'd put this in separate patch. Typically, the small the patches the better. @@ +82,3 @@ +def xdo_text(context, text): + call("xdotool type --delay 50 '%s'" %text, shell=True) + call("xdotool key 'Return'", shell=True) Why is 'return' always needed?
Ping! I'm hoping you intend to improve your patches and/or answer my questions from review at some point soon?
Review of attachment 301838 [details] [review]: ::: tests/environment.py @@ -220,3 @@ os.system('virsh -q -c qemu:///system undefine Core-5.3-2 > /dev/null 2>&1') - will remove ::: tests/general.feature @@ +131,3 @@ + * Hit "Enter" + * Save IP for machine "Core-5" + * Install TC Linux package "distro.ibiblio.org/tinycorelinux/3.x/tcz/showkey.tcz" and wait "1" seconds yes, it fits it has 107 @@ +144,3 @@ + * Focus VM in Boxes + Then Verify previously recorded signals + Then Cannot ping "Core-5" as I've explained in log verify previously recorded signals put network down in case of success, maybe it can be done inside verification step directly ::: tests/steps/general.py @@ +54,3 @@ +@step('Focus VM in Boxes') +def focus_vm(context): why, oh why? @@ +79,3 @@ +def install_tc_linux_package(context, pkg, time): + if "/" in pkg: + call("xdotool type --delay 50 'wget %s\n'" %pkg, shell=True) will add readme section, but in some cases xdotool may be unavailable and thus exit 77 and skip would be behavior, right? @@ -183,3 @@ -@step('Start box name "{box}"') -def start_boxes_via_vm(context, box): ah, an error, will fix this ::: tests/steps/utils.py @@ +80,3 @@ +@step('XType "{text}"') +def xdo_text(context, text): I know I know, but there is no other consumer of this @@ +82,3 @@ +def xdo_text(context, text): + call("xdotool type --delay 50 '%s'" %text, shell=True) + call("xdotool key 'Return'", shell=True) return writes enter to send command into the wild. It's the same as in normal typeText with pressKey(Enter) above
Created attachment 304144 [details] [review] tests: Add Focus VM in Boxes step Step for refocusing drawing area of VM after pressing some key combos in UI.
Created attachment 304145 [details] [review] tests: Add xdotool powered XType step XType tool uses xdotool to enter characters into VM drawing area. It works far better for shifted chars than dogtail.
Created attachment 304146 [details] [review] tests: Add xdotool powered XType step XType step uses xdotool to enter characters into VM drawing area. It works far better for shifted chars than dogtail.
Created attachment 304150 [details] [review] tests: Add Install TC Linux package step TC Linux has a simple package manager for additional package installations.
Review of attachment 301838 [details] [review]: ::: tests/steps/general.py @@ +54,3 @@ +@step('Focus VM in Boxes') +def focus_vm(context): Cause this is a generic step that could easily be useful for other steps/tests in future. I think I already explained why "every logical change in separate patch" is almost always a good idea on IRC quite a few times but for the record, I'll leave this Coala documentation about it here, written by my ex-student who learnt to love my insistence on commits being atomic and all. :) http://coala.readthedocs.org/en/latest/Getting_Involved/Writing_Good_Commits/ @@ +79,3 @@ +def install_tc_linux_package(context, pkg, time): + if "/" in pkg: + call("xdotool type --delay 50 'wget %s\n'" %pkg, shell=True) Unless there is a good reason not to (e.g not easily available in a major distro), I'll require it. ::: tests/steps/utils.py @@ +80,3 @@ +@step('XType "{text}"') +def xdo_text(context, text): for now but seems very generic to me to be very likely to be useful to other steps in future. BTW, could we also change the name to something more descriptive? I think name should reflect more of what the step and function does rather than what tech it uses to achieve that. @@ +82,3 @@ +def xdo_text(context, text): + call("xdotool type --delay 50 '%s'" %text, shell=True) + call("xdotool key 'Return'", shell=True) Ah so this function always takes commands and not just type text? If its meant specifically for that, I'd name it such that it reflects that.
Created attachment 304246 [details] [review] tests: Add Type text and return step This new step uses xdotool to write text into VM drawing area. It works far better for shifted chars than dogtail. Step presses return for command execution too.
Created attachment 304343 [details] [review] tests: Add "Send keycodes" test Boxes are now able to send keycodes into VMs. Test uses showkey utility executed inside the VM to record signals. Network is turned down if all signals were recorded as expected so pass/fail state can be recognized from outside of the VM.
Review of attachment 304144 [details] [review]: Good apart from these nits: * As said on IRC, when quoting something, please make use of "". In this case: tests: Add "Focus VM in Boxes" step. * I'll drop * "in Boxes" from not just log but step itself, its implied. * "after pressing some key combos in UI" since it can be used in other contexts too.
Review of attachment 304246 [details] [review]: * Same here: Please quote the quoted text in shortlog. * Since you decided to keep it generic (not just for command execution), I'll not say 'return' is pressed for command execution. How about: "This new step uses xdotool to write specified text into VM drawing area, and presses return. It works far better for shifted chars than dogtail and is very handy to execute commands in the VM." ?
Review of attachment 304343 [details] [review]: Just one minor nit and a question: * Two lines in description part of log seem to be shifted by a char. * Is "keycode" really a well-known term for this? Wikipedia says a different story and that is for "key code" not "keycode" http://en.wikipedia.org/wiki/Key_code Wonder if we should just continue (that's what I've been using in the git log) using the term "Key combos" instead?
Review of attachment 304343 [details] [review]: yeah, these two lines have probably spaces at the beginning. anyway, keycode is term from showkey utility that is used: -k --keycodes display only the interpreted keycodes (default)
Created attachment 304351 [details] [review] tests: Add "Focus VM" step Step for refocusing drawing area of VM after pressing some key combos in UI.
Created attachment 304352 [details] [review] tests: Add "Type text and return" step This step uses xdotool to write specified text into VM drawing area, and presses return. It works far better for shifted chars than dogtail and is very handy to execute commands in the VM.
Created attachment 304353 [details] [review] tests: Add "Install TC Linux package" step Step using TC Linux's simple package manager for additional package installations.
Review of attachment 304351 [details] [review]: 2nd point from prev review got missed :) * I'll drop * .. * "after pressing some key combos in UI" since it can be used in other
Review of attachment 304352 [details] [review]: ack
Review of attachment 304353 [details] [review]: ack
Created attachment 304377 [details] [review] tests: Add "Send key combos" test Boxes are now able to send keycodes into VMs. Test uses showkey utility executed inside the VM to record signals. Network is turned down if all signals were recorded as expected so pass/fail state can be recognized from outside of the VM.
Created attachment 304378 [details] [review] tests: Add "Focus VM" step Step for refocusing drawing area of VM.
Created attachment 304379 [details] [review] tests: Add "Send key combos" test Boxes are now able to send key combos into VMs. Test uses showkey utility executed inside the VM to record signals. Network is turned down if all signals were recorded as expected so pass/fail state can be recognized from outside of the VM.
(In reply to vladimir benes from comment #25) > Review of attachment 304343 [details] [review] [review]: > > yeah, these two lines have probably spaces at the beginning. > > anyway, keycode is term from showkey utility that is used: > -k --keycodes display only the interpreted keycodes (default) uhm, you're right key combos and keycodes are something different. Keycodes are codes that can be recorded by showkey utility but key combos are set of keys that are sent via Boxes to Vms. This one was tough, but I think we are all set to merge it :-) thanks for all your reviews!
Review of attachment 304378 [details] [review]: Add "Add" to description and its all good. :) I can do that when mering..
Review of attachment 304379 [details] [review]: "Boxes are" -> "Boxes is" when referring to the app rather than the things it manages. :) ::: tests/environment.py @@ +169,3 @@ def before_tag(context, tag): + if 'send_keycombos' in tag: + if not os.path.isfile('/usr/bin/xdotool'): Wasn't the conclusion from our IRC discussion that there is no reason for conditional dep here?
Created attachment 304431 [details] [review] tests: Add "Install TC Linux package" step Add step using TC Linux's simple package manager for additional package installations.
Created attachment 304432 [details] [review] tests: Add "Focus VM" step Add step for refocusing drawing area of VM.
Created attachment 304433 [details] [review] tests: Add "Send key combos" test Boxes app is now able to send key combos into VMs. Test uses showkey utility executed inside the VM to record signals. Network is turned down if all signals were recorded as expected so pass/fail state can be recognized from outside of the VM.
Review of attachment 304431 [details] [review]: ack
Review of attachment 304432 [details] [review]: ack
Review of attachment 304433 [details] [review]: Looks good otherwise. ::: tests/README @@ +82,3 @@ });" > /etc/polkit-1/rules.d/51-virtmgr.rules +* for send key combos test execution send -> sending
Review of attachment 304352 [details] [review]: Wait a sec, this step doesn't seem to be used by any of the patches in this bug. Could it be that it belongs to bug#748006?
(In reply to Zeeshan Ali (Khattak) from comment #43) > Review of attachment 304433 [details] [review] [review]: > > Looks good otherwise. > > ::: tests/README > @@ +82,3 @@ > });" > /etc/polkit-1/rules.d/51-virtmgr.rules > > +* for send key combos test execution > > send -> sending the test's name is send key combos so it's quite correct to name it the same way right?
(In reply to Zeeshan Ali (Khattak) from comment #44) > Review of attachment 304352 [details] [review] [review]: > > Wait a sec, this step doesn't seem to be used by any of the patches in this > bug. Could it be that it belongs to bug#748006? moved to 748006
(In reply to vladimir benes from comment #45) > (In reply to Zeeshan Ali (Khattak) from comment #43) > > Review of attachment 304433 [details] [review] [review] [review]: > > > > Looks good otherwise. > > > > ::: tests/README > > @@ +82,3 @@ > > });" > /etc/polkit-1/rules.d/51-virtmgr.rules > > > > +* for send key combos test execution > > > > send -> sending > > the test's name is send key combos so it's quite correct to name it the same > way right? Oh you are quoting, then "" please. :)
Created attachment 304941 [details] [review] tests: Add "Send key combos" test Boxes app is now able to send key combos into VMs. Test uses showkey utility executed inside the VM to record signals. Network is turned down if all signals were recorded as expected so pass/fail state can be recognized from outside of the VM.
Pushed with small changes. Attachment 304431 [details] pushed as 60a7b34 - tests: Add "Install TC Linux package" step Attachment 304432 [details] pushed as ebe1b12 - tests: Add "Focus VM" step Attachment 304941 [details] pushed as 84ebba8 - tests: Add "Send key combos" test