Merge lp://staging/~bjornt/charms/precise/landscape-client/container-relation-joined-test into lp://staging/charms/landscape-client
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Björn Tillenius | ||||
Approved revision: | 44 | ||||
Merge reported by: | Björn Tillenius | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp://staging/~bjornt/charms/precise/landscape-client/container-relation-joined-test | ||||
Merge into: | lp://staging/charms/landscape-client | ||||
Diff against target: |
456 lines (+215/-146) 4 files modified
hooks/common.py (+104/-100) hooks/container-relation-joined (+0/-26) hooks/hooks.py (+41/-19) hooks/test_hooks.py (+70/-1) |
||||
To merge this branch: | bzr merge lp://staging/~bjornt/charms/precise/landscape-client/container-relation-joined-test | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Chris Glass | Approve | ||
Free Ekanayaka (community) | Approve | ||
Review via email:
|
Description of the change
Add a basic test for the container-
The purpose of this branch isn't to add full test coverage for the
container-
tests for this hooks (and other hooks as well, of course). I added a
simple test to show that it's indeed possible to test it.
This is just a suggestion on how testing can be done. I'm open to
suggestion of better solutions. What I did was to extract the parts that
talk to Juju and Landscape, to make it possible to override that
functionality. I also made the hooks return an exit status, instead of
exiting directly in the hook itself.
I considered using mocker to mock out everything, but we would have to
mock out too much, so the tests would be hard to write and understand.
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 TestingJujuBrok er(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)
TestingLandscap eBroker too.
[2]
+ self.landscape_ broker = TestingLandscap eBroker( data_path= data_path) broker. _config. config = os.path.join( broker. _config. write()
+ self.landscape_
+ data_path, "landscape.conf")
+ self.landscape_
This boilerplate isn't great, and it also accesses the private _config attribute. I think one problem is that TestingLandscap eBroker 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_file. read())
+ juju_info = json.loads(
You could use landscape. lib.fs. read_file here: