Juju Charms Collection
juju-gui package

Merge lp:~teknico/charms/precise/juju-gui/1171516-more-backend-tests into lp:~juju-gui/charms/precise/juju-gui/trunk

  1. Precise Pangolin (12.04)
  2. 1171516-more-backend-tests
  3. Merge into trunk
Proposed by Nicola Larosa
Status: Merged
Merged at revision: 55
Proposed branch: lp:~teknico/charms/precise/juju-gui/1171516-more-backend-tests
Merge into: lp:~juju-gui/charms/precise/juju-gui/trunk
Diff against target: 0 lines
To merge this branch: bzr merge lp:~teknico/charms/precise/juju-gui/1171516-more-backend-tests
Reviewer Review Type Date Requested Status
charmers Pending
Review via email: mp+162106@code.launchpad.net

Description of the change

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://codereview.appspot.com/9121043/

To post a comment you must log in.
Revision history for this message
Nicola Larosa (teknico) wrote :

Reviewers: mp+162106_code.launchpad.net,

Message:
Please take a look.

Description:
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.

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)

Please review this at https://codereview.appspot.com/9121043/

Affected files:
   M .bzrignore
   A [revision details]
   M hooks/backend.py
   M hooks/stop
   M hooks/utils.py
   M tests/test_backends.py
   M tests/test_utils.py

Revision history for this message
Nicola Larosa (teknico) wrote :

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. :-)

https://codereview.appspot.com/9121043/

Revision history for this message
Benjamin Saller (bcsaller) wrote :

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?

https://codereview.appspot.com/9121043/

Revision history for this message
Nicola Larosa (teknico) wrote :

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.

https://codereview.appspot.com/9121043/

Revision history for this message
Francesco Banconi (frankban) wrote :

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) ?

https://codereview.appspot.com/9121043/

Revision history for this message
Benjamin Saller (bcsaller) wrote :

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.

https://codereview.appspot.com/9121043/

Revision history for this message
Nicola Larosa (teknico) wrote :

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.

https://codereview.appspot.com/9121043/

lp:~teknico/charms/precise/juju-gui/1171516-more-backend-tests updated
63. By Nicola Larosa

Changes per reviews.

Revision history for this message
Nicola Larosa (teknico) wrote :

*** 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

https://codereview.appspot.com/9121043/

Revision history for this message
Gary Poster (gary) wrote :

Nice branch, teknico. Thank you for landing it.

Gary

https://codereview.appspot.com/9121043/

Preview Diff

Empty

Subscribers

People subscribed via source and target branches