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

needs to be lbox reproposed when making a modification for inline changes..
 just doing this one by hand...

nice catch re. test_invoker yield exe.ended

test_open_and_close_ports

- there are four open port calls and one close here, please ditch two of
the open ports, its repetitive, executing a process (slow), and not adding
any value.

setup_subordinate_defaults isn't used and should get yanked, additionally
it still has all the unesc. machine stuff. these sort of methods (excessive
default setup) cause slow tests, ie. almost every slow unit test we have
uses the status base setup even then though most check only a fraction of
it.

LGTM, with the above minors please merge.

-k

On Mon, Jul 2, 2012 at 7:34 PM, Kapil Thangavelu <
<email address hidden>> wrote:

> 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.
> >
>
> --
> https://code.launchpad.net/~bcsaller/juju/subordinate-ports/+merge/111534
> You are subscribed to branch lp:juju.
>