Code review comment for lp://qastaging/~mbruzek/charms/precise/rabbitmq-server/tests

Revision history for this message
Marco Ceppi (marcoceppi) wrote :

Great start, see my comments below

https://codereview.appspot.com/51230045/diff/20001/tests/10_basic_deploy_test.py
File tests/10_basic_deploy_test.py (right):

https://codereview.appspot.com/51230045/diff/20001/tests/10_basic_deploy_test.py#newcode14
tests/10_basic_deploy_test.py:14: file_path = os.path.join(path,
'rabbit-server-privkey.pem')
You're sending SSL certs but never verify if they're actually working.
It'd be nice if this test also attempted an SSL connection to the
rabbitmq server and verify the certs were installed and setup properly

https://codereview.appspot.com/51230045/diff/20001/tests/20_deploy_relations_test.py
File tests/20_deploy_relations_test.py (right):

https://codereview.appspot.com/51230045/diff/20001/tests/20_deploy_relations_test.py#newcode39
tests/20_deploy_relations_test.py:39:
d.sentry.unit['rabbitmq-server/0'].relation('ceph', 'ceph:client')
While testing the ceph relation is important, you should also test the
amqp relation, as that's the primary purpose of this charm

https://codereview.appspot.com/51230045/diff/20001/tests/30_configuration_test.py
File tests/30_configuration_test.py (right):

https://codereview.appspot.com/51230045/diff/20001/tests/30_configuration_test.py#newcode54
tests/30_configuration_test.py:54: message = 'The certificate did not
match!'
Simply being able to check one of the several configuration options
isn't enough for a complete test. You should get the contents of the
configuration file (or if possible, query rabbitmq directly) to verify
the charm is doing the right thing

https://codereview.appspot.com/51230045/

« Back to merge proposal