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

> > > [6]
> > > + self.assertEqual(
> > > + {"environment-uuid": "0afcd28d-6263-43fb-8131-afa696a984f8",
> > > + "unit-name": "postgresql/0",
> > > + "api-addresses": "10.0.3.1:17070",
> > > + "private-address": "10.0.3.205"}, juju_info)
> > >
> > > Great that you added some tests!
> > >
> > > I do think that tests should set up what they are testing, though. When
> > > reading the tests, it's hard to see whether these values are correct. It
> > > would be more clear if you had a helper method, create_old_metadata(),
> > > which would accept a dict and write the files for you. If you have that,
> > > calling that method in the test makes it easier to see what the code
> > > is supposed to be doing. I shouldn't have to look at setUp() to understand
> > > how a test works, IMHO.
> >
> > I had something a bit like that in
> > http://bazaar.launchpad.net/~adam-collard/charms/precise/landscape-client
> /upgrade-charm-hook/view/30/hooks/test_hooks.py
> > but then refactored into setUp. I did that because previous reviews left
> > me with the feeling that helper methods aren't liked in tests.
> >
> > Would you prefer it if I revert to r30? Note that I don't ever need to
> > have different contents so I like having that fixed in the helper
> > itself.
>
> No, I don't think you should revert to r30, what you have there is
> basically exactly what you have now, except that you call setUp() in
> each test.
>
> Things that aren't directly important for the test should go in setUp,
> for example creating the config (although I agree with Free that you
> should use a real config object, not a stub one), and creating the
> config data path.
>
> What should be in the test is the juju_info creation. Since you check
> the keys and values in the assertion, you should also pass them in the
> method that writes the data. And you do have different data, you do an
> os.remove() call in at least one of the tests.
>
> Note that you could have default values for the tests that don't check
> the keys and values. But for those that check, it's easier to understand
> if you explicitly pass in the values.

Good spot on the os.remove() I'd forgotten about that. I've re-factored this so the tests are more explicit (don't need to grok setUp so much). Thanks for the suggestions!