Change Go get_service results to match others.

Change Go get_service results to match others.

Also bolster tests.

https://code.launchpad.net/~bac/juju-gui/loadServiceTests/+merge/158099

(do not edit description out of merge proposal)

[bac, 2013-04-10 13:15:42]

Please take a look.

[teknico, 2013-04-10 13:36:16]

LGTM, see trivials below.

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

https://codereview.appspot.com/8623043/diff/1/app/store/env/go.js#newcode611
app/store/env/go.js:611: // Set the service name to 'name' for compatability
with other
Even though "compatability" has 60M Google hits against the 105M of
"compatibility", it's still considered a misspelling. Go figure. :-)

https://codereview.appspot.com/8623043/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode567
test/test_app.js:567: dispatched = true;
A flag to check that the callback was indeed called: good idea, we should do it
more.

[gary.poster, 2013-04-10 14:01:42]

LGTM, though some of the changes I request are important.

Thank you!

Gary

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

https://codereview.appspot.com/8623043/diff/1/app/store/env/go.js#newcode613
app/store/env/go.js:613: data.Response.name = data.Response.Service;
You could delete Service.  <shrug>.  Maybe we want to gradually switch from
pyjuju spellings to jujucore, so this is fine.  <shrug again>

https://codereview.appspot.com/8623043/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode528
test/test_app.js:528: app = new Y.juju.App({ });
Please see
http://jujugui.wordpress.com/2013/04/10/test-tip-whenever-you-instantiate-our...

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode535
test/test_app.js:535: var notification = app.db.notifications.toArray()[0];
isn't toArray()[0] equivalent to item(0) (and the latter is nicer IMO)?

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode536
test/test_app.js:536: notification.get('title').should.equal('Error loading
service');
FWIW, I've been using the assert interface lately, on benji's recommendation,
and I like it a lot better.  In this case the equivalent would be

assert.strictEqual('Error loading service', notification.get('title'));

This is entirely aesthetic, I guess, but the assert approach seems clearer to
me, and also avoids the need for ever having to do those "var _ = [ASSERTION]"
hacks.

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode541
test/test_app.js:541: app = new Y.juju.App({ });
Same app cleanup request as before.

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode561
test/test_app.js:561: destroyMe.push(env);
IIRC app destoys its env. /me checks.  Yeah, it does. So this is fine but
unnecessary.

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode563
test/test_app.js:563: app = new Y.juju.App({env: env });
Even with the env, the blog post still mentions the view thing that you should
do here.

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js
File test/test_env_go.js (left):

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js#oldcode610
test/test_env_go.js:610: assert.equal(service_name, 'mysql');
Why don't you want this any more?

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js#newcode602
test/test_env_go.js:602: service_name = data.service_name;
This is no longer used in the test (not sure why, but commenting on that later)

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js#newcode610
test/test_env_go.js:610: expected.name = result.name;
should this be "expected.name = 'mysql';"?

[bac, 2013-04-10 14:54:09]

*** Submitted:

Change Go get_service results to match others.

Also bolster tests.

R=teknico, gary.poster
CC=
https://codereview.appspot.com/8623043

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

https://codereview.appspot.com/8623043/diff/1/app/store/env/go.js#newcode611
app/store/env/go.js:611: // Set the service name to 'name' for compatability
with other
On 2013/04/10 13:36:16, teknico wrote:
> Even though "compatability" has 60M Google hits against the 105M of
> "compatibility", it's still considered a misspelling. Go figure. :-)

Done.

https://codereview.appspot.com/8623043/diff/1/app/store/env/go.js#newcode613
app/store/env/go.js:613: data.Response.name = data.Response.Service;
I could but figured it just didn't matter.  <double-shrug>

https://codereview.appspot.com/8623043/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode528
test/test_app.js:528: app = new Y.juju.App({ });
On 2013/04/10 14:01:42, gary.poster wrote:
> Please see
>
http://jujugui.wordpress.com/2013/04/10/test-tip-whenever-you-instantiate-our...

Done.

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode535
test/test_app.js:535: var notification = app.db.notifications.toArray()[0];
Yes, it was a dumb pattern used a few places.  I've changed them all.

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode536
test/test_app.js:536: notification.get('title').should.equal('Error loading
service');
Yes but it is 8 characters longer.

Changed.

Oh, :)

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode541
test/test_app.js:541: app = new Y.juju.App({ });
On 2013/04/10 14:01:42, gary.poster wrote:
> Same app cleanup request as before.

Done.

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode561
test/test_app.js:561: destroyMe.push(env);
On 2013/04/10 14:01:42, gary.poster wrote:
> IIRC app destoys its env. /me checks.  Yeah, it does. So this is fine but
> unnecessary.

Done.

https://codereview.appspot.com/8623043/diff/1/test/test_app.js#newcode563
test/test_app.js:563: app = new Y.juju.App({env: env });
On 2013/04/10 14:01:42, gary.poster wrote:
> Even with the env, the blog post still mentions the view thing that you should
> do here.

Done.

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js
File test/test_env_go.js (left):

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js#oldcode610
test/test_env_go.js:610: assert.equal(service_name, 'mysql');
On 2013/04/10 14:01:42, gary.poster wrote:
> Why don't you want this any more?

Done.

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js#newcode602
test/test_env_go.js:602: service_name = data.service_name;
On 2013/04/10 14:01:42, gary.poster wrote:
> This is no longer used in the test (not sure why, but commenting on that
later)

Done.

https://codereview.appspot.com/8623043/diff/1/test/test_env_go.js#newcode610
test/test_env_go.js:610: expected.name = result.name;
On 2013/04/10 14:01:42, gary.poster wrote:
> should this be "expected.name = 'mysql';"?

Done.