Add more unit tests to the charm backend.

Add more unit tests to the charm backend.

More unit tests for backend properties, commands and methods.
In order to do that, it mangles the charm code pretty heavily:

- it adds module prefixes to all functions imported by backend.py in order to
  make them monkeypatchable in tests;
- it renames all sub-backends to mixins, for clarity. Arguably, "backend.mixins
  lists all active mixins" is clearer than "backend.backends lists all active
  sub-backends, some of which are actual backends and some others are mixins";
- it simplifies the code in a number of places;
- it removes unused or scarcely used code in backend.py, for simplicity;
- it rephrases some docstrings in more specific terms, for readability;
- it removes the overrideable testing framework in backend.py. It imposes a
  runtime and complexity cost for doing what is already done in other tests by
  monkeypatching;

Further style changes have been moved to a subsequent branch.

It also fixes two import errors.

WARNING: this branch did successfully deploy a stable juju-gui release.
Deploy tests did not run successfully though, and they seem to be broken
in trunk too, right now.

https://code.launchpad.net/~teknico/charms/precise/juju-gui/1171516-more-backend-tests/+merge/162106

(do not edit description out of merge proposal)

[teknico, 2013-05-02 14:57:29]

Please take a look.

[teknico, 2013-05-02 15:02:29]

Some comments to help with review, enjoy. :-)

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py
File hooks/backend.py (left):

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#oldcode125
hooks/backend.py:125: 
gui_properties is unused.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#oldcode234
hooks/backend.py:234: raise
Not very useful being able to say "backend['val']" instead of
"backend.config['val']".

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#newcode160
hooks/backend.py:160: prev_config = {}
Adding this allows us to simplify the code in self.different below.

https://codereview.appspot.com/9121043/diff/1/hooks/stop
File hooks/stop (left):

https://codereview.appspot.com/9121043/diff/1/hooks/stop#oldcode8
hooks/stop:8: stop,
There's no such thing in utils.

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py
File hooks/utils.py (left):

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#oldcode570
hooks/utils.py:570: """
StopChain is not trapped anywhere.

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#oldcode585
hooks/utils.py:585: workingset = reversed(workingset)
The "reverse" parameter is not used.

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#newcode43
hooks/utils.py:43: import errno
The absence of this one was causing an error.

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#newcode581
hooks/utils.py:581: callable(mixin, self)
Simpler code is simpler. :-)

[bcsaller, 2013-05-02 15:25:44]

LGTM (without qa)

The code is neatly cleaned up in a number of places. I'd suggest using
defaultdict if it makes sense in the tests. 

Thank you for this.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py
File hooks/backend.py (left):

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#oldcode125
hooks/backend.py:125: 
On 2013/05/02 15:02:29, teknico wrote:
> gui_properties is unused.

Previously it was used as the conditional trigger on restarts.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#oldcode234
hooks/backend.py:234: raise
On 2013/05/02 15:02:29, teknico wrote:
> Not very useful being able to say "backend['val']" instead of
> "backend.config['val']".

I like shortcuts, but I'm also totally fine with this change.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#oldcode237
hooks/backend.py:237: def find_missing_packages(self, *packages):
This stuff was with an eye towards testing, but resolving names from modules
works better, thank you.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#newcode14
hooks/backend.py:14: import utils
Nice change and good reason for it.

https://codereview.appspot.com/9121043/diff/1/hooks/stop
File hooks/stop (left):

https://codereview.appspot.com/9121043/diff/1/hooks/stop#oldcode8
hooks/stop:8: stop,
On 2013/05/02 15:02:29, teknico wrote:
> There's no such thing in utils.
I'd forgotten to remove this as it never ran in py-juju or with sandbox. Thanks

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py
File hooks/utils.py (left):

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#oldcode570
hooks/utils.py:570: """
On 2013/05/02 15:02:29, teknico wrote:
> StopChain is not trapped anywhere.

Yeah, YAGNI

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#oldcode585
hooks/utils.py:585: workingset = reversed(workingset)
On 2013/05/02 15:02:29, teknico wrote:
> The "reverse" parameter is not used.

Thanks, I think at one point I thought there might be a backend.stop and we
could reverse the order we ran the start chain in, but isn't needed.

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#newcode581
hooks/utils.py:581: callable(mixin, self)
On 2013/05/02 15:02:29, teknico wrote:
> Simpler code is simpler. :-)
At the time I'd been doing JS long enough I couldn't remember how to spell that.
:-/

https://codereview.appspot.com/9121043/diff/1/tests/test_backends.py
File tests/test_backends.py (right):

https://codereview.appspot.com/9121043/diff/1/tests/test_backends.py#newcode106
tests/test_backends.py:106: class GotEmAllDict(dict):
Isn't there defaultdict in the Python std lib collections for this?

[teknico, 2013-05-02 16:04:10]

bcsaller wrote:
> Isn't there defaultdict in the Python std lib collections for this?

Indeed there is. However it does not override "get", so it's not a huge win. But
it may make sense anyway, actually.

[frankban, 2013-05-02 16:13:32]

