Code review comment for lp:~lazypower/charms/precise/mediawiki/tests

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Thanks for this. I had a number of suggestions. Also if those .moved
files are in the repo that needs to be taken care of. After that is done
can you re-propose and I can approve it

https://codereview.appspot.com/54600043/diff/1/tests/00_setup.sh.moved
File tests/00_setup.sh.moved (right):

https://codereview.appspot.com/54600043/diff/1/tests/00_setup.sh.moved#newcode3
tests/00_setup.sh.moved:3: sudo apt-get install install amulet
python-requests
This file appears to be an artifact of some bzr op and should be removed

https://codereview.appspot.com/54600043/diff/1/tests/100_deploy_cloud.test
File tests/100_deploy_cloud.test (right):

https://codereview.appspot.com/54600043/diff/1/tests/100_deploy_cloud.test#newcode33
tests/100_deploy_cloud.test:33: search_term =
"\"{}\";".format(mysql_unit.info['public-address'])
Minor but its usually better to use single quotes for the string rather
than escape as the result is more readable

'"{}";'.format(...)

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

https://codereview.appspot.com/54600043/diff/1/tests/10_cache_relation_test.py#newcode16
tests/10_cache_relation_test.py:16: # Add mediawiki and mysql db server
This file and the 100_deploy_with_memcache seem redundant. Given that
theses tests are not free we should remove one.

In general for functional tests with spin up times I prefer to assert as
much as possible in a single test. As long as you don't depend on state
manipulation beyond the initial setup that is fine.

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

https://codereview.appspot.com/54600043/diff/1/tests/10_db_relation_test.py#newcode32
tests/10_db_relation_test.py:32:
Again this test is very similar to the deploy_cloud test. Is there a
reason to have twice as much code at twice the runtime cost? Can we
combine theses?

https://codereview.appspot.com/54600043/diff/1/tests/lib/helper.py
File tests/lib/helper.py (right):

https://codereview.appspot.com/54600043/diff/1/tests/lib/helper.py#newcode7
tests/lib/helper.py:7: 'Accept':
'text/html,application/xhtml+xml,application/xml;q=0.9,*/*',
as a style point defining this above the method as

LOGIN_HEADERS = {
...
}

can save you some indentation space and make things a little easier to
read.

https://codereview.appspot.com/54600043/diff/1/tests/lib/helper.py#newcode15
tests/lib/helper.py:15: def standup_unit(deployment, seconds=900):
deploy as a name? This could bring up more than one unit, no?

https://codereview.appspot.com/54600043/

« Back to merge proposal