Code review comment for lp://staging/~bjornt/charms/precise/landscape-client/container-relation-joined-test

Looks like an improvement to me, +1!

[0]

+ def get_environment_config(self):
+ """Get the shell environment variables."""
+ return self.environment

It feels a bit verbose to add a getter method for a public attribute. I'd rather make the attribute private or drop the method.

[1]

+class TestingJujuBroker(JujuBroker):

I'd probably rename this to FakeJujuBroker, for consistency with the naming that we generally use in the client and server code (and it matches the widely accepted terminology http://xunitpatterns.com/Fake%20Object.html)

TestingLandscapeBroker too.

[2]

+ self.landscape_broker = TestingLandscapeBroker(data_path=data_path)
+ self.landscape_broker._config.config = os.path.join(
+ data_path, "landscape.conf")
+ self.landscape_broker._config.write()

This boilerplate isn't great, and it also accesses the private _config attribute. I think one problem is that TestingLandscapeBroker tries to abstract away the interaction with the client, but it actually leaks it by returning the config object with get_client_config, which forces tests to manage such config object. I don't have an easy solution, so I think this good enough as a first step in a better direction.

[3]

+ with open(juju_info_path, "r") as juju_file:
+ juju_info = json.loads(juju_file.read())

You could use landscape.lib.fs.read_file here:

            juju_info = json.loads(read_file(juju_info_path))