Implemented un/expose in fakebackend

Implemented un/expose in fakebackend

Unexpose and expose have been implemented in the
fakebackend and in the PyJujuApi along with tests
to cover both the individual functionality and 
integrated.

https://code.launchpad.net/~hatch/juju-gui/impl-expose/+merge/156995

(do not edit description out of merge proposal)

[jeff.pihach, 2013-04-03 23:00:29]

Please take a look.

[matthew.scott, 2013-04-03 23:23:12]

LGTM - thanks for the additional commands.

[benji, 2013-04-04 14:19:15]

This branch looks good.  The testing is especially well done.

LGTM with some not-quite-trivial comments (but no need to re-review once you've
addressed them to your satisfaction).

https://codereview.appspot.com/8339044/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8339044/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:459: @param {String} serviceName The service name.
An @return would be nice.

https://codereview.appspot.com/8339044/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:485: @param {String} serviceName The service name.
@return again.

https://codereview.appspot.com/8339044/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/8339044/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:393: @param {Object} data Contains service_name to
expose and a callback.
@return

https://codereview.appspot.com/8339044/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:395: performOp_expose: function(data) {
Mutating an input object is a bit unclean.  Perhaps we should operate on a copy
"data".

https://codereview.appspot.com/8339044/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:408: @param {Object} data contains service_name to
unexpose and a callback.
@return

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js#newcode200
test/test_sandbox.js:200: and should be considered valid if "can add additional
units" test passes.
Oh, this looks like the source of "valid" (see below).  I still have no idea
what it means.

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js#newcode227
test/test_sandbox.js:227: Should be considered valid if "can add additional
units (integration)"
I can't parse this sentence.  It might help if it had a non-implied subject. 
I'm also not sure how a test helper can be "valid" or "invalid".

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js#newcode253
test/test_sandbox.js:253: should be considered valid if "can expose a service"
test passes.
I don't understand what "valid" means in this context.

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js#newcode279
test/test_sandbox.js:279: (integration)" test passes.
Another "valid" I don't understand.

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js#newcode777
test/test_sandbox.js:777: mock = {
Naming "mock" "expected" would be more accurate.

[jeff.pihach, 2013-04-04 14:43:06]

Thanks for the reviews! Fixes will be done before submitting

https://codereview.appspot.com/8339044/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (right):

https://codereview.appspot.com/8339044/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:459: @param {String} serviceName The service name.
On 2013/04/04 14:19:15, benji wrote:
> An @return would be nice.

Done.

https://codereview.appspot.com/8339044/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:485: @param {String} serviceName The service name.
On 2013/04/04 14:19:15, benji wrote:
> @return again.

Done.

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js#newcode227
test/test_sandbox.js:227: Should be considered valid if "can add additional
units (integration)"
On 2013/04/04 14:19:15, benji wrote:
> I can't parse this sentence.  It might help if it had a non-implied subject. 
> I'm also not sure how a test helper can be "valid" or "invalid".
I'll make this clearer. Basically it means that this code is directly tested in
those tests so if it fails then that means this helper is bad.

[benji, 2013-04-04 14:46:46]

Thanks for the explanation.

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js
File test/test_sandbox.js (right):

https://codereview.appspot.com/8339044/diff/1/test/test_sandbox.js#newcode227
test/test_sandbox.js:227: Should be considered valid if "can add additional
units (integration)"
On 2013/04/04 14:43:07, jeff.pihach wrote:
> On 2013/04/04 14:19:15, benji wrote:
> > I can't parse this sentence.  It might help if it had a non-implied subject.

> > I'm also not sure how a test helper can be "valid" or "invalid".
> I'll make this clearer. Basically it means that this code is directly tested
in
> those tests so if it fails then that means this helper is bad.

Ah.  It might be OK to just remove the comments of this type altogether.  After
all, if a test helper has a bug, then the tests it helps might fail.  That
doesn't seem like something we have to point out.

[jeff.pihach, 2013-04-04 14:49:54]

*** Submitted:

Implemented un/expose in fakebackend

Unexpose and expose have been implemented in the
fakebackend and in the PyJujuApi along with tests
to cover both the individual functionality and 
integrated.

R=matthew.scott, benji
CC=
https://codereview.appspot.com/8339044