Code review comment for lp://qastaging/~rohitk/nova/libvirt_unittests

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!