Add tests to cf-cloud-controller

Add tests to cf-cloud-controller

This is my first time using lbox.

https://code.launchpad.net/~lomov-as/charms/trusty/cf-cloud-controller/tests/+merge/215906

(do not edit description out of merge proposal)

[lomov.as, 2014-04-15 15:44:36]

Please take a look.

[benjamin.saller, 2014-04-15 18:47:55]

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#newc...
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?

[lomov.as, 2014-04-16 11:04:18]

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

https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode14
Makefile:14: sdeb: source
I like idea of tests that can be run after you fetch directory and install
dependencies with one command (like `bin/test_setup` or `bundle install` in
ruby). In order to have such thing we can left current idea of `.whl` files in
`deps` folder or can move `deps` folder to separate repository and fetch it
inside `bin/test_setup` script. What do you think about it?

https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode14
Makefile:14: sdeb: source
On 2014/04/15 18:47:55, benjamin.saller wrote:
> 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.
> 

Done. `sdeb` task is removed in revision #29

https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode34
Makefile:34: python setup.py install --user
On 2014/04/15 18:47:55, benjamin.saller wrote:
> 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.

Done. `userinstall` task is removed in revision #29.

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
On 2014/04/15 18:47:55, benjamin.saller wrote:
> I don't think we need/want a README in the bin directory, top level of the
charm
> should have the readme

Done. This README removed in revision #29

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
On 2014/04/15 18:47:55, benjamin.saller wrote:
> Last line very much isn't needed in a charm

Done.

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#newc...
tests/test_install.py:31: self.assertTrue(minstall.called)
On 2014/04/15 18:47:55, benjamin.saller wrote:
> 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?
> 

1. yes, I needed to put lots of mocks here because cc_db_migrate works mostly
with system command line (`run and install` method). I will rethink this
approach.

2. I use imp.load_source module to access code in `install` hook that is located
in `hooks/install` file and doesn't have `.py` extension. Obviously we can link
`install.py` with `install` file, but I don't like this solution as it will add
additional requirements to our charm folder. I like solution with
`imp.load_source` better. Also we can move `cc_db_migrate` function to distinct
`.py` file and import it, not sure about it, because we use `cc_db_migrate`
function only inside `install` hook.

3. we can get read of mocking charm_dir before importing `install` hook. All we
need to do is to remove `charm_dir` call into `install_upstart_scripts`
initialization.

[lomov.as, 2014-04-18 19:28:56]

Please take a look.

[benjamin.saller, 2014-04-18 20:50:32]

I like the deps changes in this version. The makefile still needs some work. 

The test itself is largely unchanged. My previous concerns remain.

I'd rather have no test of install than one that mocks everything it does. If
the only test of install is a functional one then we can write it in that style
as an amulet test. 

hooks.py looks like it depends entirely on charm helpers. If there isn't code
here that lends itself well to unit testing thats ok, in the case of making
single method calls into helpers for hooks I'd say we're doing well. It reduces
some of the testing burden. 

I'd suggest you make the changes to the makefile and remove the install test for
now, then in another branch we can write it as an integration test with amulet,
where we can verify those changes on the deployed unit.

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

https://codereview.appspot.com/88100045/diff/40001/Makefile#newcode8
Makefile:8: @echo "make sdeb - Create debian source package"
remove unused targets from makefile

https://codereview.appspot.com/88100045/diff/40001/Makefile#newcode14
Makefile:14: deb: source
We don't need deb either, charms don't become packages

https://codereview.appspot.com/88100045/diff/40001/Makefile#newcode18
Makefile:18: scripts/update-revno
again, not needed, charms are not managed as python packages

https://codereview.appspot.com/88100045/diff/40001/Makefile#newcode27
Makefile:27: rm -rf deps
If you are arguing to keep deps with wheel then we shouldn't remove them.

https://codereview.appspot.com/88100045/diff/40001/Makefile#newcode40
Makefile:40: @echo Starting fast tests...
ftests would be integration tests in the case of a charm. Charm functional tests
are deployed and run with amulet. 
Remove this target as well.

https://codereview.appspot.com/88100045/diff/40001/Makefile#newcode45
Makefile:45: @flake8 --ignore=E123,E501 $(PROJECT) $(TESTS) && echo OK
We don't need the --ignore, we can follow pep8 properly.

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

https://codereview.appspot.com/88100045/diff/40001/bin/test_setup#newcode5
bin/test_setup:5: bzr checkout --lightweight
lp:~lomov-as/+junk/cf-charms-test-assets test-assets
This is a good idea, you should push this to ~cf-charmers though, not +junk if
we are going to use it in the charms.