RTN22 #537

ricardopereira · 2016-11-22T12:41:51Z

No description provided.

mattheworiordan · 2016-11-23T01:41:30Z

Spec/RealtimeClientConnection.swift

+
+                    let authMessage = ARTProtocolMessage()
+                    authMessage.action = .Auth
+                    transport.receive(authMessage)


We may in fact not need to stub which makes the tests more resilient. Why not issue a token with a ttl of 35 seconds, and within 5 seconds you will receive the AUTH protocol message naturally. That way all of this stubbing becomes unnecessary

mattheworiordan · 2016-11-23T01:42:47Z

Spec/RealtimeClientConnection.swift

+                    let disconnectedMessage = ARTProtocolMessage()
+                    disconnectedMessage.action = .Disconnected
+                    disconnectedMessage.error = ARTErrorInfo.createWithCode(40140, message: "token error (unspecified)")
+                    transport.receive(disconnectedMessage)


Again, why stub, why not simply issue a token with a short TTL of say 10 seconds, wait for the disconnect, and then follow the rest of the steps? I really think we should be using stubbing more sparingly generally, unless it's unavoidable. In most cases I do not believe it is.

mattheworiordan · 2016-11-23T01:43:12Z

Spec/RealtimeClientConnection.swift

+                    waitUntil(timeout: testTimeout) { done in
+                        client.connection.once(.Connected) { stateChange in
+                            expect(stateChange?.reason).to(beNil())
+                            expect(initialToken).toNot(equal(client.auth.tokenDetails?.token))


Please check the connection was resumed successfully

mattheworiordan

Would like to see less fabrication of protocol messages where not needed, but otherwise good

mattheworiordan · 2016-11-25T09:56:59Z

Spec/RealtimeClientConnection.swift

+                    }
+
+                    var authorizeMethodCallCount = 0
+                    let hook = client.auth.testSuite_injectIntoMethodAfter(#selector(client.auth.authorize)) {


I would prefer to not see this injection as it's not necessary

Done dbcaec8.

mattheworiordan · 2016-12-02T16:13:17Z

Are you awaiting final review? Seems good, but would be good to make sure connections are actually resumed correctly in both cases.

tcard · 2016-12-05T13:06:05Z

Spec/RealtimeClientConnection.swift

+                    expect(transport.protocolMessagesReceived.filter{ $0.action == .Auth }).toEventually(haveCount(1), timeout: testTimeout)
+
+                    waitUntil(timeout: testTimeout) { done in
+                        client.connection.once(.Connected) { stateChange in


This is racy. The expectation above can pass after this event is fired. The event listener should be set up before. In fact, you can just remove the expectation above and check that the AUTH protocol message was received inside the CONNECTED event listener.

Yes, you're right. Will fix.

tcard · 2016-12-05T13:08:36Z

Spec/RealtimeClientConnection.swift

+                    let options = AblyTests.commonAppSetup()
+                    options.autoConnect = false
+                    options.disconnectedRetryTimeout = 1.0
+                    options.token = getTestToken(key: options.key!, ttl: 5.0)


Five seconds isn't a bit too much?

tcard · 2016-12-05T13:08:51Z

Spec/RealtimeClientConnection.swift

+                it("Ably can request that a connected client re-authenticates by sending the client an AUTH ProtocolMessage") {
+                    let options = AblyTests.commonAppSetup()
+                    options.autoConnect = false
+                    options.token = getTestToken(ttl: 5.0)


Five seconds isn't a bit too much?

Recommended by @mattheworiordan

This looks a bit brittle, 2s to connect & attach in CI may fail, can we not just give it a bit of buffer, say 6 seconds to make sure we don't get test failures because of delays in running code?

Well, OK then. I'd say you can just mock the AUTH message from the server, but well, I guess we want the real thing.

tcard · 2016-12-05T13:10:46Z

Spec/RealtimeClientConnection.swift

+                    expect(client.connection.id).to(equal(initialConnectionId))
+                    expect(authorizeMethodCallCount) == 1
+
+                    waitUntil(timeout: testTimeout) { done in


What is this whole block checking?

This block is checking if the channel was resumed successfully (recommended by @mattheworiordan).

test that uses a low value TTL for the token, attaches to a channel, then waits for the auth to complete renewing the token, and then publish over REST to that channel to make sure it's received?

It's just a fail safe to make sure that the reauth is in fact working and there is continuity over the channels

ricardopereira · 2016-12-13T00:05:27Z

@mattheworiordan passing. PTAL

mattheworiordan

LGTM

@synchronized

* Start 0.9 version * TO3k7 (#513) * Start 0.9 version * Add fallbackHostsUseDefault option * Fallback: add initialiser accepting client options * TO3k7 * Add ARTFallback+Private * Default fallbackHosts as Array of Strings * fixup! TO3k7 * fixup! Add fallbackHostsUseDefault option * fixup! TO3k7 * Update RSC15a for 0.9 (#515) * Start 0.9 version * Update RSC15a: default fallback hosts * Default fallbackHosts as Array of Strings * RSA4a (#517) * Test suite: add TestProxyHTTPExecutor.simulateIncomingServerErrorOnNextRequest * RSA4a * Fix Realtime: indicate an error and not retry the request when the server responds with a token error * Update RSC15e (#514) * Remove specs RSA10c and RSA10d (#522) * RSA10l (#524) * Auth: deprecate `authorise` in favor of `authorize`. * RSA10l * Use `authorize` instead of `authorise` (close #496) * Remove `AuthOptions.force` (#527) * Remove AuthOptions.force * Fix: make a single attempt to reissue the token and resend the request * Fix RSA10a * Fix RSC9 * Remove `prepareAuthorisationHeader` access from test suite * Fix RSA10a * Update RSC15b for 0.9 (#516) * Update RSC15b * Fix: REST fallback should only apply when the default host is used * Update RSA10a for 0.9 (#520) * Update RSA10a * Fix: authorize should change auth method to Token for future requests * Update RSA10j for 0.9 (#521) * Update RSA10j * Fix: ttl when omitted should set the default value * Fix RTN15h * RTC8 (#526) * Add ARTAuthDetails * RTC8a * RTC8a1 (part 1) * RTC8a1 (part 2) * RTC8a1 (part 3) * RTC8a2 * RTC8a3 * RTC8b * RTC8b1 * RTC8c * Send AUTH protocol message on each authorize * Fix RTC8 * Test suite: `splitDone`, when a test fails, get the right location of the failure * RSA4b (#518) * RSA4b * Fix REST: should retry the request once if a token error occurs * Fix Realtime: if the token creation failed then the connection should move to the DISCONNECTED state and reports the error * RSA4c (#519) * RSA4c * Fix: 80019 and description of the underlying failure should be emitted * Fix: if connected and the client receives an ARTProtocolMessageAuth, then authorise * Fix: if authUrl or authCallback fails and is CONNECTED then the connection should remain CONNECTED - RSA4c3 * Add `artDispatchScheduled` and a way to cancel the scheduled block * Fix: authUrl/authCallback attempt times out after realtimeRequestTimeout * Test suite: reset networkConnectEvent after spec * Enhance: debug info * fixup! Fix: authUrl/authCallback attempt times out after realtimeRequestTimeout * fixup! Fix: if connected and the client receives an ARTProtocolMessageAuth, then authorise * Fix: every realtime auth attempt should check if deadline is reached * Rename createWithNSError to createFromNSError * Fix: remove Auth dependency from WebSocketTransport * Fix: should first authorize and then connect the transport * RTN22 (#537) * RTN22 * RTN22a * Fix: realtime transport can be nil * Fix: realtime should renew token by transitioning to CONNECTING * Update RTL3 for 0.9 (#544) * Update RTL3a * Update RTL3b and RTL3c * RTL3d * RTL3e * RTL14 (#550) * UPDATE event (#559) * UPDATE event (replace ERROR event) * Fix: Connection should emit an UPDATE event * Fix RTC8a1 * Fix specs and legacy tests * RTN4h * RTN4f * RTN24 * Update RTL2 for 0.9 (#543) * Add Realtime Channel Suspended state * Add ProtocolMessageActionToStr method * Add ChannelStateChange type * Update RTL2 * RTL2f: pending - functionality hasn't been deployed * Use ChannelStateChange on channel event emitter * Test suite: simulate client suspension with before suspension callback * Update tests using channel events * Fix RTL14 * Fix: Channel on suspended should transition to SUSPENDED state * Remove RTN18 * Fix: set Suspended on all channels when Connection moves to Suspended * RTL2g * Fix RTL12 * Fix RTL3d * Fix: channel should reattach when connection is Connected * Fix: should resume connection when the connection is Suspended * Fix RTN11 * Fix RTL3e * Fix RTC8a1 * Remove testSuspendingDetachesChannel * Fix RTL3d * Fix: channel is SUSPENDED then operation will result in an error * Update RTL13 for 0.9 (#549) * RTL13a * RTL13b * RTL13c * Add ClientOptions.channelRetryTimeout * RealtimeChannel reattach after timeout * Fix: if the channel receives a server initiated DETACHED message and if the channel is in the ATTACHED or SUSPENDED states, then an attempt to reattach the channel should be made immediately * Fix: move to Suspended if attach times out * Fix: if the channel receives a server initiated DETACHED message and the channel is Attaching * Remove test about #454 (replaced by RTL13) * Update RTL4f * Update RTL4 for 0.9 (#545) * RTL4i * RTL4h * Update RTL4e * Fix: attach request should be treated as though it has failed and the channel should transition to the SUSPENDED state * Fix: attach after Detaching * Fix: if it fails to detach then move back to ATTACHED * Fix RTL5f * Update RTL6c for 0.9 (#547) * Update RTL6c2 * Update RTL6c4 * RTL6c3 for Detached * Update RTL6c4 for channel Failed * Better code completion * Add ChannelStateChange.event property (#561) * Update RTN4 for 0.9 (#560) * Update RTN4e * Update RTN4f * Add ConnectionStateChange.event property * Update RTL5 for 0.9 (#546) * RTL5j * Update RTL5a * RTL5i * Fix: if channel is SUSPENDED then the detach request transitions immediately to DETACHED state * Fix: if channel is ATTACHING then do the detach operation after the completion of the attaching * Remove testSkipsFromAttachingToDetaching * Fix: presence sync can fail * Fix RTP11b * Fix RTP9e * RTP5: pending * Fix RTL6c3 * Fix: should check channel state when a queued message is processed * TR4i (#562) * ARTProtocolMessageFlag enum: use NS_OPTIONS to define a bitmask * TR4i * Fix: should indicate that the channel has been resumed or not (RESUMED flag) * Swift performance: speed up code completion (#569) * Speed up code completion - This problem is fixed on Xcode 8 but since we are still using Xcode 7 I decided to change that code. * Test suite: addMembersSequentiallyToChannel should return the realtime client * Test suite: set AsyncDefaults.Timeout with default value * Remove pending tests (#542) * Fix: RTP6c pending test * RSL1g4: remove pending * RTP2: remove pending * Fix RTC8 * Update RTP3 (#566) * Update RTP8d (#572) * RTP17 (#571) * PresenceMap: list of internal members * RTP17 * Add ARTPresenceActionToStr method * RTP17: pending * Update RTP2 for 0.9 (#563) * RTP2: remove pending * RTP2a * RTP2b * Test suite: set Nimble.AsyncDefaults.Timeout - increase the default timeout value from all async expectations * RTP2c * RTP2d * RTP2e * RTP2f * RTP2g * Test suite: ARTPresenceMessage convenience initializer * Test suite: NSDate custom operators - convenience for use of `dateByAddingTimeInterval` * Fix RTP2b1 * Fix RTP2b2 * PresenceMap: compare for newness * Fix RTP2 * Remove warnings * RTP18 (#567) * Test suite: ARTPresenceMessage convenience initializer * RTP18 * RTP18a * RTP18b * RTP18c * Update RSA9h for 0.9 (#574) * Auth: optional arguments * Update RSA9h * Update RSA8e for 0.9 (#573) * Update RSA8e * Auth: optional arguments * Comments * Update RSA10g for 0.9 (#575) * Auth: optional arguments * Update RSA10g * Auth: subsequent authorizations with stored values * Update RTP5 for 0.9 (#570) * Update RTP5 * Update RTP5a * Update RTP5b * RTP5c * Test suite: replaceAcksWithNacks - better code completion * RTP5f * PresenceMap: existing members before Sync * Fix: presence get members should not wait for sync if sync is not in progress * PresenceMap: reenter local member * Test suite: replacing acks with nacks even with Presence action * Fix: should queue messages before the attach operation * Test suite: ARTPresenceAction description * Better debug info * Fix: should continue incrementing msgSerial serially if the connection resumes successfully * Remove warnings * Test suite: timings * RTP19 (#568) * RTP19 * RTP19a * Fix: if channel resumed successfully then do not start sync * Enhance RTP5: presence get should not have Absent members * Fix race condition * Fix #583: update httpRequestTimeout and httpMaxRetryDuration * EventEmitter: use @synchronized because NSMutableArray are not thread safe * Thread safety (#586) * New EventEmitter (using NSNotificationCenter) - In most ways that matter NSNotificationCenter is thread safe. You can add/remove observers from any thread and you can post notifications from any thread. * Events for the EventEmitter * Fix: should cancel timers when connection times out * Fix: new state change can occur before receiving publishing acknowledgement * Test suite: async forced transitions * Test suite: ack order * Test suite: stop when there's no internet * Fix: instance objects released to soon * Performed a static analysis from Xcode * fixup! Test suite: ack order * Memory leak: call session invalidate to dispose of its strong reference to the delegate * fixup! Test suite: ack order * Fix RTN19a: guarantee of a new transport (check transport reference) * Fix: ACK or NACK has not yet been received for a message, the client should consider the delivery of those messages as failed * Enhance RTN14b: better timings * Fix: REST and Realtime, wait for last operation to release the object * fixup! Test suite: ack order * Fix: cancel timers when a connection gets closed * fixup! Test suite: ack order * Test suite: timings * fixup! Enhance RTN14b: better timings * Test suite: close connections * Fix: turn off immediately reachability when close occurs * Fix RTC1d: wait for host is not reachable error * fixup! Test suite: ack order * Travis update * Fix RTN19a

mattheworiordan reviewed Nov 23, 2016

View reviewed changes

mattheworiordan requested changes Nov 25, 2016

View reviewed changes

ricardopereira force-pushed the 0-9-master branch from 0857337 to 5728e01 Compare November 30, 2016 12:03

ricardopereira force-pushed the rtn22 branch from dbcaec8 to 42c0c5a Compare November 30, 2016 13:03

ricardopereira force-pushed the rtn22 branch from 42c0c5a to 0c50dde Compare December 5, 2016 11:03

tcard reviewed Dec 5, 2016

View reviewed changes

ricardopereira force-pushed the rtn22 branch 2 times, most recently from a2495bc to 8861320 Compare December 12, 2016 18:53

mattheworiordan approved these changes Dec 13, 2016

View reviewed changes

ricardopereira added 4 commits December 14, 2016 07:42

RTN22

4e9e624

RTN22a

8f9be83

Fix: realtime transport can be nil

11d0751

Fix: realtime should renew token by transitioning to CONNECTING

36a76b9

ricardopereira force-pushed the rtn22 branch from 25e9b9f to 36a76b9 Compare December 14, 2016 07:42

ricardopereira merged commit 3532a32 into 0-9-master Dec 14, 2016

ricardopereira deleted the rtn22 branch December 14, 2016 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTN22 #537

RTN22 #537

ricardopereira commented Nov 22, 2016

mattheworiordan Nov 23, 2016

mattheworiordan Nov 23, 2016

mattheworiordan Nov 23, 2016

ricardopereira Dec 5, 2016

mattheworiordan left a comment

mattheworiordan Nov 25, 2016

ricardopereira Nov 28, 2016

mattheworiordan commented Dec 2, 2016

tcard Dec 5, 2016

ricardopereira Dec 5, 2016

tcard Dec 5, 2016

ricardopereira Dec 5, 2016

tcard Dec 5, 2016

ricardopereira Dec 5, 2016

tcard Dec 5, 2016

tcard Dec 5, 2016

ricardopereira Dec 5, 2016

mattheworiordan Dec 13, 2016

ricardopereira commented Dec 13, 2016

mattheworiordan left a comment

RTN22 #537

RTN22 #537

Conversation

ricardopereira commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardopereira commented Dec 13, 2016

mattheworiordan left a comment

Choose a reason for hiding this comment