Merge lp:~jimbaker/pyjuju/zk-test-mgmt into lp:pyjuju

[Jim Baker]

Manage the running of a test ZooKeeper instance during Twisted Trial execution:

  - Create temp directory with all required files for ZooKeeper to run
  - Work with either dev build or release versions of ZooKeeper
  - Using subprocess, start ZooKeeper, then terminate and cleanup upon
    the end of a test

Modified the following code:

  - test, via the use of a context manager that encapsulates the actual Trial run
  - ensemble/__init__.py, to add the ZK address information
  - unit tests and classes referring to "2181" in their source code

Fixes #641482

[Gustavo Niemeyer (niemeyer)]

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

[Jim Baker (jimbaker)]

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.
>

[Gustavo Niemeyer (niemeyer)]

[1]

> 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.

It's better to have an internal default value which is used by the specific class
which connects to zookeeper, than having a public value like that which is used
through out the code and directly modified by clients, for all the reasons why
global variables are bad (common state rather than local, side effects across
the board, etc).

If you want to define that for tests only, then that certainly feels ok, but can
we please have that under the test namespace rather than the main package one?

> 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.

Thanks!

> > 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.

This is a quite orthogonal point to the whole review, but having a global
variable like this is pretty much the same thing as having a "public static"
member in an "Ensemble" class, which (other :-) Java advocates won't really
buy as DI by itself.

> 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.)

Global state is bad in pretty much all cases. Imagine that we want to connect
to two different servers to interconnect them. We want to be able to build
model objects with local state, that do not depend on a unique source of
information for the server address.

Dependency injection might fail in this case as well, in fact, which is one
of the reasons why I'm not a big fan of it either. The context should
generally be provided to the objects on an instance basis, rather than
having objects configured on a class level.

[2]

Yeah, I'm sorry about this. I've screwed it up locally. Will follow up with
the continuation shortly.

review: Needs Fixing

[Gustavo Niemeyer (niemeyer)]

That looks awesome, thanks a lot!

In addition to the above, have only one significant comment, and
two minors. +1 one considering these.

[3]

I have two test failures within the branch, but all tests pass with trunk.

I've pasted the errors to: https://pastebin.canonical.com/39455/

[4]

+dataDir=%s
+clientPort=%s
+""" % (os.path.join(self.working_path, "data"), self.port))

It'd probably safer to put the data under a subdirectory of working_path,
as you have done for logs. This will avoid any potential conflicts, and
isolate the various data files in a single place.

[5]

+ with zookeeper_test_context(
+ os.environ["ZOOKEEPER_PATH"],
+ os.environ.get("ZOOKEEPER_TEST_PORT", 28181)):
+ run()

As a silly minor, would you mind to indent the second and third lines in by
another 4 spaces or something similar? It feels more readable to not have
the statement continuation aligned with the actual block content.

review: Approve

[Jim Baker (jimbaker)]

On Fri, Nov 5, 2010 at 2:29 PM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Approve
> That looks awesome, thanks a lot!
>
> I will commit to trunk momentarily, see my other comments.

> In addition to the above, have only one significant comment, and
> two minors. +1 one considering these.
>
> [3]
>
> I have two test failures within the branch, but all tests pass with trunk.
>
> I've pasted the errors to: https://pastebin.canonical.com/39455/

These errors occasionally happen on trunk checkout too. They should be fixed
of course, but not related specifically to this branch.

>
>
> [4]
>
> +dataDir=%s
> +clientPort=%s
> +""" % (os.path.join(self.working_path, "data"), self.port))
>
> It'd probably safer to put the data under a subdirectory of working_path,
> as you have done for logs. This will avoid any potential conflicts, and
> isolate the various data files in a single place.
>

Actually this is a separate subdirectory, but I have made clearer in the
current code its setup.

>
> [5]
>
> + with zookeeper_test_context(
> + os.environ["ZOOKEEPER_PATH"],
> + os.environ.get("ZOOKEEPER_TEST_PORT", 28181)):
> + run()
>
> As a silly minor, would you mind to indent the second and third lines in by
> another 4 spaces or something similar? It feels more readable to not have
> the statement continuation aligned with the actual block content.
>

