-
Notifications
You must be signed in to change notification settings - Fork 26
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 suite websocket #169
Test suite websocket #169
Conversation
…ARTProtocolMessage
…e, simulateIncomingError and simulateLostConnection
- Tests should use each event (onConnected, onError, onDisconnected, onSuspended…) to change the connection state because using the `transition` method will ignore some logic.
@tcard What do you think? |
@@ -446,7 +446,7 @@ -(void) cancelPingTimer { | |||
self.pingTimeout = nil; | |||
} | |||
|
|||
- (void)onHeartbeat:(ARTProtocolMessage *)message { | |||
- (void)onHeartbeat { |
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.
Why did you remove the ProtocolMessage?
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.
Because the method was not using it.
Looks fine, few comments re: naming |
ba7dbc3
to
5a6072c
Compare
LGTM |
I was using
client.transition
to simulate incomings of errors & connection state change and it is wrong because some of the logic is missed.To change the connection states:
client.onConnected(message: ARTProtocolMessage)
client.onDisconnected()
client.onSuspended()
client.onError(AblyTests.simulateError())
With this PR we have:
simulateIncomingNormalClose
simulateIncomingAbruptlyClose
simulateIncomingError
simulateLostConnection