Skip to content
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

RTN12c #163

Merged
merged 1 commit into from
Jan 27, 2016
Merged

RTN12c #163

merged 1 commit into from
Jan 27, 2016

Conversation

ricardopereira
Copy link
Contributor

Passing

@@ -414,6 +414,13 @@ class TestProxyTransport: ARTWebSocketTransport {
super.receive(msg)
}

func closeAbruptly() {
// Inject a ProtocolMessage.CLOSE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a valid mock, because if the real transport does indeed close abruptly it won't make up a ProtocolMessage.CLOSE. I guess the connection should be notified of the close somehow (with a callback, I guess) and then check the current state, so that if it is CLOSING it transitions to CLOSED.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this is completely the wrong behaviour. We want the transport on(closed) event equivalent to be triggered by the test suite so that the client library thinks the transport was abruptly terminated.

@ricardopereira
Copy link
Contributor Author

Rebased with changes.

@ricardopereira
Copy link
Contributor Author

DISCONNECTED (6)
Passed by the service to a client in response to a DISCONNECT message or if a connection has been disconnected because of an error state with the error reason included in the DISCONNECT Protocol Message.

https://github.com/ably/ably-ios/blob/b755c200717e81188651c8bdb19bf71f31b9d7d6/ably-ios/ARTWebSocketTransport.m#L251 I think the ARTWsAbnormalClose, ... should also call the delegate.realtimeTransportClosed instead of delegate.realtimeTransportDisconnected.

@mattheworiordan
Copy link
Member

LGTM

1 similar comment
@tcard
Copy link
Contributor

tcard commented Jan 27, 2016

LGTM

@ricardopereira
Copy link
Contributor Author

It's important to merge #169 first, remove 435317c, rebase with master and check if it's ok.

ricardopereira added a commit that referenced this pull request Jan 27, 2016
@ricardopereira ricardopereira merged commit 98e5d41 into master Jan 27, 2016
@ricardopereira ricardopereira deleted the RTN12c branch January 27, 2016 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants