Code review comment for lp://staging/~gary/juju-gui/fixGoSandboxWatcher2

Revision history for this message
Gary Poster (gary) wrote :

Reviewers: mp+190685_code.launchpad.net,

Message:
Please take a look.

Description:
Fix Go sandbox AllWatcher initialization

The Go sandbox AllWatcher was trying to send deltas immediately, which
does not match the juju core implementation. This fixes the behavior,
with a test.

It also fixes a user-visible bug. To qa, turn off the simulator in the
config file, and then deploy a service in the default go sandbox.
Before my branch, the unit would never appear, because the delta for
this change was lost. After my branch, the unit appears correctly.

https://code.launchpad.net/~gary/juju-gui/fixGoSandboxWatcher2/+merge/190685

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14548050/

Affected files (+8, -5 lines):
   A [revision details]
   M app/store/env/sandbox.js
   M test/test_sandbox_go.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision: <email address hidden>

Index: test/test_sandbox_go.js
=== modified file 'test/test_sandbox_go.js'
--- test/test_sandbox_go.js 2013-10-01 18:30:10 +0000
+++ test/test_sandbox_go.js 2013-10-11 13:47:43 +0000
@@ -164,7 +164,8 @@
        client.onmessage = function(received) {
          var receivedData = Y.JSON.parse(received.data);
          assert.equal(receivedData.Response.AllWatcherId, 42);
- assert.equal(client.get('juju').get('nextRequestId'), 1066);
+ assert.equal(receivedData.RequestId, 1066);
+ assert.isUndefined(client.get('juju').get('nextRequestId'));
          done();
        };
        client.open();

Index: app/store/env/sandbox.js
=== modified file 'app/store/env/sandbox.js'
--- app/store/env/sandbox.js 2013-10-10 17:00:09 +0000
+++ app/store/env/sandbox.js 2013-10-11 13:47:43 +0000
@@ -1066,12 +1066,12 @@
      @return {undefined} Side effects only.
      */
      handleClientWatchAll: function(data, client, state) {
- this.set('nextRequestId', data.RequestId);
- this.deltaIntervalId = setInterval(
- this.sendDelta.bind(this), this.get('deltaInterval'));
        // AllWatcherId can be hard-coded because we will only ever have one
        // client listening to the environment with the sandbox environment.
- client.receive({Response: {AllWatcherId: 42}});
+ client.receive({
+ RequestId: data.RequestId,
+ Response: {AllWatcherId: 42}
+ });
      },

      /**

« Back to merge proposal