GNOME Bugzilla – Bug 736288
Add automated tests (3.14)
Last modified: 2016-09-20 08:15:55 UTC
Created attachment 285686 [details] initial tests Following code contains tests for basic boxes functionality. It doesn't contain express install tests, usb redirect and display tests. It does contain snapshot and multi window new functionality tests. Tests are written in python and are verifying various actions in boxes using dogtail [1] and behave [2] he tests are split into two parts: 1) Scenarios in *.feature files using gherkin [3] language to describe steps to be performed. This scenarios are also useful for manual testing 2) Step definitions for dogtail in steps/*.py files. These are dogtail instructions to operate on running program Prerequisites: 1. install python-behave, libvirt, wget, virt-install 2. add polkit rule to allow wheel user to operate system broker w/o password echo "polkit.addRule(function(action, subject) { if(action.id == \"org.libvirt.unix.manage\" && subject.isInGroup(\"wheel\")) { return polkit.Result.YES; } });" > /etc/polkit-1/rules.d/50-virtmgr.rules How to run those tests: 1.run: behave from tests directory to run all tests 2 run behave -t @test_name -k to run singe test) Coverage: see [4] [1] http://fedorahosted.org/dogtail [2] https://pypi.python.org/pypi/behave [3] https://github.com/cucumber/cucumber/wiki/Gherkin [4] http://fpaste.org/131964/11829141/
Thanks so much for starting this Vladimir! I'd really prefer that you integrate these tests to Boxes source tree from the start so that we can run them as part of some makefile target (or however other apps are doing it in GNOME) and provide patches so others can insert inline comments.
Created attachment 286087 [details] [review] tests + infrastructure bits
these tests require a few things in CI: 0. have boxes there :-) 1. install several packages: libvirt libvirt-client virt-install python-behave wget tigervnc-server 2. make isos available locally (now downloaded from ibiblio and disk images from dropbox) 3. add polkit rule as listed above
Review of attachment 286087 [details] [review]: (In reply to comment #2) > Created an attachment (id=286087) [details] [review] > tests + infrastructure bits For starter, we need this divided into multiple patches and with better (more detailed) commit messages: https://wiki.gnome.org/Git/CommitMessages . We'd also want bug references in the commit messages. git-bz is a tool that would make attaching patches to bz and updating their status much simpler (don't forget the usefulness of -e flag to git-bz attach).
(In reply to comment #0) > Created an attachment (id=285686) [details] > initial tests > Prerequisites: > 1. install python-behave, libvirt, wget, virt-install Why do we need virt-install? Tests should be making use of Boxes to create/install VMs. > 2. add polkit rule to allow wheel user to operate system broker w/o password > > echo "polkit.addRule(function(action, subject) { > if(action.id == \"org.libvirt.unix.manage\" && > subject.isInGroup(\"wheel\")) { > return polkit.Result.YES; > } > });" > /etc/polkit-1/rules.d/50-virtmgr.rules Why is this needed by the way? Boxes only connects to system libvirt through read-only connection so to avoid this.
Created attachment 289665 [details] Adding automated tests Behave/Dogtail set of automated tests. Tests are divided into several feature files that can be executed separately. Tests use 3 10 MB iso files that downlaoded at the beginning. More details to be found in README file.
(In reply to comment #5) > (In reply to comment #0) > > Created an attachment (id=285686) [details] [details] > > initial tests > > Prerequisites: > > 1. install python-behave, libvirt, wget, virt-install > > Why do we need virt-install? Tests should be making use of Boxes to > create/install VMs. > if you test system broker operations, you need virt-install to create some boxes inside of system broker. > > 2. add polkit rule to allow wheel user to operate system broker w/o password > > > > echo "polkit.addRule(function(action, subject) { > > if(action.id == \"org.libvirt.unix.manage\" && > > subject.isInGroup(\"wheel\")) { > > return polkit.Result.YES; > > } > > });" > /etc/polkit-1/rules.d/50-virtmgr.rules > > Why is this needed by the way? Boxes only connects to system libvirt through > read-only connection so to avoid this. in Fedora? no I don't think so. I am using this to avoid writing password anytime I need to access system broker. Otherwise I don't need it. I wrote more details into README so please look into there if you have more questions. I used git-bz for attaching patch. Sorry this is my first time ;-)
(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #0) > > > Created an attachment (id=285686) [details] [details] [details] > > > initial tests > > > Prerequisites: > > > 1. install python-behave, libvirt, wget, virt-install > > > > Why do we need virt-install? Tests should be making use of Boxes to > > create/install VMs. > > > > if you test system broker operations, you need virt-install to create some > boxes inside of system broker. I don't think we should be doing that. The test(s) should instead check if there are VMs on system brokder and if so, run the test(s). Otherwise, these tests should just be omitted. > > > 2. add polkit rule to allow wheel user to operate system broker w/o password > > > > > > echo "polkit.addRule(function(action, subject) { > > > if(action.id == \"org.libvirt.unix.manage\" && > > > subject.isInGroup(\"wheel\")) { > > > return polkit.Result.YES; > > > } > > > });" > /etc/polkit-1/rules.d/50-virtmgr.rules > > > > Why is this needed by the way? Boxes only connects to system libvirt through > > read-only connection so to avoid this. > > in Fedora? no I don't think so. I am using this to avoid writing password > anytime I need to access system broker. Otherwise I don't need it. Hmm.. yeah, for the actual import, its needed indeed. Similarly, I think this test should check if user can connect to system libvirtd w/o password and only run if that is so. Test system should not be mandling with the environment nor it should require it, especially admin-level stuff. > I wrote more details into README so please look into there if you have more > questions. I used git-bz for attaching patch. Sorry this is my first time ;-) I will have a look. Thanks for using git-bz, it'll help you too in the long run.
Review of attachment 289665 [details]: Firstly, you didn't act on one of the first things I said during last review: Dividing the patch. :( Kudos for good commit log and also all the details in the README. One tiny nitpick about shortlog though: We like verbs in imperative form (like most projects) in shortlog. so 'Adding' -> 'Add'. Don't get me wrong, I'm very grateful for your work so far. I must admit that its been a while since I last looked at any Python code and I'm not yet familiar with how behave or dogtail work exactly so besides other known benefits, dividing this into several patches would help me out in deciphering what is going on in python and feature files too and then I can review them. In the meantime, I must look into behave documentation. :) ::: tests/README @@ +55,3 @@ + * behave (python-behave in Fedora) + * dogtail + * wget (can be replaced by python urlib) We shouldn't require this cause I think tests shouldn't download anything. Instead tests should check for known ISOs to be in some particular location and be omitted if required ISOs/medias are not found. @@ +58,3 @@ + +* for vnc tests execution + * tigervnc-server (or similar) Same goes for this, shouldn't be a hard dep. @@ +61,3 @@ + +* for system-broker test execution + * virt-install As I said in the previous comment on the bug, we shouldn't require this either. @@ +68,3 @@ + return polkit.Result.YES; + } + });" > /etc/polkit-1/rules.d/51-virtmgr.rules same here. We shouldn't require this. @@ +82,3 @@ + + * behave -i $category.feature + to run whole feature file Cool :) but i think these should be hooked to make rules.
pushing our private conversation back to the wild: On Tue, 2014-11-04 at 15:09 +0000, Zeeshan Ali wrote: On Tue, 2014-11-04 at 00:57 +0100, Vladimir Benes wrote: > > Hey Zeenix, > > Hi Vladimir, > > > I am slowly working on yet another (hopefully final) version of > > patches. Will wait for you suggestions > > Cool. I would have instead liked your inline replies to my comments. The > discussion would have all be in one place in upstream then. > > ok ok, I plan to move it back but we need to clarify several things > what I did: > > 1. pylint clean ups > > 2. exchanged wget for urllib2 for downloading isos and disk images > > Good but whatever happened to my suggestion of tests not downloading > ISOs? I'm not saying its a great suggestion and its likely we need to do > this (at least for now) but would have been nice to address it still. > > we can bundle isos later on with gnome-boxes-tests rpm for fedora or so. I would suggest to place them into gnome infrastructure now. > 4. fixes for robustness and speedup (checking for IP assigned to > > machine by dnsmasq instead of waiting 30 secs to consider machine > > installed, this can drop installation to ~15 secs) > > I'm not sure I got it. How is IP assignment a sign of installation > completion? We show a 'installing..' label under the box in the UI and > we declare the installation status in the custom xml in the domain > configuration so would be nice to look for changes in either of those > instead. > For installation it;'s probably good to use your approaches (installing is not accessible but xml should work) Just to explain this a bit: I am not speaking about installation but booting. I am checking correct vm behavior by upping and downing network and ping. So thus I need to know it's IP. I have to deal with slow low ram machines but also with quick ones. so now once new IP entry is in dnsmasq record I know that machine is up. example: @poweroff_local_livecd_box Scenario: Go into local liveCD box * Create new box "Core-5" << -- exiting once I have IP record (stored together with machine name in dictionary for later usage) * Box "Core-5" "does" exist * Ping "Core-5" <<-- I need to have IP * Go into "Core-5" box * Wait for "sleep 1" end * Type "sudo poweroff" * Wait for "sleep 20" end Then Box "Core-5" "does not" exist Then Cannot ping "Core-5" <<-- I need to have IP > > 5. moved tests around a bit so failing deps like vncserver binary or > > passwordless libvirt access fails just affected scenario > > Cool. > > > 6. tests are not defined as tests but group wise (aka feature) so they > > can be somehow skipped if needed (I have 8 groups: general, spice, > > livecd, import_vms, system_broker, multi-window, snapshots, spice) > > Sounds really good. > I tested that 77 exit code and it works, so I removed all checks and left them only in README > > what I need from you: > > 1. upload ~40 MB images to some local storage .. would ~teuf or your > > place in people.gnome.org work for it? I don't have my own. > > teuf if Christophe, not me. :) I have my space there too and yes I can > upload images there. Do we need special ISOs to solve the issue of net > ISOs not working out of the box? If so, I think we should instead: > > I know your nicks ;-) would ~teuf or your place translates into would Christophe's or Zeeshan's place. I just saw some boxes stuff in teuf's dir and some more in yours so don't know which one should be used. Anyway, for stripping down network use I would suggest to use 3rd party. I am using 10 MB iso of CoreLinux. http://distro.ibiblio.org/tinycorelinux/5.x/x86/release/ express installation is not covered at all. This is Chitra's self-realization area :-) 1. Fix netiso support (if possible) in libosinfo. I'm looking into > that.. > 2. Not use hacked ISOs but instead use the ones that work. > > I'm hoping tests don't download ISOs if they are already > downloaded/cached? > > yes of course, vmdk and qcow and one iso are downloaded just once, prior all tests. it's 3x10 so 30 MB not 40. Mea culpa. > 2. I need to suggest different approach or accept my system broker > > requirements proposal. > > > > a. package dependency to virt-install and libvirt.. these pull in > > python-libvirt and virt-manager-common > > this is needed for system broker tests.. as I am connecting to > > system broker with installed machine > > I understood that but I already suggested to you to make the tests > dynamic, i-e don't setup VMs on system but rather check if some machines > exists on system libvirt and skip the tests if no machine exists. > > no, I am strictly against this approach. You have to prepare environment for each test and clean afterwards. That's how QE works. I can imagine one time installation ( I am all the time speaking about installation but I use live isos so it's more or less starting only) to speed things a bit prior all system broker tests. But doing something dynamically for delete for example can break things around and we simply don't want to do that. > or checking for real-time system broker changes. > > Of the system being installed by test with virt-install? If you don't > install the system, I guess this isn't needed anymore then. > > I need to test that boxes are reflecting real-time changes. I need to start installation (livecd vm creation) after system broker was imported into boxes and boxes are running. So sadly, I still need virt-install. > b. passwordless access to org.libvirt.unix.manage for wheel group > > to perform above mentioned operations w/o knowing user's > > password. I don't see any other way how to install a machine into a > > system broker in generic environments. > > As I said I wrote the code so that it doesn't require elevated > privileges to connect to system libvirt. However, the issue is access to > disk images as I noted in the git log: > > ------------------- > commit: 680408327854fa9169f6d95dc989840b507d0194 > Allow user to import system libvirt VMs > > If there is no boxes defined and there is VMs available on system > libvirt, allow user to import those VMs to session libvirt (and > therefore > Boxes). > > Issues/TODO: > > * We gotta elevate privileges to change permissions on disks/images > as by > default they are not even readable to anyone but root/qemu. This > unfortunately means launching of undesirable polkit dialog. > > * The wizard summary in this case could use improvement/more info. > > * We don't give any option to user for deleting the source VMs from > system libvirt after import. If we do that, we'll have to trigger > yet > more polkit dialogs. :( Had a bit of discussion with jimmac about > this. > He suggested that perhaps we should track the imported VMs so that > we > only present option to import if there is something new to import. > > https://bugzilla.gnome.org/show_bug.cgi?id=666185 > --------------------- > > I need to think and research this more to come up with a good solution. > However, for now at least this test should be skipped if user doesn't > have access. > as I said before. If we doesn't have libvirtd running, missing virt-install or privileges.. exit 77 is done. > > 3. is it possible to skip tests somewhere in configure/Makefile if > > prerequisites are not met? E.g. I have no vncserver binary so skip (or > > WARN) vnc tests. No virt-install.. skip system_broker tests, etc > > The usual way that I know from my experience with check is that you > return a special value (77) from testcase to skip it. Could you check if > that applies to all tests with autotools? Found this related document: > > http://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html > > > Sorry to say this, but I don't plan to split test codes into more > > patches than two. Infrastructure bits and test code itself. > > Are you telling me that hour of discussion with Vadim, at the end of > which he agreed to help you divide them, was all for nothing? > > Can you please tell me exactly why you wouldn't do this? As I said to > Vadim, this is nothing exceptional I'm asking for. Check any free > software project out there that stresses on quality (linux kernel, > libvirt etc), and you'll find that this is required by every one of > them. > > Every new contributor has to go through this and its difficult for every > one of them who is new to refactoring/cleaning up patches in git so if > you actually want to become a contributor, you have to go through this. > Once you get used to it, its not that difficult and its an skill that > will help you in future. > > ok ok, long stroy short I did some splitting. Not test by test but feature wise. Now I have 8 commits for you almost ready. > I can give you some detailed introduction into several tests so you > > can see how are tests written. > > And to further ensure this is only visible to a few, you have put them > in a private mail. :( > > > But that's it. I would like to see code upstream so others can > > participate but after all it not essential for me. > > Its very sad to hear that you don't see the value of pushing these > upstream. I'll be pushing these tests anyway, with or without further > help from you. Would have been nice if you at least had cleanup your > existing patches and made the way for others to contribute further > tests. :( > I plan to contribute. Simply I didn't see a reason to split code into 50 patches. 8 seems to be pretty OK. obviously if that works for you, too :-)) > > OK let's do some example here. It may be valuable for Chitra as well. > > test case: > > > > Background: > > * Make sure that gnome-boxes is running > > * Wait until overview is loaded > > > > @new_vnc_localhost_box > > Scenario: New vnc box > > * Create new box from url "vnc://localhost:5901;" > > * Wait for "sleep 2" end > > * Press "Create" > > Then Box "localhost" "does" exist > > * Go into "localhost" box > > * Wait for "sleep 1" end > > * Hit "<Super_L>" > > * Wait for "sleep 1" end > > * Type "gnome-terminal" > > * Wait for "sleep 1" end > > * Type "echo 'walderon' > /tmp/vnc_text.txt" > > Then "walderon" is visible with command "cat /tmp/vnc_text.txt" > > Then Press "back" in vm > > > > so. the first thing that is called in test itself by behave is > > execution of environment.py. Behave defines several possible actions > > that differ according to execution time. The order (and names) of > > these actions is as follows: > > > > 0. before all <-executed prior every single test > > 1. before scenario <- executed prior name specified scenario > > 2. before tag <- executed prior tag (in this case tag = > > go_into_local_livecd_box) > > 3. after scenario > > 4. after tag > > 5 after all > > > > This environment is executed for every single test. > > Before all action for first time setup (downloading images, disabling > > screen saver, etc), then touching /tmp/boxes_counfigured and not > > repeating this again. App class is defined here via context.app_class > > = App('gnome-boxes') (App class comes from common_steps) > > > > Before scenario is not used in my tests at all. > > Before tag is used in this test (via vnc tag) for starting localhost > > vncserver. > > After step is not used except test.status is fail for log bundling. > > After tag is used for vncserver stopping > > After scenario is used deleting all possible boxes by selecting > > one-by-one and clicking delete .. and x next to undo. then boxes are > > terminated. Some more logging is done here and some sanity deletes > > too. Not sure if these deletes are still needed but they're there just > > to be sure env is clean. > > > > Note that even if tests are defined via features (like a bunch of > > tests) in Makefile.am they're executed one by one by beaker. So all > > this doesn't change even if features are used instead of tests. > > > > Behave then moves to vnc.feature file (via tag again): Feature files > > contain Scenarios (accompanied by tags for execution and > > setup/teardown -- scenario may have more tags) and scenarios contain > > steps that are defined in steps/steps.py which are very likely > > absorbing parameters to be as widely reusable as possible. > > > > Background: > > describes steps that are executed for every single test > > in this case > > * Make sure that gnome-boxes is running > > ^^ this goes back to common_steps and starts application itself > > * Wait until overview is loaded > > ^^ this waits until overview is loaded. In fact it waits for New > > button being visible. This step contains sub-step > > > > @new_vnc_localhost_box > > ^^ this defines tag for execution and via if 'vnc' in tag: in > > environment for start/stop of vnc server > > > > Scenario: New vnc box > > ^^ just human readable scenario name..not used anywhere > > > > * Create new box from url "vnc://localhost:5901;" > > ^^ here comes a bit of dogtail. Behave searches in steps.py for > > according step. In this case: > > > > @step(u'Create new box from url "{url}"') > > def create_new_vm_via_url(context, url): > > context.app.child('New').click() > > context.app.child('Continue').click() > > context.app.child('Enter URL').click() > > > > typeText(url) > > context.app.child('Continue').click() > > <snip> > > > > so it clicks New, then Continue and then Enter URL, then url (before > > mentioned step parameter) is typed in and Continue is clicked. Some > > more logic for http downloading is there but that's not important now. > > > > * Wait for "sleep 2" end > > ^^ waiting for two seconds > > * Press "Create" > > ^^ pressing create. Create is done separately as you may want to > > change some VM prefs before. Not applied here. > > After create, Boxes goes back to overview. > > > > Then Box "localhost" "does" exist > > ^^ Check that localhost box is present in overview > > > > * Go into "localhost" box > > ^^ localhost box is clicked > > > > * Wait for "sleep 1" end > > ^^ wait for 1 second to vnc screen to appear (sufficient while it's > > local, should be more if remote) > > > > * Hit "<Super_L>" > > ^^ thiis calls step that contains pressKey dogtail method with Super_L > > param. Thus overview is opened in vnc box as it should be focused > > after Go into "vm_name" step > > > > * Wait for "sleep 1" end > > ^^ Wait one more second for overview to appear > > > > * Type "gnome-terminal" > > ^^ this eneters gnome-terminal into overview search bar and hits enter > > > > * Wait for "sleep 10" end > > ^^ wait 10 more seconds (it has to be quite a lot as low memory > > machines are sllooooww) > > > > * Type "echo 'walderon' > /tmp/vnc_text.txt" > > ^^ now while focused into terminal write above mentioned echo into a > > file in /tmp > > > > Then "walderon" is visible with command "cat /tmp/vnc_text.txt" > > ^^ as the box is local, the file will be in /tmp dir with walderon > > inside. > > > > Then Press "back" in vm > > ^^ this just goes back to overview. > > > > then after tag and after scenario are executed, vncserver stopped and > > boxes deleted. > > > > So this is it. I hope you can now see how are these tests tight > > together. Press back in vm is used in every single test. Wait for > > "process" to finish too. The same for Go into "vm_name" box. And so on > > and so on. What would maybe make sense would be to split these <50 > > tests grouped in these 8 features to 8 commits. I can possibly do > > that. But splitting them into 50 commits is more or less crazy and not > > worth it. > > > > Hope this helps at least a bit. > > This helps *me* and Chitra (and tpelka and Vadim) a lot so thanks a lot > or writing them down but this would help a lot more people if you had > just put a day into dividing the patches and putting all these > details/rationales in the commit logs. > > > Zeeshan, please do answer these questions at the beginning. > > I hope I did. > > > new version of patch > > http://fpaste.org/147606/58510141/ > > > > after applying patch, configuring via ./autogen.sh > > --enable-installed-tests; make; sudo make install > > test can be executed via gnome-desktop-testing-runner > > Coo. Can't they be run as part of a makefile rule w/o installing? > Although not super important, it would help developers a lot. For now we > can add a rule that requires `install` rule to be run first and runs > gnome-desktop-testing-runner command. > > not sure how to do this, could you please contact me via IRC on gnome-hackers channel? I would like to clarify this before I will push patches to bz thanks, Vladimir
(In reply to comment #10) > pushing our private conversation back to the wild: Thanks! > > 4. fixes for robustness and speedup (checking for IP assigned to > > > machine by dnsmasq instead of waiting 30 secs to consider machine > > > installed, this can drop installation to ~15 secs) > > > > I'm not sure I got it. How is IP assignment a sign of installation > > completion? We show a 'installing..' label under the box in the UI and > > we declare the installation status in the custom xml in the domain > > configuration so would be nice to look for changes in either of those > > instead. > > > For installation it;'s probably good to use your approaches (installing is not > accessible but xml should work) > > Just to explain this a bit: I am not speaking about installation but booting. Ah ok. :) > I am checking correct vm behavior by upping and downing network and ping. So > thus I need to know it's IP. I have to deal with slow low ram machines but also > with quick ones. so now once new IP entry is in dnsmasq record I know that > machine is up. One question: Can we not just trust libvirt? you can listen to events from the domain and see if it goes up? That won't tell anything about successful boot though. > > 2. I need to suggest different approach or accept my system broker > > > requirements proposal. > > > > > > a. package dependency to virt-install and libvirt.. these pull in > > > python-libvirt and virt-manager-common > > > this is needed for system broker tests.. as I am connecting to > > > system broker with installed machine > > > > I understood that but I already suggested to you to make the tests > > dynamic, i-e don't setup VMs on system but rather check if some machines > > exists on system libvirt and skip the tests if no machine exists. > > > > > no, I am strictly against this approach. You have to prepare environment for > each test and clean afterwards. That's how QE works. I can imagine one time > installation ( I am all the time speaking about installation but I use live > isos so it's more or less starting only) to speed things a bit prior all system > broker tests. But doing something dynamically for delete for example can break > things around and we simply don't want to do that. OK. Thanks for explaining. > > Every new contributor has to go through this and its difficult for every > > one of them who is new to refactoring/cleaning up patches in git so if > > you actually want to become a contributor, you have to go through this. > > Once you get used to it, its not that difficult and its an skill that > > will help you in future. > > > > > ok ok, long stroy short I did some splitting. Not test by test but feature > wise. Now I have 8 commits for you almost ready. Really glad to hear. The point is not to split the hell out of them or making things hard for you but rather to: 1. Keep every logical change into a separate patch. 2. Try to keep every patch as small as possible. (1) almost always makes sense, however (2) isn't always possible or even doesn't make sense sometimes. E.g I asked Chitra yesterday to squash one of her patches into another one. Keeping that in mind, having patches divided by feature sounds good to me. > > I can give you some detailed introduction into several tests so you > > > can see how are tests written. > > > > And to further ensure this is only visible to a few, you have put them > > in a private mail. :( > > > > > But that's it. I would like to see code upstream so others can > > > participate but after all it not essential for me. > > > > Its very sad to hear that you don't see the value of pushing these > > upstream. I'll be pushing these tests anyway, with or without further > > help from you. Would have been nice if you at least had cleanup your > > existing patches and made the way for others to contribute further > > tests. :( > > > I plan to contribute. Simply I didn't see a reason to split code into 50 > patches. For the record, I never asked for 50 patches. :) > > Coo. Can't they be run as part of a makefile rule w/o installing? > > Although not super important, it would help developers a lot. For now we > > can add a rule that requires `install` rule to be run first and runs > > gnome-desktop-testing-runner command. > > > > > not sure how to do this, could you please contact me via IRC on gnome-hackers > channel? I would like to clarify this before I will push patches to bz As I said on IRC, I guess we can look into this after your patches are merged. Could you please put it somewhere on top of your todo list? :)
Created attachment 290437 [details] [review] Add automated tests and installed-tests infrastructure bits Behave/Dogtail set of automated tests. Tests use 3 10 MB iso files that are downloaded at the beginning. Infrastructure bits are also included so tests can be executed via gnome-desktop-testing-runner (after running ./autogen.sh --enable-installed-tests). Test can be execute one by one or in a batch. More details to be found in README file. Coverage in general section: start/stop help/credits download an iso customize memory start box search boxes
Created attachment 290438 [details] [review] Add automated tests New tests for adding vmdk and qcow2 VM images. These images are the same Tiny Core Linux LiveCD but installed and conserved. New needed steps for execution added into steps.py. Tests added into Makefile, too. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290439 [details] [review] Add automated tests LiveCD tests. Tiny Core 10 MB isos are used to avoid large downloads. Coverage: add new livecd from file and menu go into box delete/undo poweroff/pause/resume force shutdown Sometimes facing issue while box being deleted just after creation. This fails in Create new box from menu/file step. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290440 [details] [review] Add automated multi-windows tests Open one/several boxes in new windows. Power off in the window. Switching between windows was quite touch as boxes name widgets are not visible to dogtail until you go to preferences. So clicking through machines' preferences is done at the beginning. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290441 [details] [review] Add automated snapshots tests Create and revert snapshots, rename them, delete. Snapshots are tested via saving snapshot with working network then shutting it down from inside the box and reverting back to working state. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290442 [details] [review] Add automated snapshots tests Create and revert snapshots, rename them, delete. Snapshots are tested via saving snapshot with working network then shutting it down from inside the box and reverting back to working state. https://bugzilla.gnome.org/review?bug=736288
Sorry, need to do one more round, there were some mistakes I didn't realize :-(
Created attachment 290443 [details] [review] Add automated tests and installed-tests infrastructure bits Behave/Dogtail set of automated tests. Tests use 3 10 MB iso files that are downloaded at the beginning. Infrastructure bits are also included so tests can be executed via gnome-desktop-testing-runner (after running ./autogen.sh --enable-installed-tests). Test can be execute one by one or in a batch. More details about file structure and execution to be found in README file. Coverage in general section: start/stop help/credits download an iso customize memory start box search boxes https://bugzilla.gnome.org/review?bug=736288
Created attachment 290444 [details] [review] Add automated vmdk/qcow2 vm import tests New tests for adding vmdk and qcow2 VM images. These images are the same Tiny Core Linux LiveCD but installed and conserved. New needed steps for execution added into steps.py. Tests added into Makefile, too. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290445 [details] [review] Add automated LiveCD tests LiveCD tests. Tiny Core 10 MB isos are used to avoid large downloads. Coverage: add new livecd from file and menu go into box delete/undo poweroff/pause/resume force shutdown Sometimes facing issue while box being deleted just after creation. This fails in Create new box from menu/file step. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290446 [details] [review] Add automated multi-windows tests Open one/several boxes in new windows. Power off in the window. Switching between windows was quite touch as boxes name widgets are not visible to dogtail until you go to preferences. So clicking through machines' preferences is done at the beginning. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290447 [details] [review] Add automated snapshots tests Create and revert snapshots, rename them, delete. Snapshots are tested via saving snapshot with working network then shutting it down from inside the box and reverting back to working state. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290448 [details] [review] Add automated SPICE tests Spice is tested via localhost connection and network up/down adn ping from inside the machine to check if typing works. https://bugzilla.gnome.org/review?bug=736288
Created attachment 290449 [details] [review] Add automated system-broker tests These tests do have a few prerequsites: passwordless access to system-broker (see README for more details) virt-install binary running libvirtd When prerequsites are not met tests are skipped (via exit code 77) Coverage: system-broker.feature connect to system broker (SB) delete/undo machine from SB reflect delete/addition in SB pause/resume SB box force shutdown in SB restart persistence https://bugzilla.gnome.org/review?bug=736288
Created attachment 290450 [details] [review] Add Automated Vnc tests These tests have prerequisites for available vncserver binary. Otherwise skipped with exit(77). Let's do some test description here. It may be of value for other tests too.. test case: Background: * Make sure that gnome-boxes is running * Wait until overview is loaded @new_vnc_localhost_box Scenario: New vnc box * Create new box from url "vnc://localhost:5901;" * Wait for "sleep 2" end * Press "Create" Then Box "localhost" "does" exist * Go into "localhost" box * Wait for "sleep 1" end * Hit "<Super_L>" * Wait for "sleep 1" end * Type "gnome-terminal" * Wait for "sleep 1" end * Type "echo 'walderon' > /tmp/vnc_text.txt" Then "walderon" is visible with command "cat /tmp/vnc_text.txt" Then Press "back" in vm so. the first thing which is called in test itself by behave is execution of environment.py. Behave defines several possible actions that differ according to execution time. The order (and names) of these actions is as follows: 0. before all <-executed prior every single test 1. before scenario <- executed prior name specified scenario 2. before tag <- executed prior tag (in this case tag = go_into_local_livecd_box) 3. after scenario 4. after tag 5 after all This environment is executed for every single test. Before all action for first time setup (downloading images, disabling screen saver, etc), then touching /tmp/boxes_counfigured and not repeating this again. App class is defined here via context.app_class = App('gnome-boxes') (App class comes from common_steps) Before scenario is not used in my tests at all. Before tag is used in this test (via vnc tag) for starting localhost vncserver. After step is not used except test.status is fail for log bundling. After tag is used for vncserver stopping After scenario is used deleting all possible boxes by selecting one-by-one and clicking delete .. and x next to undo. then boxes are terminated. Some more logging is done here and some sanity deletes too. Not sure if these deletes are still needed but they're there just to be sure env is clean. Note that even if tests are defined via features (like a bunch of tests) in Makefile.am they're executed one by one by beaker. So all this doesn't change even if features are used instead of tests. Behave then moves to vnc.feature file (via tag again): Feature files contain Scenarios (accompanied by tags for execution and setup/teardown -- scenario may have more tags) and scenarios contain steps that are defined in steps/steps.py which are very likely absorbing parameters to be as widely reusable as possible. Background: describes steps that are executed for every single test in this case * Make sure that gnome-boxes is running ^^ this goes back to common_steps and starts application itself * Wait until overview is loaded ^^ this waits until overview is loaded. In fact it waits for New button being visible. This step contains sub-step @new_vnc_localhost_box ^^ this defines tag for execution and via if 'vnc' in tag: in environment for start/stop of vnc server Scenario: New vnc box ^^ just human readable scenario name..not used anywhere * Create new box from url "vnc://localhost:5901;" ^^ here comes a bit of dogtail. Behave searches in steps.py for according step. In this case: @step(u'Create new box from url "{url}"') def create_new_vm_via_url(context, url): context.app.child('New').click() context.app.child('Continue').click() context.app.child('Enter URL').click() typeText(url) context.app.child('Continue').click() <snip> so it clicks New, then Continue and then Enter URL, then url (before mentioned step parameter) is typed in and Continue is clicked. Some more logic for http downloading is there but that's not important now. * Wait for "sleep 2" end ^^ waiting for two seconds * Press "Create" ^^ pressing create. Create is done separately as you may want to change some VM prefs before. Not applied here. After create, Boxes goes back to overview. Then Box "localhost" "does" exist ^^ Check that localhost box is present in overview * Go into "localhost" box ^^ localhost box is clicked * Wait for "sleep 1" end ^^ wait for 1 second to vnc screen to appear (sufficient while it's local, should be more if remote) * Hit "<Super_L>" ^^ thiis calls step that contains pressKey dogtail method with Super_L param. Thus overview is opened in vnc box as it should be focused after Go into "vm_name" step * Wait for "sleep 1" end ^^ Wait one more second for overview to appear * Type "gnome-terminal" ^^ this eneters gnome-terminal into overview search bar and hits enter * Wait for "sleep 10" end ^^ wait 10 more seconds (it has to be quite a lot as low memory machines are sllooooww) * Type "echo 'walderon' > /tmp/vnc_text.txt" ^^ now while focused into terminal write above mentioned echo into a file in /tmp Then "walderon" is visible with command "cat /tmp/vnc_text.txt" ^^ as the box is local, the file will be in /tmp dir with walderon inside. Then Press "back" in vm ^^ this just goes back to overview. then after tag and after scenario are executed, vncserver stopped and boxes deleted. https://bugzilla.gnome.org/review?bug=736288
Review of attachment 290443 [details] [review]: * Kudos for providing lots of details in the commit logs. * Shortlogs are supposed to be very short (ideally < 50 chars). There are some tips here and even a checklist that I recommend always going through when submitting patches: https://wiki.gnome.org/Git/CommitMessages * On the same note, lines in description part of commit log are not supposed to exceed 74 chars. * Its customary and nice to add a prefix in front of bullet points, e.g "*" you see in here. * All patches introduce a lot of whitespace damage: Applying: Add automated tests and installed-tests infrastructure bits /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:213: trailing whitespace. Tests for GNOME 3 Boxes application. Tests are written in python using Behave [1] /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:241: trailing whitespace. /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:248: trailing whitespace. /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:249: trailing whitespace. * behave -t $test -k /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:553: trailing whitespace. warning: squelched 6 whitespace errors warning: 11 lines add whitespace errors. Add automated vmdk/qcow2 vm import tests .... ::: configure.ac @@ -56,3 @@ GTK_VNC_MIN_VERSION=0.4.4 LIBVIRT_GLIB_MIN_VERSION=0.1.9 -LIBVIRT_GCONFIG_MIN_VERSION=0.2.0 This is unrelated change and not even desirable. This is another advantage of dividing your patches into smaller pieces: Its harder to miss unrelated changes. @@ -218,3 @@ USB redirection support: $enable_usbredir Smartcard support: $enable_smartcard - oVirt support: $have_govirt Another unrelated and undesirable change. ::: tests/environment.py @@ +13,3 @@ +import sys + +def dlfile(url): Lets not use cryptic names. This is very much against our coding-style. While CodingStyle.txt was created for Vala code, most of it applies to python code too. @@ +24,3 @@ + local_file.write(f.read()) + + #handle errors This is what I'd call overdocumentation since it doesn't tell reader anything. Anyone who has even faintest idea about python would know immediately that following is an error handler. @@ +160,3 @@ + + # clean all boxes + #os.system("rm -f ~/.config/libvirt/qemu/boxes*") Please remove commented out code or add an explanation why its still present. I tend to prefer it not being there at all, usually just confuses the readers. I see commented out code in other places too, so same applies for them. ::: tests/steps/steps.py @@ +10,3 @@ + +def get_showing_node_name(name, parent, timeout=30, step=0.25): + slp = 0 same note about crytpic names and unknown abbreviations. @@ +196,3 @@ +@step(u'Press "{action}" in vm') +def press_back_in_vm(context, action): + panel = context.app.child('Boxes').children[0].findChildren(lambda x: x.roleName == 'panel' and x.showing)[0] Please don't exceed 120 chars as per our coding style.
Review of attachment 290443 [details] [review]: ::: configure.ac @@ -218,3 @@ USB redirection support: $enable_usbredir Smartcard support: $enable_smartcard - oVirt support: $have_govirt Actually it would be nice to add another line here: Installed tests: $enable_installed_tests
Review of attachment 290444 [details] [review]: Some of the comments on previous patch, apply to this one as well. > Tests added into Makefile, too. I don't think thats a detail worth mentioning in the log. ::: tests/README @@ -30,3 @@ * behave (python-behave in Fedora) * dogtail - Redundant/unrelated change. I think you want to merge it into previous patch instead.
Review of attachment 290445 [details] [review]: Same here: some of the comments on previous patches apply here too. ::: tests/steps/steps.py @@ +62,3 @@ assert call(cmd, shell=True) != 0, "Machine %s is pingable!" %vm +@step(u'Close warning') A new feature with so many new scenerios but only one more small step needed? It could be my novice eyes but is it possible that you put the needed steps in the wrong patch?
Review of attachment 290443 [details] [review]: ::: tests/steps/steps.py @@ +1,1 @@ +# -*- coding: UTF-8 -*- Could you please have a separate step file for each feature file? That would just be very systematic and keep things clean. Based on my limited experience with behave, this is very much possible.
Review of attachment 290446 [details] [review]: * Typo: touch -> tough. * I think you can remove "automated" part from all shortlogs to keep it short. Besides we don't have any other types of tests. * The details sounds like a bunch of orders. The details are supposed to describe the change in normal English. I think you want to use bullets here since you are simply listing the test case titles one after another. Something like: This adds tests for the following scenerios (following by bullets in new para). ::: Makefile.am @@ +76,3 @@ @BEHAVE_INSTALLED_TESTS_RULE@ +INSTALLED_TESTS= general.feature import-vms.feature livecd.feature \ + multi-windows.feature To keep things consistent, lets put each feature on a new line (this division could be part of this patch since they all fit nicely on one line before this). ::: tests/multi-windows.feature @@ +41,3 @@ + When Ping "Core-5 3" + * Focus "Core-5" window + * Type "sudo ifconfig eth0 down" Why do we test eth0 going down? ::: tests/steps/steps.py @@ +243,3 @@ + # for each window (aka box) + for box in boxes: + if box != context.app.children[0]: less indented => more readable (especially for large blocks of code) so we usually prefer the style: if box == context.app.children[0]: continue; # rest of the code @@ +244,3 @@ + for box in boxes: + if box != context.app.children[0]: + # define boxes New button * Are we really defining New button? Seems to me that we are just (finding it and) grabbing focus to it. * 'boxes' is quite redundant here. * On a related note please capitalize 'Boxes' when you are referring to the software to avoid confusion, unless you use the unix name: gnome-boxes. This is extreme nitpicking so don't take it too seriously. :) @@ +252,3 @@ + # if icon has text property equal to box name we've found it + if child.text == vm: + #click that icon which icon? This comment doesn't help really. :( Also please keep a space between # and the first letter, preferably capitalizing the first letter. @@ +260,3 @@ + # locate preference button and click it + buttons = panel.findChildren(lambda x: x.roleName == 'push button' \ + and not x.name and x.showing and x.sensitive) can we please align the 'and' to 'x.role..' ? @@ +263,3 @@ + buttons[0].click() + + slp = 0 same comment about unknown abbreviations. @@ +418,3 @@ + frame = core.findAncestor(predicate.GenericPredicate(name='Boxes', roleName='frame')) + assert frame != main, "Cannot focus detached window" + assert frame.children[0].children[-1].child(roleName='push button').showing == False, \ Can we please divide this line with lots of call chains using variables please?
Review of attachment 290443 [details] [review]: I've incorporated all comments and pushing new patch. ::: tests/environment.py @@ +13,3 @@ +import sys + +def dlfile(url): corrected @@ +24,3 @@ + local_file.write(f.read()) + + #handle errors yeah, documentation tuned a bit too @@ +160,3 @@ + + # clean all boxes + #os.system("rm -f ~/.config/libvirt/qemu/boxes*") removed
Created attachment 293923 [details] [review] Add general tests Behave/Dogtail set of automated tests. Tests use 3 10 MB iso files that are downloaded at the beginning. Installed-tests infrastructure bits are also included so tests can be executed via gnome-desktop-testing-runner (after running ./autogen.sh --enable-installed-tests). Test can be executed one by one or in a batch. More details to be found in README file. Coverage in general section: * start/stop * help/credits * download an iso * customize memory * start box * search boxes https://bugzilla.gnome.org/review?bug=736288
Review of attachment 290444 [details] [review]: new patch coming shortly ::: tests/README @@ -30,3 @@ * behave (python-behave in Fedora) * dogtail - removed
Created attachment 293928 [details] [review] Add vmdk/qcow2 vm import tests New tests for adding vmdk and qcow2 VM images. These images are the same Tiny Core Linux LiveCD but installed and conserved. Coverage: * import local vmdk/qcow2 images * restart persistence https://bugzilla.gnome.org/review?bug=736288
Review of attachment 290445 [details] [review]: Yes, this is correct. No new steps needed as all steps were defined before and are reused. Kudos to behave :-).
Created attachment 293930 [details] [review] Add LiveCD tests Patch adding LiveCD tests. Tiny Core 10 MB isos are used to avoid large downloads. Coverage: * add new livecd from file and menu * go into box * delete/undo * poweroff/pause/resume * force shutdown * restart persistence https://bugzilla.gnome.org/review?bug=736288
Review of attachment 290446 [details] [review]: new patch coming.. ::: Makefile.am @@ +76,3 @@ @BEHAVE_INSTALLED_TESTS_RULE@ +INSTALLED_TESTS= general.feature import-vms.feature livecd.feature \ + multi-windows.feature single feature single line now ::: tests/multi-windows.feature @@ +41,3 @@ + When Ping "Core-5 3" + * Focus "Core-5" window + * Type "sudo ifconfig eth0 down" This is checking several things not just eth0 going down but also sensitivity to keyboard presses. I am testing this via ping as it's kind of easy. ::: tests/steps/steps.py @@ +243,3 @@ + # for each window (aka box) + for box in boxes: + button = "Open in %s new windows" %len(names) done @@ +244,3 @@ + for box in boxes: + if box != context.app.children[0]: + button = "Open in %s new windows" %len(names) comments sanitized @@ +252,3 @@ + # if icon has text property equal to box name we've found it + if child.text == vm: + sleep(3) different variable name used now @@ +260,3 @@ + # locate preference button and click it + buttons = panel.findChildren(lambda x: x.roleName == 'push button' \ + done @@ +263,3 @@ + buttons[0].click() + + for box in boxes: changed to something more readable @@ +418,3 @@ + frame = core.findAncestor(predicate.GenericPredicate(name='Boxes', roleName='frame')) + assert frame != main, "Cannot focus detached window" + assert frame.children[0].children[-1].child(roleName='push button').showing == False, \ done
Created attachment 293936 [details] [review] Add multi-windows tests Switching between windows was quite tough as boxes name widgets are not visible to dogtail until you go to preferences. So clicking through machines' preferences is done just after opening new windows. Coverage: * open in new window * poweroff in new window * open 3 new windows https://bugzilla.gnome.org/review?bug=736288
Created attachment 293938 [details] [review] Add automated snapshots tests Snapshots are tested via saving snapshot with working network then shutting the network down from inside the box and reverting back to the working state. Coverage: * Create and revert snapshots * Rename snapshot * Delete https://bugzilla.gnome.org/review?bug=736288
Created attachment 293939 [details] [review] Add snapshots tests Snapshots are tested via saving snapshot with working network then shutting the network down from inside the box and reverting back to the working state. Coverage: * Create and revert snapshots * Rename snapshot * Delete https://bugzilla.gnome.org/review?bug=736288
Created attachment 293940 [details] [review] Add SPICE tests Spice is tested via localhost connection and network up/down with ping from inside the machine to check if typing works. Coverage: * new spice localhost connection * restart persistence https://bugzilla.gnome.org/review?bug=736288
Created attachment 293943 [details] [review] Add system-broker tests These tests do have a few prerequsites: passwordless access to system-broker (see README for more details) virt-install binary running libvirtd When prerequsites are not met tests are skipped (via exit code 77) Coverage: * connect to system broker (SB) * delete/undo machine from SB * reflect delete/addition in SB * pause/resume SB box * force shutdown in SB * restart persistence https://bugzilla.gnome.org/review?bug=736288
Created attachment 293948 [details] [review] Add VNC tests These tests have prerequisites for available vncserver binary. Otherwise skipped with exit(77). Coverage: * new local vnc box * restart persistence Let's do some test description here. It may be of value for other tests too.. test case: Background: * Make sure that gnome-boxes is running * Wait until overview is loaded @new_vnc_localhost_box Scenario: New vnc box * Create new box from url "vnc://localhost:5901;" * Wait for "sleep 2" end * Press "Create" Then Box "localhost" "does" exist * Go into "localhost" box * Wait for "sleep 1" end * Hit "<Super_L>" * Wait for "sleep 1" end * Type "gnome-terminal" * Wait for "sleep 1" end * Type "echo 'walderon' > /tmp/vnc_text.txt" Then "walderon" is visible with command "cat /tmp/vnc_text.txt" Then Press "back" in vm so. the first thing which is called in test itself by behave is execution of environment.py. Behave defines several possible actions that differ according to execution time. The order (and names) of these actions is as follows: 0. before all <-executed prior every single test 1. before scenario <- executed prior name specified scenario 2. before tag <- executed prior tag (tag == go_into_local_livecd_box) 3. after scenario 4. after tag 5 after all This environment is executed for every single test. Before all action for first time setup (downloading images, disabling screen saver, etc), then touching /tmp/boxes_counfigured and not repeating this again. App class is defined here via context.app_class = App('gnome-boxes') (App class comes from common_steps) Before scenario is not used in my tests at all. Before tag is used in this test (via vnc tag) for starting localhost vncserver. After step is not used except test.status is fail for log bundling. After tag is used for vncserver stopping After scenario is used deleting all possible boxes by selecting one-by-one and clicking delete .. and x next to undo. then boxes are terminated. Some more logging is done here and some sanity deletes too. Not sure if these deletes are still needed but they're there just to be sure env is clean. Note that even if tests are defined via features (like a bunch of tests) in Makefile.am they're executed one by one by beaker. So all this does not change even if features are used instead of tests. Behave then moves to vnc.feature file (via tag again): Feature files contain Scenarios (accompanied by tags for execution and setup/teardown -- scenario may have more tags) and scenarios contain steps that are defined in steps/steps.py which are very likely absorbing parameters to be as widely reusable as possible. Background: describes steps that are executed for every single test in this case * Make sure that gnome-boxes is running ^^ this goes back to common_steps and starts application itself * Wait until overview is loaded ^^ this waits until overview is loaded. In fact it waits for New button being visible. This step contains sub-step @new_vnc_localhost_box ^^ this defines tag for execution and via if 'vnc' in tag: in environment for start/stop of vnc server Scenario: New vnc box ^^ just human readable scenario name..not used anywhere * Create new box from url "vnc://localhost:5901;" ^^ here comes a bit of dogtail. Behave searches in steps.py for according step. In this case: @step(u'Create new box from url "{url}"') def create_new_vm_via_url(context, url): context.app.child('New').click() context.app.child('Continue').click() context.app.child('Enter URL').click() typeText(url) context.app.child('Continue').click() <snip> so it clicks New, then Continue and then Enter URL, then url (before mentioned step parameter) is typed in and Continue is clicked. Some more logic for http downloading is there but that's not important now. * Wait for "sleep 2" end ^^ waiting for two seconds * Press "Create" ^^ pressing create. Create is done separately as you may want to change some VM prefs before. Not applied here. After create, Boxes goes back to overview. Then Box "localhost" "does" exist ^^ Check that localhost box is present in overview * Go into "localhost" box ^^ localhost box is clicked * Wait for "sleep 1" end ^^ wait for 1 second to vnc screen to appear (sufficient while it's local, should be more if remote) * Hit "<Super_L>" ^^ call step that contains pressKey dogtail method with Super_L param. Thus overview is opened in vnc box as it should be focused after Go into "vm_name" step * Wait for "sleep 1" end ^^ Wait one more second for overview to appear * Type "gnome-terminal" ^^ enter gnome-terminal into overview search bar and hit enter * Wait for "sleep 10" end ^^ wait 10 more seconds (it has to be quite a lot as low memory machines are sllooooww) * Type "echo 'walderon' > /tmp/vnc_text.txt" ^^ now while focused into terminal write above mentioned echo into a file in /tmp Then "walderon" is visible with command "cat /tmp/vnc_text.txt" ^^ as the box is local, the file will be in /tmp dir with walderon inside. Then Press "back" in vm ^^ this just goes back to overview. then after tag and after scenario are executed, vncserver stopped and boxes deleted. https://bugzilla.gnome.org/review?bug=736288
(In reply to comment #34) > Created an attachment (id=293923) [details] [review] > Add general tests Dude! The first thing this does is delete all my VMs. :( I had important VMs that i use for various purposes setup and now they are all gone. :( At least I would expect a big warning about this in the README and commit log. Test of 'no boxes' view is good but not if that means anyone who tries the tests ends up loosing all VMs and all data on them. We need to remove the test/steps in question at least until we have a way to restore the VMs once we have run the testcase for 'no boxes' UI.
(In reply to comment #46) > (In reply to comment #34) > > Created an attachment (id=293923) [details] [review] [details] [review] > > Add general tests > > Dude! The first thing this does is delete all my VMs. :( I had important VMs > that i use for various purposes setup and now they are all gone. :( At least I > would expect a big warning about this in the README and commit log. Uff, really sorry for that. We need to announce this very transparently. I would say first run can do it in environment and quit if there are any VMs. These tests are targeted for test environment. Have you tried extundelete? You can restore files with this tool.
(In reply to comment #47) > (In reply to comment #46) > > (In reply to comment #34) > > > Created an attachment (id=293923) [details] [review] [details] [review] [details] [review] > > > Add general tests > > > > Dude! The first thing this does is delete all my VMs. :( I had important VMs > > that i use for various purposes setup and now they are all gone. :( At least I > > would expect a big warning about this in the README and commit log. > > Uff, really sorry for that. We need to announce this very transparently. I > would say first run can do it in environment and quit if there are any VMs. > These tests are targeted for test environment. I would want these tests to be available to anyone who wants to hack on Boxes (e.g me) and not everyone have separate machines for running tests. I could run tests in a VM but tests would run extremely slow since KVM won't be available inside a VM. So I think tests should be oblivious to existing VMs. If they need to delete them for a reason, they should be able to restore them too. > Have you tried extundelete? You can restore files with this tool. I didn't know of this tool. I'll give it a go.. Thanks.
(In reply to comment #48) > (In reply to comment #47) > > (In reply to comment #46) > > > (In reply to comment #34) > > > > Created an attachment (id=293923) [details] [review] [details] [review] [details] [review] [details] [review] > > > > Add general tests > > > > > > Dude! The first thing this does is delete all my VMs. :( I had important VMs > > > that i use for various purposes setup and now they are all gone. :( At least I > > > would expect a big warning about this in the README and commit log. > > > > Uff, really sorry for that. We need to announce this very transparently. I > > would say first run can do it in environment and quit if there are any VMs. > > These tests are targeted for test environment. > > I would want these tests to be available to anyone who wants to hack on Boxes > (e.g me) and not everyone have separate machines for running tests. I could run > tests in a VM but tests would run extremely slow since KVM won't be available > inside a VM. > I am testing inside VM, using nested KVM, pretty quick. http://kashyapc.com/2012/01/14/nested-virtualization-with-kvm-intel/ > So I think tests should be oblivious to existing VMs. If they need to delete > them for a reason, they should be able to restore them too. > any idea how to do this reliably? Could you help me with writing some script for this? > > Have you tried extundelete? You can restore files with this tool. > > I didn't know of this tool. I'll give it a go.. Thanks.
(In reply to comment #49) > (In reply to comment #48) > > (In reply to comment #47) > > > (In reply to comment #46) > > > > (In reply to comment #34) > > > > > Created an attachment (id=293923) [details] [review] [details] [review] [details] [review] [details] [review] [details] [review] > > > > > Add general tests > > > > > > > > Dude! The first thing this does is delete all my VMs. :( I had important VMs > > > > that i use for various purposes setup and now they are all gone. :( At least I > > > > would expect a big warning about this in the README and commit log. > > > > > > Uff, really sorry for that. We need to announce this very transparently. I > > > would say first run can do it in environment and quit if there are any VMs. > > > These tests are targeted for test environment. > > > > I would want these tests to be available to anyone who wants to hack on Boxes > > (e.g me) and not everyone have separate machines for running tests. I could run > > tests in a VM but tests would run extremely slow since KVM won't be available > > inside a VM. > > > > I am testing inside VM, using nested KVM, pretty quick. > http://kashyapc.com/2012/01/14/nested-virtualization-with-kvm-intel/ Ooh! I wasn't aware of this possibility. I'll look into it. > > So I think tests should be oblivious to existing VMs. If they need to delete > > them for a reason, they should be able to restore them too. > > > > any idea how to do this reliably? For each VM, you'll want to save the domain XML somewhere and then instead of deleting from Boxes, simply undefine them using libvirt directly (perhaps through virsh commands). When restoring, recreate the VMs using the XMLs you saved. Shouldn't be too hard. > Could you help me with writing some script > for this? After looking into some bugs and trying to get these patches merged, I was hoping to finally focus on VM import/export feature for rest of the month at least. Having said that, if you won't have time, for now please add the warnings in all appropriate places and we can merge this for 3.14 branch soon. I can look into this later. > > > Have you tried extundelete? You can restore files with this tool. > > > > I didn't know of this tool. I'll give it a go.. Thanks. This tool wont be able to help me after all. After my last disk migration, I had forgotten to upgrade my home partition from ext2 to ext4. :(
(In reply to comment #50) > (In reply to comment #49) > > (In reply to comment #48) > > > (In reply to comment #47) > > > > (In reply to comment #46) > > > > > (In reply to comment #34) > > > > > > Created an attachment (id=293923) [details] [review] [details] [review] [details] [review] [details] [review] [details] [review] [details] [review] > > > > > > Add general tests > > > > > > > > > > Dude! The first thing this does is delete all my VMs. :( I had important VMs > > > > > that i use for various purposes setup and now they are all gone. :( At least I > > > > > would expect a big warning about this in the README and commit log. > > > > > > > > Uff, really sorry for that. We need to announce this very transparently. I > > > > would say first run can do it in environment and quit if there are any VMs. > > > > These tests are targeted for test environment. > > > > > > I would want these tests to be available to anyone who wants to hack on Boxes > > > (e.g me) and not everyone have separate machines for running tests. I could run > > > tests in a VM but tests would run extremely slow since KVM won't be available > > > inside a VM. > > > > > > > I am testing inside VM, using nested KVM, pretty quick. > > http://kashyapc.com/2012/01/14/nested-virtualization-with-kvm-intel/ > > Ooh! I wasn't aware of this possibility. I'll look into it. yup, pretty powerful. > > > > So I think tests should be oblivious to existing VMs. If they need to delete > > > them for a reason, they should be able to restore them too. > > > > > > > any idea how to do this reliably? > > For each VM, you'll want to save the domain XML somewhere and then instead of > deleting from Boxes, simply undefine them using libvirt directly (perhaps > through virsh commands). When restoring, recreate the VMs using the XMLs you > saved. Shouldn't be too hard. > > > Could you help me with writing some script > > for this? > > After looking into some bugs and trying to get these patches merged, I was > hoping to finally focus on VM import/export feature for rest of the month at > least. > > Having said that, if you won't have time, for now please add the warnings in > all appropriate places and we can merge this for 3.14 branch soon. I can look > into this later. > I will add one more patch moving all configuration to backup location and back after test. hope it will work. Just testing it. > > > > Have you tried extundelete? You can restore files with this tool. > > > > > > I didn't know of this tool. I'll give it a go.. Thanks. > > This tool wont be able to help me after all. After my last disk migration, I > had forgotten to upgrade my home partition from ext2 to ext4. :( try testdisk, should be working: http://www.cgsecurity.org/wiki/TestDisk:_undelete_file_for_ext2
(In reply to comment #51) > (In reply to comment #50) > > > > So I think tests should be oblivious to existing VMs. If they need to delete > > > > them for a reason, they should be able to restore them too. > > > > > > > > > > any idea how to do this reliably? > > > > For each VM, you'll want to save the domain XML somewhere and then instead of > > deleting from Boxes, simply undefine them using libvirt directly (perhaps > > through virsh commands). When restoring, recreate the VMs using the XMLs you > > saved. Shouldn't be too hard. > > > > > Could you help me with writing some script > > > for this? > > > > After looking into some bugs and trying to get these patches merged, I was > > hoping to finally focus on VM import/export feature for rest of the month at > > least. > > > > Having said that, if you won't have time, for now please add the warnings in > > all appropriate places and we can merge this for 3.14 branch soon. I can look > > into this later. > > > > I will add one more patch moving all configuration to backup location and back > after test. hope it will work. Just testing it. Fingers crossed! While you are at it, could you please also remove whitespace noise from all patches: $ git bz apply 736288 Bug 736288 - add automated tests Add general tests Apply? [yn] y Applying: Add general tests /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:776: trailing whitespace. /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:264: new blank line at EOF. + /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:473: new blank line at EOF. + /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:776: new blank line at EOF. + warning: 4 lines add whitespace errors. > > > > > Have you tried extundelete? You can restore files with this tool. > > > > > > > > I didn't know of this tool. I'll give it a go.. Thanks. > > > > This tool wont be able to help me after all. After my last disk migration, I > > had forgotten to upgrade my home partition from ext2 to ext4. :( > > try testdisk, should be working: > http://www.cgsecurity.org/wiki/TestDisk:_undelete_file_for_ext2 Tried that but it didn't find my files. :(
Review of attachment 293923 [details] [review]: I wondered why git-bz didn't allow me to set the patch status on pushing it. Turns out that you had put the URL to the review page instead of bug itself. Which means you didn't use git-bz. I'll strongly recommend using that.
(In reply to comment #53) > Review of attachment 293923 [details] [review]: > > I wondered why git-bz didn't allow me to set the patch status on pushing it. > Turns out that you had put the URL to the review page instead of bug itself. > Which means you didn't use git-bz. I'll strongly recommend using that. I used git-bz attach 736288 HEAD so it should be OK, isn't it? I added bug number to commit message. Am I missing something? Anyway, thanks for merging, I'll commit backup changes tomorrow.
(In reply to comment #54) > (In reply to comment #53) > > Review of attachment 293923 [details] [review] [details]: > > > > I wondered why git-bz didn't allow me to set the patch status on pushing it. > > Turns out that you had put the URL to the review page instead of bug itself. > > Which means you didn't use git-bz. I'll strongly recommend using that. > > I used git-bz attach 736288 HEAD so it should be OK, isn't it? I added bug > number to commit message. Am I missing something? git-bz adds the bug URL for you when you attach. There is even a command to only add bug reference: git-bz add-url. > Anyway, thanks for merging, I'll commit backup changes tomorrow. Cool. Looking forward. I might change a few small things in the meantime, e.g Removal of ugly 'u' in the beginning of every string: https://plus.google.com/109973678654056732542/posts/iJo31gCCLsh . So please do rebase before uploading them. Oh and I made this bug for 3.14, lets create another bug for patches to master.
Created attachment 294146 [details] [review] Backup existing VMs before test execution Existing VMs are now backed up with saved states and snaphots priot each test and restored after test batch finishes. System broker link is backed up too, if present. Warning and manual restore howto were added into README file.
Created attachment 294159 [details] [review] Backup existing VMs before test execution Existing VMs are now backed up with saved states and snaphots priot each test and restored after test batch finishes. System broker link is backed up too, if present. Warning and manual restore howto were added into README file. Minor fixes to previous unicode removal patch.
Review of attachment 294159 [details] [review]: Cool. Some notes: 1. Lets prefix shortlog of test related patches with "tests: " so "tests: Backup VMs before execution" (also try your best to keep under 50 chars so dropped "existing"). 2. Description is supposed to be normal English text and hence either start a new paragraph (using a blank line) or continue next statement on the same line as the previous one ends, unless that means exceeding 74 chars limit. 3. "Minor fixes to previous unicode removal patch." is unrelated to the main change and should be in its own patch. ::: tests/environment.py @@ +74,3 @@ + + #dump all machines + call("for i in $(virsh list --all |grep boxes |awk '{print $2}'); do virsh dumpxml $i > ~/boxes_backup/$i.xml;done", shell=True) Limit for each line of code is 120 chars. @@ +76,3 @@ + call("for i in $(virsh list --all |grep boxes |awk '{print $2}'); do virsh dumpxml $i > ~/boxes_backup/$i.xml;done", shell=True) + + #move disk images Space after '#' please. @@ +158,3 @@ os.system('virsh -q -c qemu:///system undefine Core-5.3 > /dev/null 2>&1') + unrelated change to this patch. @@ +200,3 @@ + child.click(button=3) + delete = True + if delete: this indirect way of deleting when there is children to delete is a bit unreadable. Why not just 'if len(pane.children) > 0' instead? @@ +242,3 @@ + + # import machines + call("for i in $(ls ~/boxes_backup |grep xml); do virsh define ~/boxes_backup/$i;done", shell=True, stdout=f) this also violates 120 char limit @@ +271,3 @@ + print "** Restoring system broker backup" + # import system broker + call("for i in $(ls ~/boxes_backup | grep qemu____system) ; do mv ~/boxes_backup/$i ~/.cache/gnome-boxes/sources/; done", shell=True) this too. ::: tests/livecd.feature @@ -100,3 @@ # https://bugzilla.gnome.org/show_bug.cgi?id=742392 @poweroff_local_livecd_box - Scenario: Go into local liveCD box unrelated change?
Review of attachment 294159 [details] [review]: ::: tests/environment.py @@ +242,3 @@ + + # import machines + call("for i in $(ls ~/boxes_backup |grep xml); do virsh define ~/boxes_backup/$i;done", shell=True, stdout=f) nm, this one doesn't actually.
Created attachment 294199 [details] [review] tests: Fix strings broken by 1393650
Created attachment 294200 [details] [review] tests: Remove some more ugly 'u' prefixes Remove 'u' prefices that should have been done in commit 1393650.
Created attachment 294201 [details] [review] tests: Remove a redundant empty line
Created attachment 294202 [details] [review] tests: Better label for a scenario
Created attachment 294203 [details] [review] tests: Backup existing VMs before execution Existing VMs are now backed up with saved states and snaphots prior to each test and restored after test batch finishes. System broker link is backed up too, if present. Warning and manual restore howto are also added into README file.
Fixed all the issue I pointed to in review. One thing I haven't yet fixed is that snapshot deletion test is now failing for me: @delete_snapshots Scenario: Delete snapshots # snapshots.feature:26 * Make sure that gnome-boxes is running # common_steps.py:192 2.029s * Wait until overview is loaded # steps/general.py:192 0.062s * Initiate new box "Core-5" installation # steps/creation.py:69 9.700s * Create snapshot "working network" from machine "Core-5" # steps/snapshot.py:42 method return sender=:1.32 -> dest=:1.520 reply_serial=2 * Create snapshot "working network" from machine "Core-5" # steps/snapshot.py:42 35.748s Assertion Failed: FAILED SUB-STEP: * Press "Properties" Substep info: Traceback (most recent call last):
+ Trace 234537
match.run(runner.context)
self.func(context, *args, **kwargs)
get_showing_node_name(button, context.app).click()
raise Exception("Timeout: Node %s wasn't found showing" %name) Exception: Timeout: Node Properties wasn't found showing
method return sender=:1.32 -> dest=:1.521 reply_serial=2 boolean true string "/tmp/screenshot.png" * Create snapshot "network down" from machine "Core-5" # None * Delete machines "Core-5" snapshot "working network" # None * Delete machines "Core-5" snapshot "network down" # None Then "error: domain 'boxes-unknown' has no current snapshot" is visible with command "virsh snapshot-current boxes-unknown" # None Failing scenarios: snapshots.feature:26 Delete snapshots 0 features passed, 1 failed, 0 skipped 1 scenario passed, 1 failed, 0 skipped
Created attachment 294232 [details] [review] tests: Backup existing VMs before execution v2: This issue was that i had an image in the images directory but no domain associated with it. I have modified the code to always backup everything, regardless of whether or not there is anything to backup or not.
(In reply to comment #66) > Created an attachment (id=294232) [details] [review] > tests: Backup existing VMs before execution > > v2: This issue was that i had an image in the images directory but no domain > associated with it. I have modified the code to always backup everything, > regardless of whether or not there is anything to backup or not. Actually the issue is still reproducible. It seems to me that some scenerio fails to delete all boxes at the end.
Pushed all these innocent ones: Attachment 294199 [details] pushed as e2dd1b7 - tests: Fix strings broken by 1393650 Attachment 294200 [details] pushed as 67b1696 - tests: Remove some more ugly 'u' prefixes Attachment 294201 [details] pushed as 54c7816 - tests: Remove a redundant empty line Attachment 294202 [details] pushed as 7454d8e - tests: Better label for a scenario
Created attachment 294697 [details] [review] tests: Align some comments correctly
Created attachment 294720 [details] [review] tests: Correct title of a scenario
Created attachment 294721 [details] [review] tests: Don't delete test boxes through UI For some reason, deletion of boxes from UI doesn't always remove the associated storage volume so lets delete the VM's domain through virsh and delete all possible associated data manually through rm command. It also makes things much faster.
Created attachment 294722 [details] [review] tests: Backup existing VMs before execution Existing VMs along with their saved states and snaphots, and sources are now moved away (instead of deletion) prior to test batch execution and restored after wards. Warning and manual restore howto are also added into README file.
While the backup and restoring of all boxes is working much more reliably with latest patches, some tests still fail for me if i run the whole batch. I'll look again at this soon.
Created attachment 295250 [details] [review] tests: Don't delete test boxes through UI For some reason, deletion of boxes from UI doesn't always remove the associated storage volume so lets delete the VM's domain through virsh and delete all possible associated data manually through rm command. It also makes things much faster.
Created attachment 295251 [details] [review] tests: Backup existing VMs before execution Existing VMs along with their saved states and snaphots, and sources are now moved away (instead of deletion) prior to test batch execution and restored after wards. Warning and manual restore howto are also added into README file.
So with the latest patches, the backup and restoring seems to work much more reliably. Also, i made individual tests and even scenerios pass now but funny/weird thing is that if i run the whole test suite, tests still fail. :( Also I see this error: Failed to get data: Cannot assign requested address from the os.system("journalctl ..) call at tests/environment.py:185 . Now again funnily if i execute the same command manually from terminal, it works and there is no such error from it.
Created attachment 295335 [details] [review] tests: Don't delete test boxes through UI Removed unrelated changes and improved the sed regex.
Created attachment 295336 [details] [review] tests: Backup existing VMs before execution Improved the sed regex. Tests and scenarios work so much more reliably now but for some reason we still have these tests failing when i run the whole set together: livecd.feature:56 Delete local liveCD box livecd.feature:134 Force off local liveCD box multi-windows.feature:17 Poweroff in new window multi-windows.feature:27 Open three new windows
Created attachment 295339 [details] [review] tests: Don't delete test boxes through UI Final version: One small correction to sed expressions and all tests pass now. \o/
Created attachment 295340 [details] [review] tests: Backup existing VMs before execution Same as previous patch/comment.
Attachment 294697 [details] pushed as 7cc0bfa - tests: Align some comments correctly Attachment 294720 [details] pushed as 669ec92 - tests: Correct title of a scenario Attachment 295339 [details] pushed as 068784a - tests: Don't delete test boxes through UI Attachment 295340 [details] pushed as 0a30026 - tests: Backup existing VMs before execution