JohnMcLear / draw

A real time collaborative drawing tool using nodejs, socket.io & paper.js
Apache License 2.0
483 stars 157 forks source link

Tests no longer working #140

Closed tranek closed 11 years ago

tranek commented 11 years ago

A lot of the tests are no longer working in the latest version.

The tests that fail on http://draw.etherpad.org differ slightly from localhost too. For example clear canvas works on etherpad.org but is failing on localhost.

tranek commented 11 years ago

It looks like the new edit bar at the top shifts the canvas down by 36 pixels so everything on the canvas is off by 36 pixels which means there is no longer a 1 to 1 correspondence between the position of items on the canvas and where they are in the actual browser (frame). So the fix is to add 36 to all simulated clicks and stuff...

tranek commented 11 years ago

I'm working on fixing them now btw. Got two fixed so far.

tranek commented 11 years ago

Seems that I didn't quite fix them 100%. Clear and Draw have a bug in them.

JohnMcLear commented 11 years ago

I probably changed some target ids

tranek notifications@github.com wrote:

A lot of the tests are no longer working in the latest version.

The tests that fail on http://draw.etherpad.org differ slightly from localhost too. For example clear canvas works on etherpad.org but is failing on localhost.

— Reply to this email directly or view it on GitHubhttps://github.com/JohnMcLear/draw/issues/140.

This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of the organisation from which this email originated. If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone. Please contact the sender if you believe you have received this email in error. This email was sent by School Email - Safe Webmail and Hosted Email for Schools

tranek commented 11 years ago

Nah. Right now 3 tests still bug out with the same problem. Simple drawing of paths aren't capping at the end which means I need to look at the simulated mouse up calls on those when I get a chance.

tranek commented 11 years ago

So the problem is that websockets were removed. When I commented out the io.set transports code, it fixed all the tests.

Does the Joyent server use Varnish? If not, can we add websockets back in?

This doesn't solve the problem where the tests fail with xhr polling though, that needs to be looked at still. Oddly enough, xhr polling on localhost is perfectly fine. It's xhr polling on a remote server that causes issues.

Also, I wonder if xhr polling vs websockets have any effect on that notorious crash that we have been having.

JohnMcLear commented 11 years ago

joyent does not use varnish

we can add it but imho the real fix here may be a move away from socketio, shouldnt be a huge job to migrate tho

tranek commented 11 years ago

What's the alternative to socketio? I'm still going to look into why xhr polling isn't working with the tests.

JohnMcLear commented 11 years ago

sockjs and others. search ?

tranek commented 11 years ago

search ?

Sorry thought you had some in particular in mind.

tranek commented 11 years ago

OK I have a pretty solid hypothesis and solution. I think what's happening is that when we simulate a drag (mouse down + mouse up) and then reload the pad to make sure that it's on the server, the xhr polling polls one message at a time and doesn't have enough time before we reset the connection to read both the draw:progress and the draw:end messages.

By putting a 1 second delay after simulating drawing the path, it gives the server enough time to read both the draw:progress and the draw:end messages. There might be something in socketio settings to change how often it polls, but whatever, I think adding a 1 second delay won't hurt anyone.

Actually, even a half second delay allows it to work.

I will add these tiny delays to all the tests and report back. If that works, I don't think that there would be a need to replace SocketIO.

JohnMcLear commented 11 years ago

Ideally you would have this more event based, doing stuff using timer/delays is usually bad practice. Sounds good test though, keep me posted :)