Code review comment for lp:~adam-collard/charms/precise/landscape-client/upgrade-charm-hook

Revision history for this message
Adam Collard (adam-collard) wrote :

> Nice work, +1!
>
> [3]
>
> Would you mind creating a hooks/tests sub-directory and move test_hooks.py
> there? For consistency with client/server code, and to avoid cluttering the
> hooks dir (we should eventually have test_ceph.py, test_common.py etc).

The reason it's alongside is that I wanted to match the server charm. I can move both of them if you think that's better?

>
> [4]
>
> + class StubConfig(object):
> +
> + data_path = mkdtemp(prefix=self.__class__.__name__)
> + meta_data_path = os.path.join(data_path, "metadata.d")
> +
> + self.config = StubConfig()
> + self.addCleanup(rmtree, self.config.data_path)
>
> What do you think of:
>
> a) Using MockerTestCase, which sports the makeDir() method, saving
> boilerplate.

I considered it but I didn't want to introduce any dependencies beyond stdlib. I don't think it's worthwhile given that we would then have to add a build step before being able to test the charm (which sits in isolation to the bigger pieces such as client/server)

>
> b) Using a real Configuration object, you just need to set the data_path
> attribute on it.
>
> When possible it feels that using real objects is preferable, to eliminate the
> risk of interface mismatch with the stub.

Makes sense. I didn't actually consider that one. Since we already have an import-time dependency on landscape-client being installed I don't have the same objection as a)

« Back to merge proposal