New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Libvirt/libvirt node driver cleanup and tests #838

Closed
wants to merge 10 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@Katana-Steel
Contributor

Katana-Steel commented Jul 9, 2016

Extra tests for libvirt_driver and cleaner driver creation

Description

Code clean up and added more tests for the libvirt_driver module.
Cleaner driver creation with less console output when using SASL
authentication.

Status

  • done, ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)
Show outdated Hide outdated libcloud/test/compute/test_libvirt_driver.py
def __init__(self):
stub = self.stub

This comment has been minimized.

@Kami

Kami Jul 10, 2016

Member

I'm fine with this code, but I believe you could also utilize Mock for that by specifying a class you want to mock for spec argument - http://www.voidspace.org.uk/python/mock/mock.html#mock.Mock.

This would mean less code to maintain and keep up to date :)

@Kami

Kami Jul 10, 2016

Member

I'm fine with this code, but I believe you could also utilize Mock for that by specifying a class you want to mock for spec argument - http://www.voidspace.org.uk/python/mock/mock.html#mock.Mock.

This would mean less code to maintain and keep up to date :)

Show outdated Hide outdated libcloud/test/compute/test_libvirt_driver.py
name = 'Libvirt'
website = 'http://libvirt.org/'
NODE_STATE_MAP = {

This comment has been minimized.

@Kami

Kami Jul 10, 2016

Member

Hm, do we need this attributes here on the test class? They are already defined in the LibvirtNodeDriver class.

Also, usually the idea is not to have the test class inherit from the Driver class, but the test case should instantiate the driver class and use that.

For example:

def setUp(self):
    ....
    self.driver = LibvirtNodeDriver(...)

def some_test_method(self):
    nodes = self.driver.list_nodes()
    ...
@Kami

Kami Jul 10, 2016

Member

Hm, do we need this attributes here on the test class? They are already defined in the LibvirtNodeDriver class.

Also, usually the idea is not to have the test class inherit from the Driver class, but the test case should instantiate the driver class and use that.

For example:

def setUp(self):
    ....
    self.driver = LibvirtNodeDriver(...)

def some_test_method(self):
    nodes = self.driver.list_nodes()
    ...

This comment has been minimized.

@Kami

Kami Jul 10, 2016

Member

It would be great if you could make those changes so it's consistent with other drivers tests and it's a bit easier to maintain it - thanks!

@Kami

Kami Jul 10, 2016

Member

It would be great if you could make those changes so it's consistent with other drivers tests and it's a bit easier to maintain it - thanks!

raise RuntimeError('The remote Libvirt instance requires ' +
'authentication, please set \'key\' and ' +
'\'secret\' parameters')
auth = [[libvirt.VIR_CRED_AUTHNAME, libvirt.VIR_CRED_PASSPHRASE],
self._cred_callback, None]
self.connection = libvirt.openAuth(uri, auth, 0)
else:
self.connection = libvirt.open(uri)

This comment has been minimized.

@Kami

Kami Jul 10, 2016

Member

Hm, so open simply returns None if it's unable to establish a connection?

If so, that's a bit unfriendly and I guess if you want to debug it you need to set LIBVIR_DEBUG environment variable, or is there any other, more user-friendly way to do it?

@Kami

Kami Jul 10, 2016

Member

Hm, so open simply returns None if it's unable to establish a connection?

If so, that's a bit unfriendly and I guess if you want to debug it you need to set LIBVIR_DEBUG environment variable, or is there any other, more user-friendly way to do it?

This comment has been minimized.

@Katana-Steel

Katana-Steel Jul 12, 2016

Contributor

my bad... actually it will raise libvirt.libvirtError in case of a failed connection, fixing

@Katana-Steel

Katana-Steel Jul 12, 2016

Contributor

my bad... actually it will raise libvirt.libvirtError in case of a failed connection, fixing

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 10, 2016

Member

Thanks for this contribution.

I've added some comments. It would be great if you could at least update the tests to follow the same approach as other driver test (sorry for not mentioning in your first tests PR, I missed it :().

Member

Kami commented Jul 10, 2016

Thanks for this contribution.

I've added some comments. It would be great if you could at least update the tests to follow the same approach as other driver test (sorry for not mentioning in your first tests PR, I missed it :().

@Katana-Steel

This comment has been minimized.

Show comment
Hide comment
@Katana-Steel

Katana-Steel Jul 12, 2016

Contributor

reviewed the suggestions and I did read up on Mock for testing
and updated the test code to mock essentially the entire libvirt module just to be safe ;)

