cf-charmers team mailing list archive

[Benjamin Saller <benjamin.saller@xxxxxxxxxxxxx>]

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.

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.