Code review comment for lp:~jimbaker/pyjuju/zk-test-mgmt

On Fri, Nov 5, 2010 at 11:51 AM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
> Hey Jim,
>
> Here are some preliminary comments:
>
> [1]
>
> +"""This port can be overriden when testing"""
> +ZOOKEEPER_PORT = 2181

+
> +"""For convenience, this should be kept synchronized with the
> ZOOKEEPER_PORT"""
> +ZOOKEEPER_LOCAL_ADDRESS = "127.0.0.1:2181"
>

>
> We've managed to avoid global variables thus far, and it'd
> be nice if we could manage to maintain things that way. It's
> fine to have these defined for tests, though, but this is
> sitting outside of the test environment.
>

This is one reason it took a while to do this, but it seems better than the
alternative, which is to have "127.0.0.1:2181" (and variants) scattered
through our code.

>
> Also, this is minor, since we're talking about tests, but this
> redundancy feels a bit strange. You can just define this latter
> one and very easily obtain the port number out of it. That said,
> the port number isn't even used anywhere, so let's just get rid
> of it entirely.
>

Agreed port number is not currently being used. I was of two minds on this
because it seemed potentially useful for the future. Having one derived from
the other would be nicer, but property doesn't work on module-level names,
and spending engineering on it to wrap in a class seemed silly. Probably
best to remove.

>
> +# some standard dependency injection in the Java parlance we need to do
> +# to override our normal assumptions so we can test
>
> As a random comment, dependency injection is somewhat like
> the opposite of a global variable. :-)
>

Yeah, except in Python, being able to just change it is something like DI.
There's no need to write an accessor when you can just set an attribute, so
it works the same. (Real DI of course allows for more sophisticated sense of
context, but in this case global context seems to be just right.)

>
> [2]
>
> The branch is missing some pieces. zookeeper_test_context wasn't added,
> specifically.
>

This function is in ensemble.tests.common, which I have verified is in the
branch.

> --
> https://code.launchpad.net/~jimbaker/ensemble/zk-test-mgmt/+merge/40160
> You are the owner of lp:~jimbaker/ensemble/zk-test-mgmt.
>