Done.

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

[Jim Baker (jimbaker)]

On Fri, Nov 5, 2010 at 2:08 PM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Needs Fixing
> [1]
>
> > 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.
>
> It's better to have an internal default value which is used by the specific
> class
> which connects to zookeeper, than having a public value like that which is
> used
> through out the code and directly modified by clients, for all the reasons
> why
> global variables are bad (common state rather than local, side effects
> across
> the board, etc).
>
> If you want to define that for tests only, then that certainly feels ok,
> but can
> we please have that under the test namespace rather than the main package
> one?
>

I have move the address lookup to
ensemble.tests.common.get_test_zookeeper_address. In non-test code, this is
instead driven by an optional new env var, ZOOKEEPER_ADDRESS. This is set by
the test process, and is used in two places in non-test code:

1. In the construction of the client in ensemble.control.initialize.command,
then passed down as in trunk
2. In ensemble.providers.dummy.MachineProvider.connect (and this is really
just test code too, just put in parallel)

>
> > 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.
>
> Thanks!
>
>
Done

=== modified file 'ensemble/__init__.py'
--- ensemble/__init__.py	2010-05-04 14:11:23 +0000
+++ ensemble/__init__.py	2010-11-08 23:03:47 +0000
@@ -0,0 +1,1 @@
+#

=== modified file 'ensemble/agents/tests/common.py'
--- ensemble/agents/tests/common.py	2010-09-20 19:53:57 +0000
+++ ensemble/agents/tests/common.py	2010-11-08 23:03:47 +0000
@@ -7,6 +7,7 @@
 from ensemble.agents.base import TwistedOptionNamespace
 from ensemble.environment.config import EnvironmentsConfig
 from ensemble.state.tests.common import StateTestBase
+from ensemble.tests.common import get_test_zookeeper_address
 
 
 SAMPLE_ENV = """\
@@ -52,7 +53,7 @@
 
     def get_agent_config(self):
         options = TwistedOptionNamespace()
-        options["zookeeper_servers"] = "localhost:2181"
+        options["zookeeper_servers"] = get_test_zookeeper_address()
         return succeed(options)
 
     @inlineCallbacks

=== modified file 'ensemble/agents/tests/test_base.py'
--- ensemble/agents/tests/test_base.py	2010-09-15 19:53:52 +0000
+++ ensemble/agents/tests/test_base.py	2010-11-08 23:03:47 +0000
@@ -10,11 +10,13 @@
 from txzookeeper import ZookeeperClient
 from ensemble.lib.testing import TestCase
 from ensemble.lib.mocker import MATCH
+from ensemble.tests.common import get_test_zookeeper_address
 
 from ensemble.agents.base import (
     BaseAgent, TwistedOptionNamespace, AgentRunner)
 from ensemble.errors import NoConnection
 
+
 MATCH_APP = MATCH(lambda x: isinstance(x, Componentized))
 
 
@@ -105,13 +107,13 @@
         This will create an agent instance,  parse the cli args, passes them to
         the agent, and starts the agent runner.
         """
-        self.change_args("es-agent", "--zookeeper-servers", "localhost:2181")
+        self.change_args("es-agent", "--zookeeper-servers", get_test_zookeeper_address())
         runner = self.mocker.patch(AgentRunner)
         runner.run()
         mock_agent = self.mocker.patch(BaseAgent)
 
         def match_args(config):
-            self.assertEqual(config["zookeeper_servers"], "localhost:2181")
+            self.assertEqual(config["zookeeper_servers"], get_test_zookeeper_address())
             return True
 
         mock_agent.configure(MATCH(match_args))
@@ -143,7 +145,7 @@
 
         pid_file = self.makeFile()
         self.change_args(
-            "es-agent", "--zookeeper-servers", "localhost:2181",
+            "es-agent", "--zookeeper-servers", get_test_zookeeper_address(),
             "--pidfile", pid_file)
         runner = self.mocker.patch(AgentRunner)
         logger = self.mocker.patch(AppLogger)
