Code review comment for lp:~bcsaller/pyjuju/subordinate-ports

while the implementation LGTM, the tests need fixing.

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode37
juju/hooks/tests/test_invoker.py:37: self._invokers = {} # client_id ->
Invoker
pep8 need an extra space before comment

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode1519
juju/hooks/tests/test_invoker.py:1519: (yield
unit_state.get_open_ports()),
This test never verifies the implementation, namely that the container
ports are open.

https://codereview.appspot.com/6324045/diff/1/juju/hooks/tests/test_invoker.py#newcode1560
juju/hooks/tests/test_invoker.py:1560: "closed 80/tcp"])
pep8 need an extra blank line after this test

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py
File juju/state/tests/test_firewall.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_firewall.py#newcode192
juju/state/tests/test_firewall.py:192: """
this test also seems superfluous. the only impl file being modified by
the branch is the invoker, yet we have tests in numerous state test
files verifying what has already been verified by other tests in those
files

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py
File juju/state/tests/test_service.py (right):

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode176
juju/state/tests/test_service.py:176: m1 = yield
self.machine_state_manager.add_machine_state(
machine creation and assignment is superflous

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode2771
juju/state/tests/test_service.py:2771: def
test_get_open_ports_subordiate(self):
typo in test name

https://codereview.appspot.com/6324045/diff/1/juju/state/tests/test_service.py#newcode2810
juju/state/tests/test_service.py:2810: self.assertEqual(
how is this entire test not entirely superfluous?

https://codereview.appspot.com/6324045/