Merge lp://qastaging/~foxxtrot/launchpad/308198 into lp://qastaging/launchpad

Needs test.

Since no unit tests exist for Client yet, this will take a bit longer to set up. I am working on it, however.

Thanks. Looking forward to the update!

That library is actually pretty well covered in lib/canonical/launchpad/windmill/jstests/launchpad_ajax.js

Ah, thank you, I was surprised not to see a test directory alongside the client.js code. I'll likely add my tests to this location.

Just noticed the updates. Feel free to prod reviewers when your branches are not moving forward... and thanks for bearing with us!

I'm not familiar with jum yet so I won't pretend to have much to say about the tests. Just a few small notes:

1 === modified file 'lib/canonical/launchpad/javascript/client/client.js'
2 --- lib/canonical/launchpad/javascript/client/client.js 2010-02-16 16:29:36 +0000
3 +++ lib/canonical/launchpad/javascript/client/client.js 2010-07-09 06:29:49 +0000
4 @@ -308,7 +308,12 @@
5
6 LP.client.Collection = function(client, representation, uri) {
7 /* A grouped collection of objets from the Launchpad web service. */
8 + var index, entry;
9 this.init(client, representation, uri);
10 + for (index = 0 ; index < this.entries.length ; index += 1) {

In JavaScript you can just index++ instead of index += 1.

11 + entry = this.entries[index];
12 + this.entries[index] = new LP.client.Entry(client, entry, entry.self_link);

Methinks this line be too long. We stop at 78 columns.

1 === modified file 'lib/canonical/launchpad/javascript/client/client.js'
2 --- lib/canonical/launchpad/javascript/client/client.js 2010-02-16 16:29:36 +0000
3 +++ lib/canonical/launchpad/javascript/client/client.js 2010-07-09 06:29:49 +0000
4 @@ -308,7 +308,12 @@
5
6 LP.client.Collection = function(client, representation, uri) {
7 /* A grouped collection of objets from the Launchpad web service. */
8 + var index, entry;
9 this.init(client, representation, uri);
10 + for (index = 0 ; index < this.entries.length ; index += 1) {
11 + entry = this.entries[index];
12 + this.entries[index] = new LP.client.Entry(client, entry, entry.self_link);
13 + }
14 };
15
16 LP.client.Collection.prototype = new LP.client.Resource();
17
18 === modified file 'lib/canonical/launchpad/windmill/jstests/launchpad_ajax.js'
19 --- lib/canonical/launchpad/windmill/jstests/launchpad_ajax.js 2010-03-11 16:42:49 +0000
20 +++ lib/canonical/launchpad/windmill/jstests/launchpad_ajax.js 2010-07-09 06:29:49 +0000
21 @@ -457,6 +457,35 @@
22 var collection = test.result.args[0];
23 jum.assertTrue(collection instanceof LP.client.Collection);
24 jum.assertEquals(4, collection.total_size);
25 + },
26 + 'wait_action',
27 + function (test) {
28 + jum.assertEquals('success', test.result.callback);
29 + var entries = test.result.args[0].entries,
30 + length = test.result.args[0].total_size,
31 + index;
32 + for (index = 0 ; index < length ; index += 1) {
33 + jum.assertTrue(entries[index] instanceof LP.client.Entry);
34 + }
35 + },
36 + 'wait_action',
37 + function (test) {
38 + jum.assertEquals('success', test.result.callback);
39 + var entry = test.result.args[0].entries[0];
40 + jum.assertEquals('test', entry.display_name);
41 + entry.set('display_name', "Set Display Name");
42 + entry.lp_save({on: test.create_yui_sync_on()});

Ending a test in an action without observed result is not ideal. It becomes even less so in an uncomplaining language like JavaScript. Could you verify that the change goes through?

Jeroen

Setting to WIP as its been reviewed and action is needed.

> In JavaScript you can just index++ instead of index += 1.

Habit. I base my JS on what passes JSLint, and Crockford has this unreasonable hatred of the increment operator. I'll revise, I didn't look very hard to see what the convention was.

>
> 11 + entry = this.entries[index];
> 12 + this.entries[index] = new LP.client.Entry(client, entry,
> entry.self_link);
>
> Methinks this line be too long. We stop at 78 columns.

Edit coming soon.
>
>
> 1 === modified file
> 'lib/canonical/launchpad/javascript/client/client.js'
> 2 --- lib/canonical/launchpad/javascript/client/client.js 2010-02-16
> 16:29:36 +0000
> 3 +++ lib/canonical/launchpad/javascript/client/client.js 2010-07-09
> 06:29:49 +0000
> 4 @@ -308,7 +308,12 @@
> 5
> 6 LP.client.Collection = function(client, representation, uri) {
> 7 /* A grouped collection of objets from the Launchpad web service.
> */
> 8 + var index, entry;
> 9 this.init(client, representation, uri);
> 10 + for (index = 0 ; index < this.entries.length ; index += 1) {
> 11 + entry = this.entries[index];
> 12 + this.entries[index] = new LP.client.Entry(client, entry,
> entry.self_link);
> 13 + }
> 14 };
> 15
> 16 LP.client.Collection.prototype = new LP.client.Resource();
> 17
> 18 === modified file
> 'lib/canonical/launchpad/windmill/jstests/launchpad_ajax.js'
> 19 --- lib/canonical/launchpad/windmill/jstests/launchpad_ajax.js
> 2010-03-11 16:42:49 +0000
> 20 +++ lib/canonical/launchpad/windmill/jstests/launchpad_ajax.js
> 2010-07-09 06:29:49 +0000
> 21 @@ -457,6 +457,35 @@
> 22 var collection = test.result.args[0];
> 23 jum.assertTrue(collection instanceof LP.client.Collection);
> 24 jum.assertEquals(4, collection.total_size);
> 25 + },
> 26 + 'wait_action',
> 27 + function (test) {
> 28 + jum.assertEquals('success', test.result.callback);
> 29 + var entries = test.result.args[0].entries,
> 30 + length = test.result.args[0].total_size,
> 31 + index;
> 32 + for (index = 0 ; index < length ; index += 1) {
> 33 + jum.assertTrue(entries[index] instanceof
> LP.client.Entry);
> 34 + }
> 35 + },
> 36 + 'wait_action',
> 37 + function (test) {
> 38 + jum.assertEquals('success', test.result.callback);
> 39 + var entry = test.result.args[0].entries[0];
> 40 + jum.assertEquals('test', entry.display_name);
> 41 + entry.set('display_name', "Set Display Name");
> 42 + entry.lp_save({on: test.create_yui_sync_on()});
>
> Ending a test in an action without observed result is not ideal. It becomes
> even less so in an uncomplaining language like JavaScript. Could you verify
> that the change goes through?

This is immediately followed by a 'get' and then the check to make sure the change is saved. Lines 44--53 in the diff. I'm not terribly familiar with jum, but this *seemed* to be what was being done in other similar test cases.

A push ...

> > Ending a test in an action without observed result is not ideal. It becomes
> > even less so in an uncomplaining language like JavaScript. Could you verify
> > that the change goes through?
>
> This is immediately followed by a 'get' and then the check to make sure the
> change is saved. Lines 44--53 in the diff. I'm not terribly familiar with jum,
> but this *seemed* to be what was being done in other similar test cases.

My bad, I'm sure. Looks fine to me... Apart from some weird indentation on the respective first lines of all those jum test functions. Is that intentional?

Anyway, ignoring the fact that I don't know the first thing about the subject matter I have no further objects to this branch.

Jeroen

Is there anything further I should do regarding this branch and it's associated bug?

Well, it needs to be landed obviously. I wasn't aware that you didn't have someone to do this; I'll see if I can make that work now.

Oh, I can't do that right now; having some trouble with my machine. I'll ask someone else on the team.

All sorted now. It's on the way to EC2 for landing.