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

Revision history for this message
Björn Tillenius (bjornt) wrote :

On Mon, Oct 14, 2013 at 07:53:40AM -0000, Adam Collard wrote:
> > > > [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!

Thanks, looks good now, +1!

  vote approve

review: Approve

« Back to merge proposal