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

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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.

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.

+# 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. :-)

[2]

The branch is missing some pieces. zookeeper_test_context wasn't added, specifically.

review: Needs Fixing

« Back to merge proposal