RSC15a as a part of 0.9 #501
Conversation
| expect(resultFallbackHosts).to(equal(expectedFallbackHosts)) | ||
| } | ||
|
|
||
| it("until httpMaxRetryCount has been reached, if an empty array of fallback hosts is provided, then fallback host functionality is disabled, use defaults instead") { |
There was a problem hiding this comment.
I don't think is right. If an empty array or fallback hosts is configured, then according to http://docs.ably.io/client-lib-development-guide/features/#RSC15a, If an empty array of fallback hosts is provided, then fallback host functionality is disabled. It appears that you are instead using the defaults?
There was a problem hiding this comment.
Sorry, I see this is fixed in b0ae3c6
There was a problem hiding this comment.
I see this is fixed in b0ae3c6
Yes, exactly, thanks for feedback.
| expect(resultFallbackHosts).to(equal(expectedFallbackHosts)) | ||
| } | ||
|
|
||
| it("until all fallback hosts have been tried, if an empty array of fallback hosts is provided, then fallback host functionality is disabled, use defaults instead") { |
There was a problem hiding this comment.
Same problem here, it appears you are using the defaults instead of disabling the fallback functionality
tcard
force-pushed
the
RSC15a
branch
3 times, most recently
from
0f4462f to
94426c2
Compare
|
|
||
| it("until httpMaxRetryCount has been reached, if custom fallback hosts are provided in ClientOptions#fallbackHosts, then they will be used instead") { | ||
| let options = ARTClientOptions(key: "xxxx:xxxx") | ||
| options.httpMaxRetryCount = 5 |
There was a problem hiding this comment.
Given you've provided 5, why not set this to 4 for example to ensure that httpMaxRetryCount is working as opposed to the max of hosts available is being used?
|
Please review and merge if everything looks good. |
|
@mattheworiordan could you please find some spare time to take a look at 1cef166 ? |
| @@ -839,6 +839,7 @@ class RestClient: QuickSpec { | |||
| } | |||
|
|
|||
| expect(testHTTPExecutor.requests).to(haveCount(Int(1 + options.httpMaxRetryCount))) | |||
| expect((testHTTPExecutor.requests.count) < (_fallbackHosts.count + 1)).to(beTrue()) | |||
There was a problem hiding this comment.
Why not expect(testHTTPExecutor.requests).to(haveCount(Int(1 + options.httpMaxRetryCount))) though?
There was a problem hiding this comment.
Agree, I'm using local variable _fallbackHosts and its count property.
Assert based on that hard-coded array is useless, because who knows, how big an array can be.
If I use assert based on options.fallbackHosts.count like that:
expect((testHTTPExecutor.requests.count) < (options.fallbackHosts.count + 1)).to(beTrue())
I can check that count of executed requests (which is 5 = 1 to primary host + 4 to fallback hosts, cuz I use httpMaxRetryCount = 4 value) is less than max available count (which is 6 = 1 to primary + 5 to fallback hosts).
So I just wanted to be sure that reduce in retry count from 5 to 4 - works as expected, does it make sense @mattheworiordan ?
There was a problem hiding this comment.
@EvgenyKarkan on that line you're checking the _fallbackHosts, not the options.fallbackHosts. You already verifying the number of requests of the testHTTPExecutor and the _fallbackHosts is a local var of the test suite that we control, so I think that line is useless.
…ed assert for that case.
ricardopereira
force-pushed
the
RSC15a
branch
from
674f32f to
5f75cc8
Compare
|
Rebased and tests are passing. |
Please review and merge if everything looks good.