Add implicit attach test cases for channel publish and subscribe #115
Conversation
gokhanbarisaker
force-pushed
the
channel-test-implicitattach
branch
2 times, most recently
from
e7d79a3
to
1510587
Compare
|
LGTM, a few small comments |
|
What would you like me to use for error code? Is 90001 fine for Presence#get(...)? |
|
LGTM, @paddybyers you happy for this to be merged now? |
| * @return: the current present members. | ||
| * @throws AblyException | ||
| */ | ||
| public synchronized PresenceMessage[] get() { | ||
| public synchronized PresenceMessage[] get() throws AblyException { | ||
| if (channel.state == ChannelState.failed) { |
There was a problem hiding this comment.
Why is this check here, and not on the other methods below?
Edit: in fact, doesn't the attach() already throw if the state is failed ?
There was a problem hiding this comment.
My current focus is on Presence with this pull request. If other methods have features like RTP11b, I am happy to implement that for them also
There was a problem hiding this comment.
If the channel is in the failed state, does the attach() throw an exception? If so, the check above (l44) can be removed.
There was a problem hiding this comment.
Currently, channel.attach() does not throw an exception for it. Would you like me to move the state check to channel.attach(), instead of presence.get()?
There was a problem hiding this comment.
Would you like me to move the state check to channel.attach(), instead of presence.get()?
No, we think that the state should be checked in Presence.get(), only calling attach() if not failed.
So please add that check to the other get() variants below.
See my comment above. |
Spec: RTP11b
gokhanbarisaker
force-pushed
the
channel-test-implicitattach
branch
from
51b8d17
to
0a0fda8
Compare
|
|
||
| /* wait until connected */ | ||
| new ConnectionWaiter(ably.connection).waitFor(ConnectionState.connected); | ||
| assertEquals("Verify failed state reached", ably.connection.state, ConnectionState.connected); |
There was a problem hiding this comment.
This is verifying that the connected state is reached, not failed as in the message. (This problem also exists in the test cases below.)
There was a problem hiding this comment.
holy copy & paste, my bad. fixing now
|
See my comments on the tests. I stopped adding comments because I think they all have the same problem. |
gokhanbarisaker
force-pushed
the
channel-test-implicitattach
branch
from
8b05c08
to
309cf47
Compare
|
Thanks |
…implicitattach
…implicitattach
…implicitattach
…implicitattach
…implicitattach
…implicitattach
…implicitattach
…implicitattach
Resolves: #45