Merge lp://qastaging/~rohitk/nova/libvirt_unittests into lp://qastaging/~hudson-openstack/nova/trunk

Only two minor pep8 violations (lines 309, and 2915 - adding a comma will make them go away). Also, a nit about the whitespace in the XML (lines 321 to 332). Other than that, it looks good.

Fixed, however, please use latest pep8 (0.6.1). The code was validated for pep8 0.6.1. Thanks!

22 +from StringIO import StringIO

Although other imports in the file don't follow this, we should only be importing modules, not classes. So technically this should be just "import StringIO"

44 + 'injected': 1}

Minor nitpick, this indent should be consistent with the others.

116 + except_msg = "Test failed with an unknown exception"

This is an interesting strategy, but I would suggest when something happens and you don't know what went wrong to just do a self.fail(error_message), where 'error_message' is the exception contents. Just saying "unknown exception" isn't very helpful when trying to debug, but the failure message will help out greatly.

For example:

524 + try:
525 + conn.spawn(instance, network_info)
526 + except Exception, e:
527 + fail = 1
528 + time.sleep(1)
529 +
530 + self.assertFalse(fail, msg=self.except_msg)

Would give me a generic failure message, but:

524 + try:
525 + conn.spawn(instance, network_info)
526 + except Exception as err:
527 + self.fail(err)

Would give the reason, while still making sure the test failed.

Lines 300 - 307, you shouldn't need those backslashes and if you could, where applicable, line up parameters under the space after the method's opening "(" like this:

300 + conn.firewall_driver.setattr('setup_basic_filtering',
301 + self.fake_none)

In all of the tests, FLAGS are auto-magically reset, so feel free to just edit them directly:

348 + fake_flag = 'VIR_MIGRATE_UNDEFINE_SOURCE,VIR_MIGRATE_PEER2PEER'
349 + self.mox.StubOutWithMock(connection.FLAGS, 'live_migration_flag')
350 + connection.FLAGS.live_migration_flag = fake_flag

Could safely become:

348 + migration_flag = 'VIR_MIGRATE_UNDEFINE_SOURCE,VIR_MIGRATE_PEER2PEER'
349 + FLAGS.live_migration_flag = migration_flag

There are existing structures for Mocks and Stubs, so this:

451 + self.mox.StubOutWithMock(connection.LibvirtConnection, 'get_info')
452 + connection.LibvirtConnection.get_info = fake_get_info

could just be:

451 + self.stubs.Set(connection.LibvirtConnection, 'get_info', fake_get_info)

if you wanted to do it in a single line.

As a general comment, if you find yourself repeating test "setup", such as creating a connection and setting some test parameters, you might consider breaking similar tests into their own TestCase classes and putting the common code in setUp().

I'm going to keep looking at this, as it's a good number of tests...but hopefully this will kick off some discussion.

Thanks for the tests!

test_get_cpu_info
test_get_cpu_info_nocpus
test_get_cpu_info_nokeys
test_get_disks
test_get_interfaces
test_get_interfaces_libxml_exception

These tests are failing when libvirt isn't installed on the testing machine. Can you skip these tests if libvirt isn't installed?

Thanks Brian for the comments till now. I'll make those fixes wherever applicable.

Brian, request you to please review the branch for merging. I have implemented all the review comments by you. Thanks.

Hi Rohit!

Awesome work on these tests :)

One little nit from me...

 import shutil
 import sys
+import time
 import tempfile

+import StringIO
 from xml.etree.ElementTree import fromstring as xml_to_tree

That actually needs to be in alphabetical order, and StringIO is a standard lib module and so should be above tempfile import..., so like this:

import shutil
import StringIO
import sys
import tempfile
import time

from xml.etree.ElementTree import fromstring as xml_to_tree

Cheers!
jay

Thanks Jay. Fixed the import sequence and other regressions cropped due to trunk changes. All tests have passed at my end.

Looks great!

Getting 3 test errors: http://paste.openstack.org/show/2381/

The first two errors are fixed. I am not able to reproduce the third error, from test_virt_drivers.py, and this file(or it's dependencies) was not modified by this branch. Also noticed large test failures in current trunk due to which full cobertura report is not being generated.
Please review branch for modified files. Thanks!

I'm getting the third error with your most recent changes as well from a fresh, unmodified checkout. Not sure why you're not getting it.

One thought, make sure you run the entire test suite, not just that one file or class of tests....it may be conflicting with something run in other tests...which would be bad.

Hi William,

The issue was with the method test_map_to_instance_info that directly set the driver.InstanceInfo object to invalid test values and failed to unset during teardown. I have removed that code and modified the test to use only FakeVirtDomain object, and now the tests are passing without affecting test_virt_drivers tests. Thank you for pointing it out.

I have run the full suite and tests have passed. Please review.

Run tests output at http://paste.openstack.org/show/2474/

Everything passes for me now Rohit. Thanks, looks good.

Awesome to see a new collection of unit tests in the project! Everything passes for me.

Hey, I gave some bad advice earlier, which is 100% my fault. Unfortunately I was wrong when I said that you should set flags like "FLAGS.live_migration_flag = migration_flag". In tests flags seem to be reset only when you set them like this:

self.flags(live_migration_flag=migration_flag)

So just to be safe I think that these tests should follow that principle. Have you spoken with Soren? He also has a major branch for libvirt tests (lp:~soren/nova/virt-test-improvements), although I don't know how much they will conflict.

Sorry for taking so long to get back to you on this branch!

> self.flags(live_migration_flag=migration_flag)
>
> So just to be safe I think that these tests should follow that principle.
>
Changed wherever applicable.

> Have you spoken with Soren? He also has a major branch for libvirt tests
> (lp:~soren/nova/virt-test-improvements), although I don't know how much they
> will conflict.

I just checked Soren's branch. Although there isn't any conflicting code or tests, I think the fake images library he has added will help me with not having to create the fakes in test_libvirt. I am putting the branch push on hold till I sync with Soren.

Thanks for checking my branch.