-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(realtime): Testing echoMessages=true and echoMessages=false #226
test(realtime): Testing echoMessages=true and echoMessages=false #226
Conversation
I just thought of another solution that would avoid waiting for an arbitrary duration:
I'm not sure if this will work because I don't know if message ordering is guaranteed. Let me know if you'd like me to attempt this, but it would diverge from the tests I saw in the other languages. |
Yes it is for a channel in the same region, so that will work, that is definitely a better way of approaching it |
…nality * Removed tests that have to do with connection testing * Merged tests for true and false into one test * This new test does not depend on waiting on an arbitrary time period
@mattheworiordan I have combined both tests into one so that there is no wait for an arbitrary amount of time. I have removed some unnecessary tests as you suggested. |
testMsg2 = 'World!'; | ||
|
||
monitorConnection(test, rt1); | ||
monitorConnection(test, rt2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW. I think the monitorConnection method supports monitorConnection(test, rt1, rt2);
* Now checking incoming messages for ordering and channel. * Test now guarantees that messages are sent after listener are attached. * Factored test finalising code into finishTest().
…s arrive in wrong order
@mattheworiordan I have uploaded improved code. I think this version is better because it also checks for messages arriving in the expected order. There is a longer list of callbacks but hopefully with the comments I added it should be readable. Let me know. |
realtimeNoEchoChannel.subscribe('event0', function(msg) { | ||
receivedMessagesNoEcho.push(msg); | ||
finishTest(); | ||
}, function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this second function? The signature for subscribe is subscribe(name, callback(msg))
, there is no third aargument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there is an optional third argument which is a callback:
RealtimeChannel.prototype.subscribe = function(/* [event], listener, [callback] */) {
It allows me to do exactly what I wanted, i.e. to do stuff after the listener is subscribed to the channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's an undocumented API and I expect may change in the future, not sure why that's even in there as it's rather unrelated to the subscribe method itself. For clarity however, given the third callback is undocumented, I would prefer to simply use channel.attach(function(err) { ... do somethign .. })
over the subscribe as it's a lot clearer what is happening. Sorry once again, no idea why that argument exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea why that argument exists.
FWIW you and Paddy have discussed this before -- see 1b151df. Paddy argued strongly in favour of having a callback parameter (as opposed to throwing an exception) to notify a channel attach fail or a channel in a failed state, for a call which triggers an implicit attach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that is a different point entirely @SimonWoolf. This is a second callback on a subscribe
method which has in implicit attach, whereas that discussion was in relation to whether we throw an exception immediately or pass the exception to the callback. This callback is unrelated to that discussion as far as I can tell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the same issue as the discussion on whether to throw or call the callback in the already-failed case.
Apologies if I'm not being clear. Matt's comment was that he wasn't sure "why that argument exists". Since the outcome of the earlier discussion was that we should call the callback (rather than throw), that relies on that argument existing, hence why I linked to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattheworiordan OK I am trying to follow the discussion. I think I now understand the semantics of attach
and subscribe
after reading the docs and code more carefully. If I understand correctly attach
is an event that fires once a connection has been established, and subscribing to a channel's event can cause an implicit attach if the channel is not already attached.
I think I got it now, will attempt again and let you know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does
attach
rely onsubscribe
's 2nd callback argument?
The discussion linked was on a change in RealtimeChannel.prototype.subscribe
...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I am stupid, those pesky little plus signs are there to confuse people like me :) I do think we should still avoid using the undocumented callback API though as it's likely to change and testing it simply obfuscates the code and is likely to break in future. Sorry for the confusion.
…favor of channel.attach()
@mattheworiordan OK I believe I now call |
test(realtime): Testing echoMessages=true and echoMessages=false
At first I was hoping that something could be done so I could somehow query the server and have it tell me that all pending messages up to now have been sent. I scanned the code and docs for the SYNC message and for the ping/HEARTBEAT mechanism.
I thought I could send a message followed by a ping and then if I got the ping back but not the message, I could assume that the message was not echoed. Then I wouldn't have to stall the tests for an arbitrary amount of seconds.
When the above didn't work, I had a look at the implementations in other languages.
I set my timeout to 15 seconds.