RTN17с as a part of 0.9

[EvgenyKarkan]

Please review and merge if everything looks good.

[mattheworiordan]

I was not aware that RTN17c changed. Are you sure there are not already existing tests for this?

[EvgenyKarkan]

Yes, RTN17c test exists, but it tests default fallback hosts.

0.9 has a difference from 0.8:

See RSC15a for details on how custom fallback hosts are specified and used

Ok, RSC15a diffs are:

If an array of custom fallback hosts are provided in ClientOptions#fallbackHosts, then they will be used instead.
If an empty array of fallback hosts is provided, then fallback host functionality is disabled

That's why I added test for custom fallback hosts.
Am I wrong here @mattheworiordan?

[mattheworiordan]

Ok, fine, but it does feel like we're perhaps testing all the permutations when in reality testing if custom fallback hosts are used does not need to be tested with every other permutation of ClientOptions because we should be reasonably confident that the logic to inject custom fallback hosts is in one place and thus applies globally.

[EvgenyKarkan]

@mattheworiordan please let me try to describe what I think on this...

Current PR is based on RTN17b where I've added an ability for ARTRealtime object to use custom fallback hosts, also in RTN17b PR I've added test to cover or an array of ClientOptions#fallbackHosts is provided requirement.

Well, here I'm trying to cover 2 cases with my tests:

• retry custom fallback hosts in random order after checkin if an internet connection is available (my test based on existing default fallback hosts test),
• check if fallback hosts feature disabled if an empty array is provided, I did such test for ARTRest, but because we are on ARTRealtime page now, so I decided to add it for ARTRealtime too.

So in my understanding this PR has at least 2 reasons to live, but I could be wrong.

I guess you want me to combine current logic and tests with RTN17b PR and close this PR?
Could you give me a hint in which way should I move forward here please?

[mattheworiordan]

retry custom fallback hosts in random order after checkin if an internet connection is available (my test based on existing default fallback hosts test),

Fine, but this seems overkill when really we should know that setting the fallback host options simply updated the defaults used by the client library and does not result in an entirely new codepath. Whether the default fallback hosts or custom fallback hosts are used, it should not affect anything other than which hosts are used.

check if fallback hosts feature disabled if an empty array is provided, I did such test for ARTRest, but because we are on ARTRealtime page now, so I decided to add it for ARTRealtime too.

Ok, fine, that makes sense, a quick test for that is good.

So in my understanding this PR has at least 2 reasons to live, but I could be wrong.

Perhaps, but I would argue the first point is overkill. Leave it and merge if existing already though, no need to do more work if it adds test coverage.

[tcard]

@EvgenyKarkan I've changed the base of the pull request to master. Please do that going forward.

[ricardopereira]

Rebased and tests are passing.