Implemented RSC15f and RTN17e and improved fallback handling #385
Conversation
There was a problem hiding this comment.
Coming in as cold to this codebase as I am it's near impossible to provide more constructive input on these changes, in particular the refactor, without spending much more time than I have available. A case in point is the RealtimeWorkflow class which is barely documented in respect of interface and, thus, daunting to approach for those (such as myself) who have not been immersed in this codebase yet.
I've submitted a few comments by way of quick / obvious wins, but otherwise I think we'll just have to ensure these changes have the right impact when you get to creating further tests (hopefully next week).
This commit fixes two long standing annoyances 1. Centralise the internet check to the Workflow. Now the skip internet check is done when we process the disconnected state. 2. Renamed OnAttachToContext in the State classes. For ages it hasn't done what's on the tin. All it really does now is start the timer if necessary.
This refactor includes both the realtime and rest clients. Realtime - I moved the connecting error logic to the Realtime workflow and created a special command to handle the different between receiving a disconnected and an error message from the server while connecting. Rest: The logic now follows the spec so we retry all possible options and on every request and save a preferred host when we succeed
Organised the workflow tests and the only addition is RTN14g which was moved from ConnectingStateSpecs. Everything else is the same here.
No new tests here just a tweak for WhenTransportFails_ShouldTransitionToDisconnectedAndEmitErrorWithRetry
This commit updates the logic for existing tests and adds an extra RTN17b and RTN17f to test the logic of the rest library when Realtime is connected to a fallback host.
marto83
force-pushed
the
feature/fallback-handling
branch
from
55e3cad to
1d4ea52
Compare
Fair point. I've added a summary comment to Realtime Workflow. All checks pass so let me know if you are happy with the fixes and I'll merge this into develop as well. |
|
Thanks for adding those comments. That really helps. If you're still happy then please go ahead and get this merged, please. |
|
Morning @QuintinWillison. I am happy to get this merged. I changed the base branch to |
To help you review this pull request you can look at it in 2 parts