Implemented fakebackend remove_relation

Implemented fakebackend remove_relation

Created unit and sandbox integration tests and qa'd
to make sure that it worked properly with the GUI.

https://code.launchpad.net/~hatch/juju-gui/rem-rel/+merge/159407

(do not edit description out of merge proposal)

[jeff.pihach, 2013-04-17 14:56:01]

Please take a look.

[gary.poster, 2013-04-17 15:22:32]

LGTM with reverting config-debug.js, and then merging trunk and getting the
tests to pass again.  Nicely done!

Thanks,

Gary

https://codereview.appspot.com/8830043/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/8830043/diff/1/app/config-debug.js#newcode27
app/config-debug.js:27: sandbox: true,
Oops--back this out.

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

https://codereview.appspot.com/8830043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:97: function parseEndpointStrings(endpoints,
context) {
This is fine as a function--I kinda like it this way, tbh.  That said, out of
curiosity, given that we need context, why did you not make this a method on the
FakeBackend?

https://codereview.appspot.com/8830043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:724: @method addRelation
Good catch.  This might be a nice simple thing to add to the docstring linter.

https://codereview.appspot.com/8830043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:824: // This error should never be hit but it's
here JIC
It strikes me that a more likely error, in theory at least, here and in
addRelation, is that the services are not found.  We probably won't encounter
this in the GUI... <shrug>

https://codereview.appspot.com/8830043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:836: // Check to see if the interface names match
More readable to split the condition up, I guess.  Cool.

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

https://codereview.appspot.com/8830043/diff/1/app/store/env/sandbox.js#newcod...
app/store/env/sandbox.js:562: @param {Object} data Object contains the
operation, two endpoint strings
Thanks for cleanup.

https://codereview.appspot.com/8830043/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/8830043/diff/1/test/test_fakebackend.js#newcod...
test/test_fakebackend.js:1025: describe('FakeBackend.removeRelation', function()
{
Nice test, thank you.

https://codereview.appspot.com/8830043/diff/1/test/test_fakebackend.js#newcod...
test/test_fakebackend.js:1039: fakebackend = setupData.fakebackend;
Please merge trunk, run tests, and adjust.  You'll need to switch the above two
lines to simply

fakebackend = utils.makeFakeBackendWithCharmStore();

https://codereview.appspot.com/8830043/diff/1/test/test_fakebackend.js#newcod...
test/test_fakebackend.js:1041: deployResult = undefined;
I s'pose you could do that in afterEach.  <shrug>

[jeff.pihach, 2013-04-17 16:09:04]

Thanks for the review, changes made and trunk merged!

https://codereview.appspot.com/8830043/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/8830043/diff/1/app/config-debug.js#newcode27
app/config-debug.js:27: sandbox: true,
oops!

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

https://codereview.appspot.com/8830043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:97: function parseEndpointStrings(endpoints,
context) {
at the time I wasn't sure if any other methods would require this function - now
I'm questioning that decision. I'll move it back in.

https://codereview.appspot.com/8830043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:824: // This error should never be hit but it's
here JIC
Yeah that is probably right however I have run into it with tests failing which
helped me narrow down the issue really fast.

https://codereview.appspot.com/8830043/diff/1/app/store/env/fakebackend.js#ne...
app/store/env/fakebackend.js:836: // Check to see if the interface names match
Yeah that was my intention :)

https://codereview.appspot.com/8830043/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/8830043/diff/1/test/test_fakebackend.js#newcod...
test/test_fakebackend.js:1039: fakebackend = setupData.fakebackend;
Thanks for that - the failing tests after merge would have been very confusing
:)

https://codereview.appspot.com/8830043/diff/1/test/test_fakebackend.js#newcod...
test/test_fakebackend.js:1041: deployResult = undefined;
On 2013/04/17 15:22:32, gary.poster wrote:
> I s'pose you could do that in afterEach.  <shrug>

Done.

[jeff.pihach, 2013-04-17 16:12:41]

Please take a look.

[bcsaller, 2013-04-17 17:10:24]

LGTM

A few trivial comments, I would prefer using the helper. Extracting the relation
match find code to relation list would be nice, but you can land w/o that if you
see it as a hardship.

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

https://codereview.appspot.com/8830043/diff/9001/app/store/env/fakebackend.js...
app/store/env/fakebackend.js:840: return true;
I'd be tempted to add a find(epA, epB) to relationList at this point, you do
this dance more than once now.

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

https://codereview.appspot.com/8830043/diff/9001/app/store/env/sandbox.js#new...
app/store/env/sandbox.js:608: data.endpoint_a, data.endpoint_b);
I think you can use the helper here.
(untested)

OP(this, 'removeRelation', ['endpoint_a', 'endpoint_b'], 
data);

If that works I'd prefer we use it.

[jeff.pihach, 2013-04-17 21:27:30]

Thanks for the reviews! Ben I'm going to make a followup to look into creating a
helper

[jeff.pihach, 2013-04-17 21:30:03]

*** Submitted:

Implemented fakebackend remove_relation

Created unit and sandbox integration tests and qa'd
to make sure that it worked properly with the GUI.

R=gary.poster, bcsaller
CC=
https://codereview.appspot.com/8830043