From d7df0a04ca91a277d96710704c277151348ff7d4 Mon Sep 17 00:00:00 2001 From: Dan Mosedale Date: Thu, 6 Feb 2014 20:50:53 -0800 Subject: [PATCH 1/5] Tests and implements connection in the server constructor, and tweaks behavior a bit in the anonymous case. --- static/spa/talkilla/js/spa.js | 22 +++++-- test/frontend/spa/talkilla_spa_test.js | 84 ++++++++++++++++++++++---- 2 files changed, 88 insertions(+), 18 deletions(-) diff --git a/static/spa/talkilla/js/spa.js b/static/spa/talkilla/js/spa.js index 048ac4b9..335fa80e 100644 --- a/static/spa/talkilla/js/spa.js +++ b/static/spa/talkilla/js/spa.js @@ -25,6 +25,12 @@ var TalkillaSPA = (function() { this.server.on("reconnection", this._onServerEvent.bind(this,"reconnection")); this.server.on("message", this._onServerMessage.bind(this)); + + // so we can get instantshare notifications without being logged in + // XXX get rid of this in favor of something that doesn't require + // an open connection and server round trip + // (eg indexedDB or proxied postMessage) + this.server.connect(); } TalkillaSPA.prototype = { @@ -38,11 +44,17 @@ var TalkillaSPA = (function() { else if (type === "reconnection") this.port.post("reconnection", (new payloads.Reconnection(event))); else if (type === "connected") { - this.port.post(type, { - addresses: [{type: "email", value: this.email}], - capabilities: this.capabilities - }); - this.server.presenceRequest(); + if ("email" in this) { + this.port.post(type, { + addresses: [{type: "email", value: this.email}], + capabilities: this.capabilities + }); + this.server.presenceRequest(); + } else { + this.port.post(type, { + capabilities: this.capabilities + }); + } } else this.port.post(type, event); diff --git a/test/frontend/spa/talkilla_spa_test.js b/test/frontend/spa/talkilla_spa_test.js index 7b7724e0..4e75e138 100644 --- a/test/frontend/spa/talkilla_spa_test.js +++ b/test/frontend/spa/talkilla_spa_test.js @@ -1,5 +1,5 @@ /* global sinon, SPAPort, Server, TalkillaSPA, expect, payloads */ -/* jshint unused:false */ + "use strict"; describe("TalkillaSPA", function() { @@ -12,6 +12,8 @@ describe("TalkillaSPA", function() { sandbox = sinon.sandbox.create(); port = new SPAPort(); server = new Server(); + + sandbox.stub(server, "connect"); spa = new TalkillaSPA(port, server, {capabilities: ["call", "move"]}); }); @@ -21,23 +23,79 @@ describe("TalkillaSPA", function() { expect(spa.capabilities).eql(["call", "move"]); }); + it("should call the server's connect method", function () { + sinon.assert.calledOnce(server.connect); + sinon.assert.calledWithExactly(server.connect); + }); }); describe("#_onServerEvent", function() { - it("should post a connect event to the port", function() { - spa.email = "foo"; - sandbox.stub(spa.port, "post"); + describe("connected", function () { - spa.server.trigger("connected"); + // XXX email shouldn't be a public member of the API, I don't think, + // and as a result, and we should be avoiding using it to do the tests + // I suspect we really want to be testing the connect and connected + // events as a pair. + + it("should post a connected event containing the email address " + + "to the worker port if spa.email is set", function() { + + spa.email = "foo"; + sandbox.stub(spa.port, "post"); + + spa.server.trigger("connected"); + + sinon.assert.calledOnce(spa.port.post); + sinon.assert.calledWithExactly( + spa.port.post, "connected", { + addresses: [{type: "email", value: "foo"}], + capabilities: ["call", "move"] + } + ); + }); + + it("should post a connected event to the worker port if " + + "not containing an email address is spa.email is not set", function() { + + delete spa.email; + sandbox.stub(spa.port, "post"); + + spa.server.trigger("connected"); + + sinon.assert.calledOnce(spa.port.post); + sinon.assert.calledWithExactly( + spa.port.post, "connected", { + capabilities: ["call", "move"] + }); + }); + + it("should post a presenceRequest to the server if spa.email is set", + function() { + spa.email = "foo"; + sandbox.stub(spa.port, "post"); + sandbox.stub(spa.server, "presenceRequest"); + + spa.server.trigger("connected"); + + sinon.assert.calledOnce(spa.server.presenceRequest); + sinon.assert.calledWithExactly(spa.server.presenceRequest); + }); + + + it("should not post a presenceRequest to the server if spa.email" + + " is not set", function() { + + delete spa.email; + + sandbox.stub(spa.port, "post"); + sandbox.stub(spa.server, "presenceRequest"); + + spa.server.trigger("connected"); + + sinon.assert.notCalled(spa.server.presenceRequest); + }); - sinon.assert.calledOnce(spa.port.post); - sinon.assert.calledWithExactly( - spa.port.post, "connected", { - addresses: [{type: "email", value: "foo"}], - capabilities: ["call", "move"] - } - ); }); it("should post a reconnection event to the port", function() { @@ -80,7 +138,7 @@ describe("TalkillaSPA", function() { describe("#_onConnect", function() { it("should connect to the server", function() { - sandbox.stub(spa.server, "connect"); + spa.server.connect.reset(); // already stubbed, reset the state // The Talkilla SPA doesn't need any credentials. This is // handled via cookies. From 21a6e8e39d32bcf0193f081aed470e4316eae52a Mon Sep 17 00:00:00 2001 From: Dan Mosedale Date: Fri, 7 Feb 2014 10:06:19 -0800 Subject: [PATCH 2/5] Apply minimal version of long-polling fix from Standard8 --- server/presence.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/presence.js b/server/presence.js index 276fdec5..11f34a8a 100644 --- a/server/presence.js +++ b/server/presence.js @@ -123,9 +123,9 @@ api = { var user; if (isAnon) - user = api._setupUser(anons, api._genId()); + user = api._setupUser(anons, api._genId(), firstRequest); else - user = api._setupUser(users, req.session.email); + user = api._setupUser(users, req.session.email, firstRequest); user.touch(); From 537b7258622714120f9b1e3ae54c8bfb6b50538d Mon Sep 17 00:00:00 2001 From: Dan Mosedale Date: Fri, 7 Feb 2014 11:22:07 -0800 Subject: [PATCH 3/5] Add more logging to the /stream endpoint --- server/presence.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/presence.js b/server/presence.js index 11f34a8a..bf16e7a1 100644 --- a/server/presence.js +++ b/server/presence.js @@ -122,6 +122,9 @@ api = { var isAnon = !req.session.email; var user; + logger.debug({firstRequest: firstRequest, email: !!req.session.email}, + "/stream connection made"); + if (isAnon) user = api._setupUser(anons, api._genId(), firstRequest); else From 58c41c98115c000ebc4a6a44e211bab8379ef3e3 Mon Sep 17 00:00:00 2001 From: Dan Mosedale Date: Fri, 7 Feb 2014 11:43:25 -0800 Subject: [PATCH 4/5] Apply minimal test fix from Standard8's long-polling PR --- test/server/presence_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/server/presence_test.js b/test/server/presence_test.js index 7048bdfb..51b05bca 100644 --- a/test/server/presence_test.js +++ b/test/server/presence_test.js @@ -415,7 +415,7 @@ describe("presence", function() { // away from testing the impl with stubs and instead look at the // side effect on anons for this specific test sinon.assert.calledOnce(api._setupUser); - sinon.assert.calledWithExactly(api._setupUser, anons, fakeId); + sinon.assert.calledWithExactly(api._setupUser, anons, fakeId, false); }); }); From 6688f929b3f4d65dc831b6d05b313b96b0f7726b Mon Sep 17 00:00:00 2001 From: Dan Mosedale Date: Fri, 7 Feb 2014 13:54:57 -0800 Subject: [PATCH 5/5] Revert unnecessary call to connect in SPA constructor and test --- static/spa/talkilla/js/spa.js | 5 ----- test/frontend/spa/talkilla_spa_test.js | 6 ------ 2 files changed, 11 deletions(-) diff --git a/static/spa/talkilla/js/spa.js b/static/spa/talkilla/js/spa.js index 335fa80e..df0144d6 100644 --- a/static/spa/talkilla/js/spa.js +++ b/static/spa/talkilla/js/spa.js @@ -26,11 +26,6 @@ var TalkillaSPA = (function() { this._onServerEvent.bind(this,"reconnection")); this.server.on("message", this._onServerMessage.bind(this)); - // so we can get instantshare notifications without being logged in - // XXX get rid of this in favor of something that doesn't require - // an open connection and server round trip - // (eg indexedDB or proxied postMessage) - this.server.connect(); } TalkillaSPA.prototype = { diff --git a/test/frontend/spa/talkilla_spa_test.js b/test/frontend/spa/talkilla_spa_test.js index 4e75e138..cbb51c9e 100644 --- a/test/frontend/spa/talkilla_spa_test.js +++ b/test/frontend/spa/talkilla_spa_test.js @@ -22,11 +22,6 @@ describe("TalkillaSPA", function() { expect(spa.capabilities).to.be.a("array"); expect(spa.capabilities).eql(["call", "move"]); }); - - it("should call the server's connect method", function () { - sinon.assert.calledOnce(server.connect); - sinon.assert.calledWithExactly(server.connect); - }); }); describe("#_onServerEvent", function() { @@ -138,7 +133,6 @@ describe("TalkillaSPA", function() { describe("#_onConnect", function() { it("should connect to the server", function() { - spa.server.connect.reset(); // already stubbed, reset the state // The Talkilla SPA doesn't need any credentials. This is // handled via cookies.