-
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
[ECO-4786] Document private API usage in tests #1756
Conversation
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.
Great investigation of private API usage!
So it seems like we would still need to figure out what to do with stuff like:
- accessing private client options like
webSocketConnectTimeout
; - accessing
channel.channelOptions
,rest.serverTimeOffset
,realtime.connection.connectionManager.options
properties; - modifying values for
suspendedChannel.state
,connectionManager.lastActivity
; - private method calls
realtime.connection.connectionManager.disconnectAllTransports();
as they are not something that proxy server solves for us.
And I have a couple of more concerete questions/thoughts on some points below.
- `var PresenceMessage = Ably.Realtime.PresenceMessage;` | ||
- replacing `channel.sendPresence` with a version that checks the presence message’s client ID | ||
- replacing `transport.send` with a version that checks the encoded data in the protocol message | ||
- `channel.presence.members.waitSync(cb);` |
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.
not sure what this and channel.sync();
below does, might be tricky to replace in UTS
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.
Didn't give it any thought whilst working on this document, but:
waitSync
— might be something we can replace withpresence.get()
withwaitForSync
set totrue
sync
— yeah, don't know what that does; the end result is that it sends aSYNC
protocol message (which I can't even see the spec mentioning as something the client should do, except for RTP19 which describes how a test should behave; I’m probably missing something though)
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.
thanks! Will keep this open as it might be useful for others to see the discussion
- replacing `transport.send` with a version that checks the encoded data in the protocol message | ||
- `channel.presence.members.waitSync(cb);` | ||
- `var connId = realtime.connection.connectionManager.connectionId;` | ||
- `channel.presence._myMembers.put(` |
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.
can this API be replaced with proxy functionality? Like inject a presence message to add a member.
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 can't answer this question with much certainty without spending some time reminding myself of how presence works. But the thing is that the feature that this is testing — automatic presence re-entry, as described by RTP17g — is specified in terms of a private API; that is, the spec point refers to the "internal PresenceMap
".
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.
So if you have a spec point that says "some action needs to be done for all members of this private data structure", it's fair enough for the test to insert something into that private data structure to check that the action is done for 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.
The fact that some of the library behaviour is specified in terms of private API means that it's unlikely we'll be able to completely remove private API usage from the tests.
Yeah, I haven't thought about those at all. If following my suggested approach in the protocol message interception RFC —
— we’d begin by seeing whether we can rewrite the test to not use the private API. |
Resolves ECO-4786.
812851e
to
28523ba
Compare
Gonna close this PR; superseded by ECO-4821 and ECO-4834. |
Resolves ECO-4786.