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

Already on GitHub? Sign in to your account

Riakclient tests #8

Merged
merged 3 commits into from Apr 11, 2012

Conversation

Projects
None yet
2 participants
Contributor

dctrwatson commented Apr 11, 2012

No description provided.

John Watson added some commits Apr 11, 2012

@Fluxx Fluxx commented on an outdated diff Apr 11, 2012

tests/nydus/db/backends/riak/tests.py
+ self.expected = {
+ 'host': '127.0.0.1',
+ 'port': 8098,
+ 'prefix': 'riak',
+ 'mapred_prefix': 'mapred',
+ 'client_id': None,
+ }
+
+ self.conn = Riak(num=0)
+
+ def test_init(self):
+ args, _, _, defaults = getargspec(Riak.__init__)
+ args = [arg for arg in args if arg != 'self']
+
+ self.assertItemsEqual(args, self.expected.keys())
+ self.assertItemsEqual(defaults, self.expected.values())
@Fluxx

Fluxx Apr 11, 2012

Contributor

This a little bit subtle, but unless a method needs to adhere to a certain interface, I don't think it's necessary/useful to test that the __init__ method kwargs are certain values. It's more important to test behavior of the object.

So in this case, wat's important to the __init__ behavior is:

  1. Omission of certain properties default to expected values. I.e. leaving out the port defaults to 8098.
  2. When providing properties, it uses the user provided properties when constructing the connection. I.e .I provide port 8000 and the connection is on 8000.

I would test these by constructing clients with various combinations of default and provided properties and asserting state on the returned connection from connect()

@Fluxx Fluxx commented on an outdated diff Apr 11, 2012

tests/nydus/db/backends/riak/tests.py
+ 'client_id': None,
+ }
+
+ self.conn = Riak(num=0)
+
+ def test_init(self):
+ args, _, _, defaults = getargspec(Riak.__init__)
+ args = [arg for arg in args if arg != 'self']
+
+ self.assertItemsEqual(args, self.expected.keys())
+ self.assertItemsEqual(defaults, self.expected.values())
+
+ self.assertDictContainsSubset(self.expected, self.conn.__dict__)
+
+ def test_identifier(self):
+ self.assertEquals('http://127.0.0.1:8098/riak', self.conn.identifier)
@Fluxx

Fluxx Apr 11, 2012

Contributor

Also test that when provided different properties in the constructor the identifier changes. Right now I can change the implementation of identifier to this and the test would still pass:

@property
def identifier(self):
    return 'http://127.0.0.1:8098/riak'

@Fluxx Fluxx commented on an outdated diff Apr 11, 2012

tests/nydus/db/backends/riak/tests.py
+ def test_identifier(self):
+ self.assertEquals('http://127.0.0.1:8098/riak', self.conn.identifier)
+
+ @mock.patch('nydus.db.backends.riak.RiakClient')
+ def test_connect_riakclient_options(self, _RiakClient):
+ self.conn.connect()
+
+ _RiakClient.assert_called_with(**self.expected)
+
+ def test_connect_returns_riakclient(self):
+ client = self.conn.connect()
+
+ self.assertIsInstance(client, RiakClient)
+
+ def test_provides_retryable_exceptions(self):
+ self.assertIn(RiakError, self.conn.retryable_exceptions)
@Fluxx

Fluxx Apr 11, 2012

Contributor

Are the other exceptions, socket.error, httplib.HTTPException important to test for too?

@Fluxx Fluxx commented on an outdated diff Apr 11, 2012

tests/nydus/db/backends/riak/tests.py
+ args, _, _, defaults = getargspec(Riak.__init__)
+ args = [arg for arg in args if arg != 'self']
+
+ self.assertItemsEqual(args, self.expected.keys())
+ self.assertItemsEqual(defaults, self.expected.values())
+
+ self.assertDictContainsSubset(self.expected, self.conn.__dict__)
+
+ def test_identifier(self):
+ self.assertEquals('http://127.0.0.1:8098/riak', self.conn.identifier)
+
+ @mock.patch('nydus.db.backends.riak.RiakClient')
+ def test_connect_riakclient_options(self, _RiakClient):
+ self.conn.connect()
+
+ _RiakClient.assert_called_with(**self.expected)
@Fluxx

Fluxx Apr 11, 2012

Contributor

To be a little more refined on this test, what's important for connect() is that it's called with the values of self.port, self.host, self.prefix, etc. So you might want to write the test more this way:

_RiakClient.assert_called_with(host=self.conn.host, port=self.conn.port, prefix=self.conn.prefix, mapred_prefix=self.conn.mapred_prefix, client_id=self.conn.client_id)

This helps your test be more expressive on what the actual behavior is, and helps insulate your test against unnecessarily breaking when things change.

@dctrwatson dctrwatson added a commit that referenced this pull request Apr 11, 2012

@dctrwatson dctrwatson Merge pull request #8 from disqus/riakclient-tests
Riakclient tests
e06d177

@dctrwatson dctrwatson merged commit e06d177 into master Apr 11, 2012

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