Added amulet tests for haproxy charm.

Added amulet tests for haproxy charm.



https://code.launchpad.net/~mbruzek/charms/precise/haproxy/trunk/+merge/202945

(do not edit description out of merge proposal)

[mbruzek, 2014-01-23 20:30:11]

Please take a look.

[benjamin.saller, 2014-01-23 20:54:52]

LGTM with minors

some suggestions below. Thanks for this.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py
File tests/10_deploy_test.py (right):

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode19
tests/10_deploy_test.py:19: #d.add('apache2', units=units)
comment and commented out code can be removed.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode24
tests/10_deploy_test.py:24: template_path = os.path.join(path,
'default_apache.tmpl')
I think the preferred way of doing this is with pkg_resources:

import pkg_resources
my_data = pkg_resources.resource_string(__name__, "default_apache.tmp")

which would return the contents of the package relative file as a string.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode29
tests/10_deploy_test.py:29: configuration = {'vhost_https_template' :
encodedTemplate.decode('ascii')}
Were you really seeing issues that forced you down the path of acsii
encode/decodes?

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode65
tests/10_deploy_test.py:65: print('Private address of the apache2 relation ',
relation['private-address'])
I think we can remove the print here, unless you think its worth keeping.

[mbruzek, 2014-01-24 15:32:13]

Thanks for the review Benjamin.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py
File tests/10_deploy_test.py (right):

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode19
tests/10_deploy_test.py:19: #d.add('apache2', units=units)
Thanks for the comment, removed the comments.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode24
tests/10_deploy_test.py:24: template_path = os.path.join(path,
'default_apache.tmpl')
I was not aware of pkg_resources. I get an error when I try that code in
python3.

Python 3.3.3 (default, Jan 11 2014, 05:48:01) 
[GCC 4.8.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pkg_resources
>>> template = pkg_resources.resource_string(__name__, "default_apache.tmpl")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3/dist-packages/pkg_resources.py", line 891, in
resource_string
    self, resource_name
  File "/usr/lib/python3/dist-packages/pkg_resources.py", line 1292, in
get_resource_string
    return self._get(self._fn(self.module_path, resource_name))
  File "/usr/lib/python3/dist-packages/pkg_resources.py", line 1370, in _get
    "Can't perform this operation for loaders without 'get_data()'"
NotImplementedError: Can't perform this operation for loaders without
'get_data()'

Seems to work fine in python2.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode29
tests/10_deploy_test.py:29: configuration = {'vhost_https_template' :
encodedTemplate.decode('ascii')}
You are right I can convert (encode) the template with utf-8 but all the
character set of base65 can be represented in ascii, so I will keep that on the
decode.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode65
tests/10_deploy_test.py:65: print('Private address of the apache2 relation ',
relation['private-address'])
I really like having the print there for manual testing.  It does not create too
much extra logging I would prefer to keep it.

[mbruzek, 2014-01-24 16:58:51]

Please take a look.

[mbruzek, 2014-01-24 16:59:48]

Please take a look.

[Marco Ceppi, 2014-02-07 12:28:27]

These LGTM, there's a suggestion as to a more...bashonic? bashinista? Anyways
these will be merged shortly

https://codereview.appspot.com/56140043/diff/60001/tests/00_setup.sh
File tests/00_setup.sh (right):

https://codereview.appspot.com/56140043/diff/60001/tests/00_setup.sh#newcode9
tests/00_setup.sh:9: if [ $? -ne 0 ]; then
This is really just a knitpick, but you can combine these two lines to just

if ! dpkg -s amulet 2>/dev/null; then

If you drop the [] and just run a command the if statement will be based on the
exit status. so 0 is true otherwise false. The 2>/dev/null is just to redirect
the stderr (really optional and not a big deal).