Code review - Issue 8236043: Added add_unit integration to fake PyJujuAPIhttps://codereview.appspot.com/2013-04-01T23:21:44+00:00rietveld
Message from unknown
2013-04-01T20:56:08+00:00jeff.pihachurn:md5:1eb7d9e98dac3d8029c3925262e98d01
Message from jeff.pihach@canonical.com
2013-04-01T20:56:11+00:00jeff.pihachurn:md5:3cdc46761dedd135cb7a9f9f1af0effa
Please take a look.
Message from gary.poster@canonical.com
2013-04-01T21:35:22+00:00gary.posterurn:md5:1163eb5fca94d5000b67e76cacbb3bcd
On the right track. Thank you.
https://codereview.appspot.com/8236043/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):
https://codereview.appspot.com/8236043/diff/1/app/store/env/sandbox.js#newcode365
app/store/env/sandbox.js:365: var res = this.get('state').addUnit(data.service_name, data.num_units);
Something like this.
if (res.error) {
data.err = res.error;
} else {
data.result = Y.Array.map(
res.units, function(u) {return u.id});
}
https://codereview.appspot.com/8236043/diff/1/app/store/env/sandbox.js#newcode366
app/store/env/sandbox.js:366: data.response = res;
Not this. :-)
https://codereview.appspot.com/8236043/diff/1/test/test_sandbox.js
File test/test_sandbox.js (right):
https://codereview.appspot.com/8236043/diff/1/test/test_sandbox.js#newcode531
test/test_sandbox.js:531: client.onmessage = function(received) {
You can verify that the result is a list of ids (see app/views/service.js in _addUnitCallback). Nice but not necessary to show that they are the right ids.
It would be nice to also have a separate test showing that you properly create an err when there is an error. Pass an invalid number of units or reference a service that does not exist.
https://codereview.appspot.com/8236043/diff/1/test/test_sandbox.js#newcode545
test/test_sandbox.js:545: env.add_unit('kumquat', 2, function(data) {
Good test. You could look at the data here also, but that's probably overkill.
Message from jeff.pihach@canonical.com
2013-04-01T22:22:35+00:00jeff.pihachurn:md5:064245df52b5116227de7568a29db062
Thanks for the review - new code incoming.
https://codereview.appspot.com/8236043/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):
https://codereview.appspot.com/8236043/diff/1/app/store/env/sandbox.js#newcode365
app/store/env/sandbox.js:365: var res = this.get('state').addUnit(data.service_name, data.num_units);
updated.
https://codereview.appspot.com/8236043/diff/1/test/test_sandbox.js
File test/test_sandbox.js (right):
https://codereview.appspot.com/8236043/diff/1/test/test_sandbox.js#newcode531
test/test_sandbox.js:531: client.onmessage = function(received) {
Updated the test to check for a valid object and added err test.
Message from unknown
2013-04-01T22:33:54+00:00jeff.pihachurn:md5:fce0225619ef798d7ffd197d33ae6b23
Message from jeff.pihach@canonical.com
2013-04-01T22:33:56+00:00jeff.pihachurn:md5:05b527520aeeb3dcb3b8b1cb7758be8c
Please take a look.
Message from gary.poster@canonical.com
2013-04-01T22:37:50+00:00gary.posterurn:md5:b1dfaff0d9b0512abf125540edafedd1
LGTM. Thank you for sticking with this, and sorry for not being clear!
Gary
https://codereview.appspot.com/8236043/diff/7001/test/test_sandbox.js
File test/test_sandbox.js (right):
https://codereview.appspot.com/8236043/diff/7001/test/test_sandbox.js#newcode531
test/test_sandbox.js:531: client.onmessage = function(received) {
assert.isUndefined(received.err)
?
Message from unknown
2013-04-01T22:46:56+00:00jeff.pihachurn:md5:10fc6474d1de75165e15911f5d41d9d6
Message from jeff.pihach@canonical.com
2013-04-01T22:46:58+00:00jeff.pihachurn:md5:0c266921bc3210d152aa989690804289
Please take a look.
Message from bcsaller@gmail.com
2013-04-01T23:13:52+00:00bcsallerurn:md5:947ef414667eea0ee67b0f483e386a3a
LGTM though the way you call deploy in your last test is off so not sure what behavior your getting there.
https://codereview.appspot.com/8236043/diff/12001/test/test_sandbox.js
File test/test_sandbox.js (right):
https://codereview.appspot.com/8236043/diff/12001/test/test_sandbox.js#newcode590
test/test_sandbox.js:590: 'cs:wordpress', 'kumquat', {llama: 'pajama'}, null, 1, callback);
What signature for deploy is this?
Message from jeff.pihach@canonical.com
2013-04-01T23:17:02+00:00jeff.pihachurn:md5:d793b084a2b5addfc20e31e1119893da
Thanks guys for the reviews!
I had copied that signature from a previous test but it looks like it lines up with the deploy method in python.js ln227
Message from unknown
2013-04-01T23:18:01+00:00jeff.pihachurn:md5:0f611cf073c3bc7da611334b50faaa02
Message from bcsaller@gmail.com
2013-04-01T23:20:48+00:00bcsallerurn:md5:c26946536c59e45101290a2d8a8ba8d3
On 2013/04/01 23:17:02, jeff.pihach wrote:
> Thanks guys for the reviews!
>
> I had copied that signature from a previous test but it looks like it lines up
> with the deploy method in python.js ln227
ahh, fakebackend has a different signature.
Message from jeff.pihach@canonical.com
2013-04-01T23:21:44+00:00jeff.pihachurn:md5:6b66df530b3d35a8844a01a8788fa8b7
*** Submitted:
Added add_unit integration to fake PyJujuAPI
add_unit integration for the fake backend of PyJujuAPI
is completed with unit and integration tests using the
same methods as the UI.
R=gary.poster, bcsaller
CC=
https://codereview.appspot.com/8236043