cf-charmers team mailing list archive

[Benjamin Saller <benjamin.saller@xxxxxxxxxxxxx>]

Thanks for doing this branch and for using lbox.

At this time however I'm not comfortable that the testing adds very much
to our confidence that the charm works.

Please read over my comments and we can talk about how better to
proceed.



https://codereview.appspot.com/88100045/diff/20001/Makefile
File Makefile (right):

https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode14
Makefile:14: sdeb: source
These targets, the comments in all, the deb building, don't make sense
at all in the context of a charm.

Additionally the pattern of replicating the deps to each charm, and
charm-helpers is becoming unattractive.

I am torn if we should alter the pattern such that the developer can see
from the readme that they should create their own virtual env, once in
their account, install the deps and use that single virtual env to run
all these tests.

https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode34
Makefile:34: python setup.py install --user
Again, this doesn't make sense, don't blindly copy things. One doesn't
install charms in their home directory as a python development project.

https://codereview.appspot.com/88100045/diff/20001/bin/README
File bin/README (right):

https://codereview.appspot.com/88100045/diff/20001/bin/README#newcode1
bin/README:1: This directory contains executables for accessing
cf-cloud-controller juju charm functionality
I don't think we need/want a README in the bin directory, top level of
the charm should have the readme

https://codereview.appspot.com/88100045/diff/20001/bin/test_setup
File bin/test_setup (right):

https://codereview.appspot.com/88100045/diff/20001/bin/test_setup#newcode9
bin/test_setup:9: # python setup.py develop
Last line very much isn't needed in a charm

https://codereview.appspot.com/88100045/diff/20001/tests/test_install.py
File tests/test_install.py (right):

https://codereview.appspot.com/88100045/diff/20001/tests/test_install.py#newcode31
tests/test_install.py:31: self.assertTrue(minstall.called)
This test mocks everything and proves nothing about the code or that it
functions as intended.

The need to use imp.load_source rather than import the module directly
indicates there are other structural issues as well.

Why doesn't the module import directly? It looks like that install's
code is contained in two methods that shouldn't be invoked on import.
What is being run such that the charm_dir is needed for the import to
even work? Can we fix that instead?

https://codereview.appspot.com/88100045/

-- 
https://code.launchpad.net/~lomov-as/charms/trusty/cf-cloud-controller/tests/+merge/215906
Your team Cloud Foundry Charmers is requested to review the proposed merge of lp:~lomov-as/charms/trusty/cf-cloud-controller/tests into lp:~cf-charmers/charms/trusty/cf-cloud-controller/trunk.