@@ -218,13 +220,13 @@
     def test_run_base_raises_error(self):
         """The base class agent, raises a notimplemented error when started."""
         client = self.mocker.patch(ZookeeperClient)
-        client.connect("localhost:2181")
+        client.connect(get_test_zookeeper_address())
         client_mock = self.mocker.mock()
         self.mocker.result(succeed(client_mock))
         self.mocker.replay()
 
         agent = BaseAgent()
-        agent.configure({"zookeeper_servers": "localhost:2181"})
+        agent.configure({"zookeeper_servers": get_test_zookeeper_address()})
         d = agent.startService()
         self.failUnlessFailure(d, NotImplementedError)
         return d

=== modified file 'ensemble/agents/tests/test_machine.py'
--- ensemble/agents/tests/test_machine.py	2010-10-14 19:54:48 +0000
+++ ensemble/agents/tests/test_machine.py	2010-11-08 23:03:47 +0000
@@ -17,6 +17,8 @@
 from ensemble.state.environment import EnvironmentStateManager
 from ensemble.state.machine import MachineStateManager, MachineState
 from ensemble.state.service import ServiceStateManager
+from ensemble.tests.common import get_test_zookeeper_address
+
 
 from .common import AgentTestBase
 
@@ -169,7 +171,7 @@
         yield self.agent.start()
 
         mock_deployment = self.mocker.patch(ServiceUnitDeployment)
-        mock_deployment.start("0", "localhost:2181")
+        mock_deployment.start("0", get_test_zookeeper_address())
 
         test_deferred = Deferred()
 
@@ -225,7 +227,7 @@
         test_deferred = Deferred()
 
         mock_deployment = self.mocker.patch(ServiceUnitDeployment)
-        mock_deployment.start("0", "localhost:2181")
+        mock_deployment.start("0", get_test_zookeeper_address())
         self.mocker.result(succeed(True))
         mock_deployment.destroy()
 

=== modified file 'ensemble/control/initialize.py'
--- ensemble/control/initialize.py	2010-09-22 20:28:49 +0000
+++ ensemble/control/initialize.py	2010-11-08 23:03:47 +0000
@@ -1,3 +1,5 @@
+import os
+
 from twisted.internet.defer import inlineCallbacks
 from twisted.internet import reactor
 
@@ -22,7 +24,9 @@
     """
     Initialize Zookeeper hierarchy
     """
-    client = yield ZookeeperClient("localhost:2181").connect()
+    zk_address = os.environ.get("ZOOKEEPER_ADDRESS", "127.0.0.1:2181")
+    print "Using ZK address", zk_address
+    client = yield ZookeeperClient(zk_address).connect()
     try:
         hierarchy = StateHierarchy(
             client, options.admin_identity, options.instance_id)

=== modified file 'ensemble/formula/tests/test_publisher.py'
--- ensemble/formula/tests/test_publisher.py	2010-10-14 16:08:48 +0000
+++ ensemble/formula/tests/test_publisher.py	2010-11-08 23:03:47 +0000
@@ -14,6 +14,7 @@
 from ensemble.providers.dummy import FileStorage
 from ensemble.state.formula import FormulaStateManager
 from ensemble.state.errors import StateChanged
+from ensemble.tests.common import get_test_zookeeper_address
 
 from ensemble.environment.tests.test_config import (
     EnvironmentsConfigTestBase, SAMPLE_ENV)
@@ -37,7 +38,7 @@
         self.formula_storage_key = "%s:%s" % (
             self.formula_key, self.formula.get_sha256())
 
-        self.client = ZookeeperClient("127.0.0.1:2181")
+        self.client = ZookeeperClient(get_test_zookeeper_address())
         self.storage = FileStorage(self.makeDir())
         self.publisher = FormulaPublisher(self.client, self.storage)
 

=== modified file 'ensemble/ftests/test_connection.py'
--- ensemble/ftests/test_connection.py	2010-09-29 20:31:59 +0000
+++ ensemble/ftests/test_connection.py	2010-11-08 23:03:47 +0000
@@ -12,10 +12,10 @@
 import pwd
 import zookeeper
 