Contributor

Katana-Steel commented Jul 12, 2016

reviewed the suggestions and I did read up on Mock for testing
and updated the test code to mock essentially the entire libvirt module just to be safe ;)

Rene Kjellerup added some commits Jul 8, 2016

Rene Kjellerup
cleaned up LibvirtNodeDriver.__init__
checking the uri string for +tcp before trying to do the SASL
connection to the libvirtd service
Rene Kjellerup
passing None to open will use defaults
if we get a connection with None, retrive the connection uri from libvirt.virConnection
Rene Kjellerup
import mock module directly
the mock module is not in unittest2 so include it directly
@Katana-Steel

This comment has been minimized.

Show comment
Hide comment
@Katana-Steel

Katana-Steel Jul 13, 2016

Contributor

the have_libvirt check makes using the mock framework hard when libvirt isn't installed.

I'll take any suggestions... I could go back to inherit and just use MagicMock for virConnection

it seems bad to assume libvirt is installed for tests to pass. Or would that be an acceptable change?

Contributor

Katana-Steel commented Jul 13, 2016

the have_libvirt check makes using the mock framework hard when libvirt isn't installed.

I'll take any suggestions... I could go back to inherit and just use MagicMock for virConnection

it seems bad to assume libvirt is installed for tests to pass. Or would that be an acceptable change?

@@ -37,10 +38,12 @@ deps = -r{toxinidir}/requirements-tests.txt
[testenv:py2.6-lxml]
deps = -r{toxinidir}/requirements-tests.txt
unittest2
mock

This comment has been minimized.

@Kami

Kami Jul 13, 2016

Member

mock is already specified in requirements-tests.txt so no need to also specify it here.

In any case, not a big deal, I will remove it while merging :)

@Kami

Kami Jul 13, 2016

Member

mock is already specified in requirements-tests.txt so no need to also specify it here.

In any case, not a big deal, I will remove it while merging :)

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 13, 2016

Member

@Katana-Steel Ideally we can make it work without requiring libvirt to be installed, at least for the basic unit tests.

I'm fine with having additional integration tests which can require libvirt to be installed as long as those don't run by default if libvirt is not installed (we can use unittest.skipIf and skip if libvirt is not installed).

Member

Kami commented Jul 13, 2016

@Katana-Steel Ideally we can make it work without requiring libvirt to be installed, at least for the basic unit tests.

I'm fine with having additional integration tests which can require libvirt to be installed as long as those don't run by default if libvirt is not installed (we can use unittest.skipIf and skip if libvirt is not installed).

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 13, 2016

Member

Thanks for making those changes.

I will make the mock change and get the tests passing locally and then merge it into trunk.

Member

Kami commented Jul 13, 2016

Thanks for making those changes.

I will make the mock change and get the tests passing locally and then merge it into trunk.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 13, 2016

Member

I made those changes and merged it into trunk - 01c6db4 , d9fc1ed, a4bddfe

Thanks!

Member

Kami commented Jul 13, 2016

I made those changes and merged it into trunk - 01c6db4 , d9fc1ed, a4bddfe

Thanks!

@asfgit asfgit closed this in 608b7ec Jul 13, 2016

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 13, 2016

Member

Also - 6626808

Some time in the next couple of days when I get a chance I will look into running libvirt tests on Travis CI.

For one, we need to make sure all the system level libvirt dependencies are installed on Travis and after that we need to install libvirt library in all the virtual environments used by tox.

Member

Kami commented Jul 13, 2016

Also - 6626808

Some time in the next couple of days when I get a chance I will look into running libvirt tests on Travis CI.

For one, we need to make sure all the system level libvirt dependencies are installed on Travis and after that we need to install libvirt library in all the virtual environments used by tox.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 13, 2016

Member

It looks like that Travis doesn't have libvirt system package whitelisted so sadly it doesn't look like we can get this easily to run on new Travis container based infrastructure :/

https://travis-ci.org/apache/libcloud/jobs/144437976#L118

Member

Kami commented Jul 13, 2016

It looks like that Travis doesn't have libvirt system package whitelisted so sadly it doesn't look like we can get this easily to run on new Travis container based infrastructure :/

https://travis-ci.org/apache/libcloud/jobs/144437976#L118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment