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

can you merge/resolve with trunk, there's a conflict.
thanks,
-k

On Mon, Jul 2, 2012 at 6:31 PM, Benjamin Saller <email address hidden> wrote:

>
>
> 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#newcode1519
> juju/hooks/tests/test_invoker.py:1519: (yield
> unit_state.get_open_ports()),
> On 2012/07/01 02:32:22, hazmat wrote:
> > This test never verifies the implementation, namely that the container
> ports are
> > open.
>
> Right you are, thanks
>
>
> 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: """
> fair enough, removed
>
> On 2012/07/01 02:32:22, hazmat wrote:
> > 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#newcode2810
> juju/state/tests/test_service.py:2810: self.assertEqual(
> On 2012/07/01 02:32:22, hazmat wrote:
> > how is this entire test not entirely superfluous?
>
> removed
>
> https://codereview.appspot.com/6324045/
>
> --
> https://code.launchpad.net/~bcsaller/juju/subordinate-ports/+merge/111534
> You are subscribed to branch lp:juju.
>