Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Broadcast Test Fails #42

Closed
Radagaisus opened this Issue Sep 8, 2012 · 6 comments

Comments

Projects
None yet
3 participants

I tried to fix this myself, but nothing seems to work:

sockets.coffee: server broadcasts
----------------------------------------
✖ reached1 (not reached)
✖ reached2 (not reached)
✖ data1 (not reached)
✖ data2 (not reached)

Using io.sockets.send works, but it's not really a broadcast (because the sender will receive the message as well).

Owner

shimaore commented Sep 9, 2012

Yes, they have been failing (I think since the code was added); but if you only test that specific block then it works just fine. (Remove all .coffee files from test/ except for index.coffee and sockets.coffee, edit sockets.coffee to only leave the last test, and it passes.) I suspect this is a bug in socket.io but I've never had the time to chase it down.

The test still fails in isolation.

Owner

shimaore commented Sep 15, 2012

Argh. That's because I had been testing the wrong thing -- the last test in tests/sockets.coffee is not the broadcast test.

OK, so I tested with independent processes:

s.coffee

#!/usr/bin/coffee
require('zappajs') 15001, ->
  @on 'shout': ->
    @broadcast 'shouted', @data

c1.coffee

#!/usr/bin/coffee
io = require 'socket.io-client'
socket = io.connect("http://127.0.0.1:15001/")
socket.emit 'shout', {foo: 'bar'}

c2.coffee

#!/usr/bin/coffee
io = require 'socket.io-client'
socket = io.connect("http://127.0.0.1:15001/")
socket.on 'shouted', (data) ->
  console.log 'reached1'
  if data.foo is 'bar'
    console.log 'data1'

c3.coffee: same as c2.coffee, replace reached1 and data1

Start s.coffee, c2.coffee, c3.coffee (or multiple instances of c2), then c1.coffee, and things work.

So (we had kind of guessed that) zappa is correct; but either (a) the test driver as it is written in zappa is incorrect (one of the tools under tests/support/ makes wrong assumptions? the test isn't written properly?) or (b) socket.io-client doesn't work well with multiple instances in the zappa process connecting to the same server.

I then modified c2.coffee:

#!/usr/bin/coffee
io = require 'socket.io-client'
socket = io.connect("http://127.0.0.1:15001/")
socket.on 'shouted', (data) ->
  console.log 'reached1'
  if data.foo is 'bar'
    console.log 'data1'

socket2 = io.connect("http://127.0.0.1:15001/")
socket2.on 'shouted', (data) ->
  console.log 'reached2'
  if data.foo is 'bar'
    console.log 'data2'

and it still worked! So looks like socket.io-client is fine and we need to look into either the test itself, or the test driver.

The problem has nothing to do with broadcast, that's a red herring. The issue seems to be that the first client is the only one to connect. If I put a 'on connection' handler into the test app, it only recognizes the first connection. The 'server rooms' test passes because the clients initiate the connection to the server. This test fails because the server does not know who the clients are. Working on it now.

Ahh, I understand now - the app has one property for the socket, which notifies the server of connection on first creation. Subsequent calls to connect reuse the same socket, which is incorrect. The connect method should return a handle, which event handlers are attached to (just in the test client [for now]) so that specific connections can be checked / tested.

datashaman added a commit to datashaman/zappajs that referenced this issue Aug 18, 2013

datashaman added a commit to datashaman/zappajs that referenced this issue Aug 18, 2013

Merge branch 'feature/issue-42' into develop
* feature/issue-42:
  Force new connection on io.connect #42

shimaore added a commit that referenced this issue Aug 18, 2013

Merge pull request #90 from datashaman/feature/issue-42
Force new connection on io.connect #42

Whoah! :)

This can be closed.

@shimaore shimaore closed this Aug 28, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment