On Mon, Oct 14, 2013 at 06:55:13AM -0000, Adam Collard wrote:
>  > [1]
> > 
> > +lint:
> > +       bzr ls-lint
> > 
> > I'm not sure this is needed. writng "bzr ls-lint" isn't much harder then
> > "make lint", and the latter is confusing for non-landscapers, since
> > ls-lint is a landscape-specific plugin.
> 
> lint is a make target for server, client and server charm. I think
adding it to the client charm completes the symmetry. In addition, given
that the client already has this target and is also open for non
Landscapers I don't think it's any more confusing. Without that I'd need
to document what linting rules should be used - the Makefile serves that
purpose and additionally is executable :)

Sure, fair enough.



> > [2]
> > 
> > -chown(juju_info_file)
> > 
> > Why was this removed? Why isn't it necessary to change the owner of the
> > file anymore?
> 
> It was removed to make passing the tests feasible without running as
> root. As I looked in to how I might restructure the code and/or tests to
> mock out the chown I realised that it simply wasn't necessary - as far
> as I can tell it was copy-pasta from the config re-writing piece that
> the charm also does. I verified that landscape user can read the file,
> and that's all they need to be able to do.

Ok, sounds reasonable.


> > [5]
> > 
> > +    juju_info = {}.fromkeys(juju_key_map.values())
> > 
> > fromkeys is a classmethod, so let's use the class instead
> > of an instance:
> > 
> >   juju_info = dict.fromkeys(juju_key_map.values())
> > 
> > Although, you could also not create the dictionary with the
> > keys upfront, and instead compare the keys after you've migrated
> > the meta data files. Not sure which looks better, but it feels
> > more intuitive to check the presence of something, rather than
> > to see whether the value is None.
> 
> I wanted juju_info to be strict about what keys are valid, and not
> just a general dictionary. To that end I wanted to create it up front
> and then populate the specific values. Error-checking then fell out as
> being looking for None.

Well, you can still have it to be strict by checking to the keys after
it has been created. It's still a general dictionary, even if you create
the keys upfront. If you want it to be strict a class would be better. I
feel it's more naturally to check for the presence of keys for dicts,
but it's a matter of taste. I'm fine with checking for None as well.


> Have switched to using dict instead of {} though.
> 
> > 
> > 
> > [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.


-- 
Björn Tillenius | https://launchpad.net/~bjornt