LGTM, and thanks for all the tests :-)

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py
File hooks/backend.py (left):

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#oldcode228
hooks/backend.py:228: def __getitem__(self, key):
Thanks for removing unused code.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#newcode122
hooks/backend.py:122: class ImprovMixin(object):
I agree it's more clear now that they are all mixins.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#newcode149
hooks/backend.py:149: through composition
Missing final dot.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#newcode205
hooks/backend.py:205: return any(self.config.get(key) !=
self.prev_config.get(key)
Nice. What do you think about avoiding some lookups here? e.g.

get_current = self.config.get
get_previous = self.prev_config.get
return any(get_current(key) != get_previous(key) for key in keys)

Just an idea, do with that what you want.

https://codereview.appspot.com/9121043/diff/1/hooks/stop
File hooks/stop (right):

https://codereview.appspot.com/9121043/diff/1/hooks/stop#newcode6
hooks/stop:6: from utils import log_hook
Did the same change in my branch, almost specific to this issue :-/

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#newcode43
hooks/utils.py:43: import errno
On 2013/05/02 15:02:29, teknico wrote:
> The absence of this one was causing an error.

Good catch.

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#newcode580
hooks/utils.py:580: if callable:
Shrug: callable is also the name of a builtin function.

https://codereview.appspot.com/9121043/diff/1/tests/test_backends.py
File tests/test_backends.py (right):

https://codereview.appspot.com/9121043/diff/1/tests/test_backends.py#newcode106
tests/test_backends.py:106: class GotEmAllDict(dict):
On 2013/05/02 15:25:44, bcsaller wrote:
> Isn't there defaultdict in the Python std lib collections for this?

Yes, collections.defaultdict.

https://codereview.appspot.com/9121043/diff/1/tests/test_backends.py#newcode183
tests/test_backends.py:183: test_backend =
backend.Backend(config=GotEmAllDict(False))
collections.defaultdict(lambda: False) ?

[bcsaller, 2013-05-02 16:15:27]

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#newcode205
hooks/backend.py:205: return any(self.config.get(key) !=
self.prev_config.get(key)
On 2013/05/02 16:13:32, frankban wrote:
> Nice. What do you think about avoiding some lookups here? e.g.
> 
> get_current = self.config.get
> get_previous = self.prev_config.get
> return any(get_current(key) != get_previous(key) for key in keys)
> 
> Just an idea, do with that what you want.

I'm a fan of that idea.

[teknico, 2013-05-02 16:43:35]

Thanks for the reviews!

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#newcode149
hooks/backend.py:149: through composition
On 2013/05/02 16:13:32, frankban wrote:
> Missing final dot.

Done.

https://codereview.appspot.com/9121043/diff/1/hooks/backend.py#newcode205
hooks/backend.py:205: return any(self.config.get(key) !=
self.prev_config.get(key)
> frankban wrote:
>> What do you think about avoiding some lookups here?

bcsaller wrote:
> I'm a fan of that idea.

Yeah, it's inside a loop after all. Let's optimize prematurely! ;-)

https://codereview.appspot.com/9121043/diff/1/hooks/stop
File hooks/stop (right):

https://codereview.appspot.com/9121043/diff/1/hooks/stop#newcode6
hooks/stop:6: from utils import log_hook
frankban wrote:
> Did the same change in my branch, almost specific to this issue :-/

I know, but I needed it to have any hope to run deploy tests. They did not
succeed anyway, but that's another matter...

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#newcode429
hooks/utils.py:429: if e.errno != errno.EEXIST:  # File exists.
This one was weird. Sublime Text was complaining about the single space before
the inline comment, and that message was obscuring the much more important one
about the import error (see top of file). :-/

https://codereview.appspot.com/9121043/diff/1/hooks/utils.py#newcode580
hooks/utils.py:580: if callable:
frankban wrote:
> Shrug: callable is also the name of a builtin function.

That's what was nagging me about it! Thanks for pointing it out, changed.

https://codereview.appspot.com/9121043/diff/1/tests/test_backends.py
File tests/test_backends.py (right):

https://codereview.appspot.com/9121043/diff/1/tests/test_backends.py#newcode106
tests/test_backends.py:106: class GotEmAllDict(dict):
> bcsaller wrote:
>> Isn't there defaultdict in the Python std lib collections
>> for this?

frankban wrote:
> Yes, collections.defaultdict.

Ok, changed to use it, even if it still needs to override "get".

https://codereview.appspot.com/9121043/diff/1/tests/test_backends.py#newcode183
tests/test_backends.py:183: test_backend =
backend.Backend(config=GotEmAllDict(False))
frankban wrote:
> collections.defaultdict(lambda: False) ?

As mentioned, I still need to override the "get" method, so it'll still have to
be a subclass.

[teknico, 2013-05-02 16:53:14]

*** Submitted:

Add more unit tests to the charm backend.

More unit tests for backend properties, commands and methods.
In order to do that, it mangles the charm code pretty heavily:

- it adds module prefixes to all functions imported by backend.py in order to
  make them monkeypatchable in tests;
- it renames all sub-backends to mixins, for clarity. Arguably, "backend.mixins
  lists all active mixins" is clearer than "backend.backends lists all active
  sub-backends, some of which are actual backends and some others are mixins";
- it simplifies the code in a number of places;
- it removes unused or scarcely used code in backend.py, for simplicity;
- it rephrases some docstrings in more specific terms, for readability;
- it removes the overrideable testing framework in backend.py. It imposes a
  runtime and complexity cost for doing what is already done in other tests by
  monkeypatching;

Further style changes have been moved to a subsequent branch.

It also fixes two import errors.

WARNING: this branch did successfully deploy a stable juju-gui release.
Deploy tests did not run successfully though, and they seem to be broken
in trunk too, right now.

R=bcsaller, frankban
CC=
https://codereview.appspot.com/9121043

[gary.poster, 2013-05-02 17:20:50]

Nice branch, teknico.  Thank you for landing it.

Gary