-
Notifications
You must be signed in to change notification settings - Fork 42
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
Interoperability and event handler robustness #146
Interoperability and event handler robustness #146
Conversation
LGTM |
@@ -215,6 +215,7 @@ h3(#rest-channel). Channel | |||
** @(RSL5c)@ Tests must exist that encrypt and decrypt the following fixture data for "AES 128":https://github.com/ably/ably-common/blob/master/test-resources/crypto-data-128.json and "AES 256":https://github.com/ably/ably-common/blob/master/test-resources/crypto-data-256.json to ensure the client library encryption is compatible across libraries | |||
* @(RSL6)@ Message decoding | |||
** @(RSL6a)@ All messages received will be decoded automatically based on the @encoding@ field and the payloads will be converted into the format they were originally sent using i.e. binary, string, or JSON | |||
*** @(RSL6a1)@ A set of tests must exist to ensure that the client library provides data encoding & decoding interoperability with other client libraries. The tests must use the "set of predefined interoperability message fixtures":https://github.com/ably/ably-common/blob/master/test-resources/messages-encoding.json to 1) publish a raw message to the REST API using the JSON transport and subscribe to the message using Realtime to ensure the @data@ attribute matches the fixture; 2) publish a message using the REST client library and retrieve the raw message using the history REST API using the JSON transport ensuring the @data@ matches the fixture; 3) perform the client library operation using both @JSON@ and @MsgPack@ transports. For reference, see the "Ruby":https://github.com/ably/ably-ruby/pull/94 and "iOS":https://github.com/ably/ably-ios/pull/459 implementations |
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.
In 1) and 2), "using the JSON transport" is irrelevant (if I'm not mistaken here).
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.
I disagree strongly here, the transport is incredibly relevant, and the transport for injecting & retrieving the fixtures is only reliable over JSON with our current fixture data
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.
@tcard for clarity, what I am trying to achieve here is decoupling the action from the fixture. So there is always two stages.
In the case of testing decoding:
- Set up fixture (we do this over JSON only)
- Receive message from Ably using the client library and ensure the
data
object is exactly what we expected. Do this for MsgPack and JSON.
Then for encoding tests.
- Publish message using the Ably client lib using JSON & MsgPack
- Check the fixture (we do this over JSON only)
The problem I have with https://github.com/ably/ably-ios/pull/459/files#diff-8c3eccfb7635c28b9fadd0e4fcb6e6b0R3069 and https://github.com/ably/ably-ios/pull/459/files#diff-8c3eccfb7635c28b9fadd0e4fcb6e6b0R3114 is that you're conflating the ideas and thus missing out essential goals for our interoperability tests
c2bd932
to
cd55809
Compare
I've aded two commits here to address the problems of interoperability and an issue that @tcard highlighted that may affect some client libraries. I believe we can close https://github.com/ably/wiki/issues/22 too once done.
cc @paddybyers @tcard @SimonWoolf