-
 from ensemble.errors import NoConnection
 from ensemble.lib.testing import TestCase
 from ensemble.state.sshclient import SSHClient
+from ensemble.tests.common import get_test_zookeeper_address
 
 
 class ConnectionTest(TestCase):
@@ -39,7 +39,7 @@
         Forwarding a port spawns an ssh process with port forwarding arguments.
         """
         connect_deferred = self.client.connect(
-            "localhost:2181", timeout=20)
+            get_test_zookeeper_address(), timeout=20)
 
         def validate_connected(client):
             self.assertTrue(client.connected)
@@ -80,7 +80,7 @@
         """
         SSHClient.remote_user = "rabbit"
         connect_deferred = self.client.connect(
-            "localhost:2181", timeout=10)
+            get_test_zookeeper_address(), timeout=10)
         self.failUnlessFailure(connect_deferred, NoConnection)
 
         def validate_log(result):

=== modified file 'ensemble/machine/tests/test_unit_deployment.py'
--- ensemble/machine/tests/test_unit_deployment.py	2010-11-02 16:27:50 +0000
+++ ensemble/machine/tests/test_unit_deployment.py	2010-11-08 23:03:47 +0000
@@ -15,6 +15,8 @@
 from ensemble.lib.mocker import MATCH
 from ensemble.machine.unit import ServiceUnitDeployment
 from ensemble.machine.errors import UnitDeploymentError
+from ensemble.tests.common import get_test_zookeeper_address
+
 
 MATCH_PROTOCOL = MATCH(lambda x: isinstance(x, ProcessProtocol))
 
@@ -67,7 +69,7 @@
         if it does not exist and a running service unit agent.
         """
         self.deployment.unpack_formula(self.bundle)
-        d = self.deployment.start("0", "localhost:2181")
+        d = self.deployment.start("0", get_test_zookeeper_address())
 
         @inlineCallbacks
         def validate_result(result):
@@ -87,7 +89,7 @@
         if it does not exist and a running service unit agent.
         """
         self.deployment.unpack_formula(self.bundle)
-        d = self.deployment.start(21, "localhost:2181")
+        d = self.deployment.start(21, get_test_zookeeper_address())
 
         @inlineCallbacks
         def validate_result(result):
@@ -108,7 +110,7 @@
         """
         self.deployment.unit_agent_module = "magichat.xr1"
         self.deployment.unpack_formula(self.bundle)
-        d = self.deployment.start("0", "localhost:2181")
+        d = self.deployment.start("0", get_test_zookeeper_address())
 
         self.failUnlessFailure(d, UnitDeploymentError)
 
@@ -165,7 +167,7 @@
         """
         error = self.assertRaises(
             UnitDeploymentError,
-            self.deployment.start, "0", "localhost:2181")
+            self.deployment.start, "0", get_test_zookeeper_address())
         self.assertIn("Formula must be unpacked first.", str(error))
 
     @inlineCallbacks
@@ -175,7 +177,7 @@
         on the machine, and kills the unit agent process.
         """
         self.deployment.unpack_formula(self.bundle)
-        yield self.deployment.start("0", "localhost:2181")
+        yield self.deployment.start("0", get_test_zookeeper_address())
         # give the process time to write its pid file
         yield self.sleep(0.1)
         pid = int(open(self.deployment.pid_file).read())

=== modified file 'ensemble/providers/dummy.py'
--- ensemble/providers/dummy.py	2010-09-30 00:31:07 +0000
+++ ensemble/providers/dummy.py	2010-11-08 23:03:47 +0000
@@ -5,10 +5,10 @@
 from txzookeeper import ZookeeperClient
 from twisted.internet.defer import succeed, fail
 
+import ensemble
 from ensemble.errors import FileNotFound, ProviderError
 from ensemble.machine import ProviderMachine
 
-
 class DummyMachine(ProviderMachine):
     """Provider machine implementation specific to the dummy provider."""
 
@@ -23,7 +23,7 @@
 
     def connect(self):
         """Connect to the zookeeper ensemble running in the machine provider"""
-        return ZookeeperClient("127.0.0.1:2181").connect()
+        return ZookeeperClient(os.environ.get("ZOOKEEPER_ADDRESS", "127.0.0.1:2181")).connect()
 
     def list_machines(self):
         """List all the machine running in the provider."""

=== modified file 'ensemble/providers/ec2/tests/test_connect.py'
--- ensemble/providers/ec2/tests/test_connect.py	2010-10-14 21:14:05 +0000
+++ ensemble/providers/ec2/tests/test_connect.py	2010-11-08 23:03:47 +0000
@@ -20,6 +20,7 @@
 from ensemble.providers.ec2.tests.common import EC2TestMixin
 from ensemble.providers.ec2.connect import EC2Connect
 from ensemble.errors import ProviderInteractionError
+from ensemble.tests.common import get_test_zookeeper_address
 
 
 class EC2ConnectTest(EC2TestMixin, TestCase):
@@ -154,7 +155,7 @@
         self.mocker.replay()
 
         zookeeper.set_debug_level(0)
-        yield connected_client.connect("localhost:2181")
+        yield connected_client.connect(get_test_zookeeper_address())
 
         client_result = []
 

=== modified file 'ensemble/providers/ec2/tests/test_launch.py'
--- ensemble/providers/ec2/tests/test_launch.py	2010-10-15 21:30:31 +0000
+++ ensemble/providers/ec2/tests/test_launch.py	2010-11-08 23:03:47 +0000
@@ -13,6 +13,7 @@
 from ensemble.providers.common import (
     DEFAULT_REPOSITORIES, DEFAULT_PACKAGES, BOOTSTRAP_PACKAGES)
 from ensemble.state.auth import make_identity
+from ensemble.tests.common import get_test_zookeeper_address
 
 from ensemble.lib.testing import TestCase
 from ensemble.lib.mocker import MATCH
@@ -167,7 +168,7 @@
         launch = EC2LaunchMachine(provider)
         variables = launch.get_machine_variables(
             machine_data={"machine-id": "machine-2",
-                          "ensemble-zookeeper-hosts": "127.0.0.1:2181"})
+                          "ensemble-zookeeper-hosts": get_test_zookeeper_address()})
         cmd = "cd /usr/lib/ensemble/ensemble && sudo /usr/bin/bzr switch %s"%(
             custom_branch)
         self.failUnlessIn(cmd, variables["scripts"])

=== modified file 'ensemble/providers/tests/test_dummy.py'
--- ensemble/providers/tests/test_dummy.py	2010-09-30 00:31:07 +0000
+++ ensemble/providers/tests/test_dummy.py	2010-11-08 23:03:47 +0000
@@ -113,8 +113,7 @@
 
     def setUp(self):
         directory = self.makeDir()
-        self.provider = MachineProvider(
-            "foo", {"storage-directory": directory})
+        self.provider = MachineProvider("foo", {"storage-directory": directory})
         self.storage = self.provider.get_file_storage()
 
     def test_get_file_non_existant(self):

=== modified file 'ensemble/state/tests/common.py'
--- ensemble/state/tests/common.py	2010-10-21 12:12:26 +0000
+++ ensemble/state/tests/common.py	2010-11-08 23:03:47 +0000
@@ -10,6 +10,7 @@
 from ensemble.formula.tests.test_directory import sample_directory
 from ensemble.formula.directory import FormulaDirectory
 from ensemble.state.topology import InternalTopology
+from ensemble.tests.common import get_test_zookeeper_address
 
 
 class StateTestBase(TestCase):
@@ -18,7 +19,7 @@
     def setUp(self):
         zookeeper.set_debug_level(0)
         self.formula = FormulaDirectory(sample_directory)
-        self.client = ZookeeperClient("localhost:2181")
+        self.client = ZookeeperClient(get_test_zookeeper_address())
 
         yield self.client.connect()
         yield self.client.create("/formulas")
@@ -32,7 +33,7 @@
         # Close and reopen connection, so that watches set during
         # testing are not affected by the cleaning up.
         self.client.close()
-        client = ZookeeperClient("localhost:2181")
+        client = ZookeeperClient(get_test_zookeeper_address())
         yield client.connect()
         deleteTree(handle=client.handle)
         client.close()

=== modified file 'ensemble/state/tests/test_initialize.py'
--- ensemble/state/tests/test_initialize.py	2010-09-22 18:59:04 +0000
+++ ensemble/state/tests/test_initialize.py	2010-11-08 23:03:47 +0000
@@ -9,6 +9,7 @@
 from ensemble.state.initialize import StateHierarchy
 from ensemble.state.machine import MachineStateManager
 from ensemble.lib.testing import TestCase
+from ensemble.tests.common import get_test_zookeeper_address
 
 
 class LayoutTest(TestCase):
@@ -16,7 +17,7 @@
     def setUp(self):
         self.log = self.capture_logging("ensemble.state.init")
         zookeeper.set_debug_level(0)
-        self.client = ZookeeperClient("localhost:2181")
+        self.client = ZookeeperClient(get_test_zookeeper_address())
         self.identity = make_identity("admin:genie")
         self.layout = StateHierarchy(
             self.client, self.identity, "i-abcdef")

=== added file 'ensemble/tests/common.py'
--- ensemble/tests/common.py	1970-01-01 00:00:00 +0000
+++ ensemble/tests/common.py	2010-11-08 23:03:47 +0000
@@ -0,0 +1,152 @@
+import logging
+import os
+import os.path
+import shutil
+import subprocess
+import sys
+import tempfile
+
+from contextlib import contextmanager
+from glob import glob
+
+
+__all__ = ("ManagedZooKeeper",
+           "zookeeper_test_context",
+           "get_zookeeper_test_address")
+
+log = logging.getLogger("ensemble.tests.common")
+
+
+class ManagedZooKeeper(object):
+    """Class to manage the running of a ZooKeeper instance for testing.
+    
+    Note: no attempt is made to probe the ZooKeeper instance is
+    actually available, or that the selected port is free. In the
+    future, we may want to do that, especially when run in a
+    Hudson/Buildbot context, to ensure more test robustness."""
+
+    def __init__(self, install_path, port=28181):
+        """Define the ZooKeeper test instance.
+
+        @param install_path: The path to the install for ZK
+        @param port: The port to run the managed ZK instance
+        """
+
+        self.install_path = install_path
+        self.port = port
+        self.host = "127.0.0.1"
+
+    def run(self):
+        """Run the ZooKeeper instance under a temporary directory.
+
+        Writes ZK log messages to zookeeper.log in the current directory.
+        """
+        
+        self.working_path = tempfile.mkdtemp()
+        log.info("Starting test zookeeper: installed=%s, port=%s",
+                 self.install_path, self.port)
+
+        config_path = os.path.join(self.working_path, "zoo.cfg")
+        log_path = os.path.join(self.working_path, "log")
+        log4j_path = os.path.join(self.working_path, "log4j.properties")
+        data_path = os.path.join(self.working_path, "data")
+
+        # various setup steps
+        os.mkdir(log_path)
+
+        with open(config_path, "w") as config:
+            config.write("""
+tickTime=2000
+dataDir=%s
+clientPort=%s
+""" % (data_path, self.port)) 
+
+        with open(log4j_path, "w") as log4j:
+            log4j.write("""
+# DEFAULT: console appender only
+log4j.rootLogger=INFO, ROLLINGFILE
+log4j.appender.ROLLINGFILE.layout=org.apache.log4j.PatternLayout
+log4j.appender.ROLLINGFILE.layout.ConversionPattern=%d{ISO8601} [myid:%X{myid}] - %-5p [%t:%C{1}@%L] - %m%n
+log4j.appender.ROLLINGFILE=org.apache.log4j.RollingFileAppender
+log4j.appender.ROLLINGFILE.Threshold=DEBUG
+log4j.appender.ROLLINGFILE.File=zookeeper.log
+""")
+
+        self.process = subprocess.Popen(
+            args=["java",
+                  "-cp", self.classpath,
+                  "-Dzookeeper.log.dir=%s" % log_path,
+                  "-Dzookeeper.root.logger=INFO,CONSOLE",
+                  "-Dlog4j.configuration=file:%s" % log4j_path,
+                  # "-Dlog4j.debug",
+                  "org.apache.zookeeper.server.quorum.QuorumPeerMain",
+                  config_path],
+            )
+
+    @property
+    def classpath(self):
+        """Get the classpath necessary to run ZooKeeper."""
+
+        # Two possibilities, as seen in zkEnv.sh:
+        # Check for a release - top-level zookeeper-*.jar?
+        jars = glob((os.path.join(
+            self.install_path, 'zookeeper-*.jar')))
+        if jars:
+            # Relase build (`ant package`)
+            jars.extend(glob(os.path.join(
+                self.install_path,
+                "lib/*.jar")))
+        else:
+            # Development build (plain `ant`)
+            jars = glob((os.path.join(
+                self.install_path, 'build/zookeeper-*.jar')))
+            jars.extend(glob(os.path.join(
+                self.install_path,
+                "build/lib/*.jar")))
+        return ":".join(jars)
+
+    @property
+    def address(self):
+        """Get the address of the ZooKeeper instance."""
+        return "%s:%s" % (self.host, self.port)
+
+    def stop(self):
+        """Stop the ZooKeeper instance and cleanup."""
+        self.process.terminate()
+        shutil.rmtree(self.working_path)
+
+
+"""Global to manage the ZK test address - only for testing of course!"""
+_zookeeper_address = "127.0.0.1:2181"
+
+
+def get_test_zookeeper_address():
+    """Get the current test ZK address, such as '127.0.0.1:2181'"""
+    return _zookeeper_address
+
+
+@contextmanager
+def zookeeper_test_context(install_path, port=28181):
+    """Manage context to run/stop a ZooKeeper for testing and related vars.
+
+    @param install_path: The path to the install for ZK
+    @param port: The port to run the managed ZK instance
+    """
+    global _zookeeper_address
+    
+    saved_zookeeper_address = _zookeeper_address
+    saved_env = os.environ.get("ZOOKEEPER_ADDRESS")
+    test_zookeeper = ManagedZooKeeper(install_path, port)
+    test_zookeeper.run()
+    os.environ["ZOOKEEPER_ADDRESS"] = _zookeeper_address = test_zookeeper.address
+    try:
+        yield test_zookeeper
+    finally:
+        test_zookeeper.stop()
+        _zookeeper_address = saved_zookeeper_address
+        if saved_env:
+            os.environ["ZOOKEEPER_ADDRESS"] = saved_env
+        else:
+            del os.environ["ZOOKEEPER_ADDRESS"]
+
+

=== modified file 'test'
--- test	2010-07-08 22:47:52 +0000
+++ test	2010-11-08 23:03:47 +0000
@@ -1,12 +1,17 @@
 #!/usr/bin/env python
+import os
 import sys
-import os
 from twisted.scripts.trial import run
 
+from ensemble.tests.common import zookeeper_test_context
 
 FUNCTIONAL = '--functional'
 
 def main(args):
+    if not os.environ["ZOOKEEPER_PATH"]:
+        print "Environment variable ZOOKEEPER_PATH must be defined"
+        exit()
+    
     matched = [arg for arg in args if arg.startswith("ensemble")]
 
     if FUNCTIONAL in sys.argv:
@@ -20,8 +25,11 @@
         packages.remove("ftests")
         sys.argv.extend(
             ["ensemble.%s"%p for p in packages])
-    run()
+
+    with zookeeper_test_context(
+            os.environ["ZOOKEEPER_PATH"],
+            os.environ.get("ZOOKEEPER_TEST_PORT", 28181)):
+        run()
 
 if __name__ == "__main__":
     main(sys.argv[1:])
-