On 05/11/2012 09:38 PM, tangchen wrote: > Hi, Chris: > > Thanks for all these comments. :) > >> tangchen, >> >> Thanks a TON for all these great tests, overall it's very thorough >> and extensive. I'd really like to see some of these tests in >> action, but didn't want to spend lots of time setting it up w/o >> providing some initial feedback: >> >> There are a handful of whitespace errors: >> client/tests/libvirt/tests/virsh_migrate.py line 45 >> client/tests/libvirt/tests/virsh_migrate.py line 263 >> client/virt/subtests.cfg.sample line 586 >> client/virt/subtests.cfg.sample around line 592 > > OK, I'll fix them. > >> >> do_error_migration() This is a good catch, we need to do negative >> testing like this. However, implementing it inside a test makes me >> a little uncomfortable. It may be that other test modules would >> like to do migration error testing but aren't able to because the >> code is locked up in this test. >> >> Your comment says the migration functions are hiding some error >> conditions, which reads to me like a deficiency in the core code. >> Could we fix vm.migrate() and/or virsh_migrate() to support error >> testing? Maybe a keyword parameter with a default could be added to >> one or both to support it? >> > True. I thought about this before. My original idea was handling no > error in vm.migrate(), but this was tough. As you said, a keyword > parameter with a default is a quit good idea. I'm glad to finish it. > Leave it to me. :) > >> do_migration() Thanks for changing "dly" to "delay", it's much >> clearer. >> >> #FIXME: vm.verify_kernel_crash() I'm pretty sure we fixed this by >> now, but I neglected to update this test. I'll confirm by simple >> test (messing up root= parameter in grub). >> >> # For safety reasons, we'd better back up xmlfile. This is a >> fantastic idea! What do you think about adding this as a method of >> the vm class, so we can use it elsewhere also? >> > Not only back-up-xml function, but alse some other functions (such as > rename-a-vm, which is under developping) may be useful in all libvirt > tests, especially in error situations. And IMO, just like > libvirtd_start/stop functions, we can put them together in > libvirt_vm.py. What do you think? :)
Yep, always happy to see extensions like this added. I've got a laundry list of them myself, which I plan on working over the coming months. I seem to remember reading at some point, someone wanted to split the virsh_*() functions off into a virsh.py module. If you want to tackle that, please feel free, but it's not urgent by any stretch. I'm sure it'll come up again when the code/test split nears. I'm not super particular about where the code lands, and libvirt_vm.py sounds like a good place for now. >> If any of the above fixes/changes you don't have time/resources >> for, just let me know and I'll take care of 'em. Overall it looks >> really good and I'll be happy to see this applied. >> > It's no problem, I'll finish them. > >> I'll start working up a pair of lab boxes to run this code on. >> Depending on how many "challenges" I run into doing that, it could >> be late next week before I get to run any of these cool new tests. >> But I'll work on it and let you know if I have more feedback. >> >> Thanks again! >> > Thanks. :) > I'm still working on getting some lab boxes, but have it mostly figured out. I'm excited to see all these new migration tests running! -- Chris Evich, RHCA, RHCE, RHCDS, RHCSS Quality Assurance Engineer e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214 _______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
