Update RSC15a to new requirements #466
Conversation
|
Thanks @CezaryKopacz this is very good. Note that we try not to ever add more tests to our old Obj-C test suite and always use the Swift ones, unless just a small change is required and therefore it's quicker than rewriting in Swift. But you didn't know that, so thanks. |
| } | ||
| } | ||
| //check artfallback randomises the order. | ||
| XCTAssertFalse(inOrder); |
There was a problem hiding this comment.
BTW. This test will unfortunately fail every 6 (3x2) times as randomising a set of fallback hosts will be in the original order once every 6 times statistically. So this test is probably not ideal. We could make it statistically unlikely to fail by having a decent number of random hosts (12 would result in failure once every 480 million test runs) or simply run the tests multiple times in a row and only fail if all test runs fail.
There was a problem hiding this comment.
Good point! BTW I can see similar test which will probably fail too. Anyway, what needs to be done yet in order to merge PR?
There was a problem hiding this comment.
Well I wouldn't worry about the other tests. If you could fix this one that would be great.
There was a problem hiding this comment.
Please check my latest commit
There was a problem hiding this comment.
We have already support for doing this clean by overriding the random function. See https://github.com/ably/ably-ios/blob/e049dab0e87eaa1643e21d70ae3a771529e0d9e7/Spec/RealtimeClientConnection.swift#L2362-L2372 which lets us expect a given order: https://github.com/ably/ably-ios/blob/e049dab0e87eaa1643e21d70ae3a771529e0d9e7/Spec/RealtimeClientConnection.swift#L2585 If we get the expected order, we prove that fallback logic is using the ARTFallback_getRandomHostIndex function correctly, and we can just trust that the default value for ARTFallback_getRandomHostIndex works as expected
| @"test2.ably.com", | ||
| @"test3.ably.com", | ||
| @"test4.ably.com", | ||
| @"test5.ably.com"]; |
There was a problem hiding this comment.
5x4x3x2 = 1 in 120 test will fail.
Why not just go for 15 hosts, for example, giving you a 1 in 479001600 change the test will fail?
| @@ -154,7 +154,11 @@ - (void)executeRequest:(NSMutableURLRequest *)request completion:(void (^)(NSHTT | |||
| } | |||
| if (retries < _options.httpMaxRetryCount && [self shouldRetryWithFallback:request response:response error:error]) { | |||
| if (!blockFallbacks && [request.URL.host isEqualToString:(_prioritizedHost ? _prioritizedHost : [ARTDefault restHost])]) { | |||
There was a problem hiding this comment.
RSC15b has changed as well, so this condition needs to incorporate this: "or an array of ClientOptions#fallbackHosts is provided"
There was a problem hiding this comment.
Thanks @tcard, however @CezaryKopacz's scope was only to look at RSC15a for now, so that's a valid comment, but will be addressed in a separate PR.
There was a problem hiding this comment.
Oh OK, sorry, just felt weird as if you're using custom fallback hosts you most probably are also using a custom primary host.
|
This PRs seems to have stalled. @mattheworiordan should I finish it up? |
|
@tcard checking with 10Clouds if they are happy for us to merge as it was a trial task so arguably not our right to use without their permission. I'll have an answer soon. |
|
@mattheworiordan What's the state of this? /cc @tcard BTW, this should be rebased with master. @EvgenyKarkan updated the RSA15b spec. There's already a |
No description provided.