Socket.io Testing #865
Socket.io Testing #865
Conversation
… which does resolve to 1.3.6 right now but is different fork (i guess)
| @@ -57,7 +57,7 @@ | |||
| "passport-twitter": "^1.0.2", | |||
| "phantomjs": ">=1.9.0", | |||
| "serve-favicon": "^2.3.0", | |||
| "socket.io": "^1.3.5", | |||
| "socket.io": "Automattic/socket.io", | |||
There was a problem hiding this comment.
I'm a little bit concerned and maybe we should wait for the next socket.io version. Unclear when it's released tho.
This line equals "socket.io": "socketio/socket.io",
There was a problem hiding this comment.
@bastianwegge I'm skeptical about having the development version of socket.io brought into the project.
There was a problem hiding this comment.
@codydaig I was talking to @mleanos about that. What we need would be the development version of socket.io-client for testing, since it has the extraHeaders included. This is the only reasonable way to test socket.io at the moment. Other ways include varieties of hacks, that are not worth including. I'll give this a shot in about 4-5 hours (when it's evening time and I'm off work).
There was a problem hiding this comment.
@bastianwegge I agree that's the most reasonable way of testing. Using extraHeaders is what @mleanos and I came up with while debugging, it just isn't in the stable version yet.
IMO, if you want to write the tests for socket.io and put them in the repository as pending, that's fine. But I don't want the project to rely on a development version of a dependency.
There was a problem hiding this comment.
There was a problem hiding this comment.
I agree with @codydaig that we don't want to introduce the development version of socket.io. I think there's a simple solution to this issue. How about we just install the development version of socket.io-client? Socket.IO exposes the internal socket.io-client that's installed with the package which is exposed to this app via https://github.com/meanjs/mean/blob/master/modules/core/server/views/layout.server.view.html#L52
If we install the stand-alone socket.io-client then we can use this version in our tests. I believe our app will still use the socket.io-client installed with socket.io. Although, I'm not sure of this; my initial guess is that it won't have any impact at all. However, maybe someone else has some insight into that concern.
So the changes to package.json would look like this
"socket.io": "1.3.6",
"socket.io-client": "https://github.com/socketio/socket.io-client#master"
There was a problem hiding this comment.
+1 to use socket.io-client head for testing until extraHeaders is on release
There was a problem hiding this comment.
@codydaig pointed out that the socket.io-client package isn't installed with socket.io if the stand-alone socket.io-client is installed. So given that behavior, my above proposal may not be viable.
Perhaps, we will just have to wait for engine.io-client to update the npm package to their latest version. I wouldn't expect to be waiting too long for this to happen.
In the meantime, we could work on building these tests & polishing up this PR in anticipation for the engine.io-client project to be updated.
There was a problem hiding this comment.
Didn't you checkout the difference from the 1.3.6 to the head was the dependency of the socket.io-client?
There was a problem hiding this comment.
@bastianwegge Yes. I saw that. I was hoping that we could install the stand-alone HEAD version of socket.io-client, and still use the version installed with socket.io. Guess that's not the case, as Cody pointed out. I say we sit tight and work on these tests in the meantime.
|
@mleanos @bastianwegge What's the status of this PR? |
|
@codydaig I'm in contact with the owner of socketio and a couple hours earlier he started a survey on what to improve next. https://docs.google.com/forms/d/1GjvjTmy-ax9GiqbSQm_BYzECKpkUP_xyQrKGprjFfis/viewform?c=0&w=1 |
|
@bastianwegge Cool. Thanks for the update! |
|
@codydaig I suppose we're just waiting for the engine.io package to be updated in NPM, since we can't properly test socketio atm. |
|
@mleanos @bastianwegge @codydaig is the latest engine.io update enough to finish this PR? |
|
The feature is not included in the release as far as I can see. |
|
Which feature do we need? Is there any way around waiting for engine.io? |
|
Since I'm using it in one of my projects and spent a complete day on this, the final decision for me was a forked release. Is this an option for you? |
|
@bastianwegge what is missing that you need a forked release for? |
|
Read the PR or one of the 2 links mleanos and I posted here. |
|
@bastianwegge If |
|
So your suggestion is requiring the latest |
|
It's already included in the latest socket.io https://github.com/socketio/socket.io/blob/master/package.json and we have the latest https://github.com/socketio/socket.io/blob/master/package.json Everything should be doable. |
|
I'm gonna try it tomorrow evening. Thanks for pointing that out! |
|
@bastianwegge Any update on this PR? |
|
@bastianwegge @mleanos can we wrap up this PR and get it merged in? |
|
Sorry, already wanted to give an update. Tried to get it to work the last couple of days but it seems like the latest version upgrades broke the functionality to send messages to sockets completely. |
|
@bastianwegge what code doesn't work exactly? Socket.io is pretty widely used, I can't imagine that haven't released a new version if there is an issue. |
|
And with this I'm out. If anyone's interested in driving this one, you're welcome. |
|
@mleanos do you think you could try to finish up this PR? Socket testing is extremely important, and anything we can do to get it in would be amazing. If you can, please start by rebasing and then I'll try to help you out by looking into what tests fail and how to fix them. If not, I'll try to finish this up in a new PR. |
|
@ilanbiala I'm looking into this right now. It's been a while since I was working on this. Good news, is that it looks like upgrading to socketio @codydaig Do you happen to have the test code that were working together on, for this? I must have deleted the branch. |
|
@mleanos or you could just continue where this PR left off |
|
@mleanos I do not. :( |
Hey guys, just wanted to share something that's actually working for me. I worked with @mleanos over the weekend on this. The Automattic/socket.io in the package.json links to the latest socket.io Library, which does seem to have one or more bugfixes which makes the
extraHeaderswork.@codydaig @rhutchison @lirantal
! This is work-in-progress !