-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/real time flags #28
Conversation
* Compiles and test, need to add some tests then get it into a sample app * Should be testing but not getting the errors through Fuse so can't run into the logic * Probably gone as far as I can with 2.x fuel, let's try the 3.x * Move to Retrofit - seems to be going well so far, test runs * Tidying up, setTrait test not working * All the tests are passing so will finish the retrofit migration * Updated some of the logic and added setTraits * Checkpoint commit before trying generic converter * Generics working fine * All passing for flags and such with the new generic caching * Mostly swapped to Retrofit, now need to do the analytics * Analytics now over to retrofit * Add caching for the getFlags endpoint * Get rid of the last of Fuel * Another clear-out and all working fine on the tests * Now using Retrofit cache, remove the old stuff * Now just using HTTP caching * Delete the old caching logic * Finishing off, should be done for defaults and caching * Remove unneeded todo * Remove some more code * Still just playing around with it * Move cache configuration to its own data class * Tidy up the cache config and the tests * Update the comments * Now covers the caching tests * Tidy up some more of the tests * Some more tidying up * Default to caching disabled * Last few PR comments * Split the read and write timeout for HTTP * Initial basic implementation, let's try to get things hooked up to the server * Seems to be generally working * Checkpoint commit, seems to be generally working now just need to get the flags on update * Checkpoint commit before making the changes OK'd by Matthew to move the update clock into the event service * Ensure that the event source just reconnects if it loses the connection * Events and timers now all hooked-up and working in the manual integration test * Got the integration test working * Tidy everything up and move sensitive data to environment variables * Add a new test to cover the event stream going through a reconnect cycle * Added test for the live stream of flags, tidied up the imports and various thing, changed the logic a bit for when we need to do updates from events
…egrationTests.kt Co-authored-by: Matthew Elwell <mjelwell89@gmail.com>
…re obvious what's going on if we don't configure properly
…/foresightmobile/flagsmith-kotlin-android-client into feature/real-time-flags-squashed
* Feature/real time flags (#5) * Compiles and test, need to add some tests then get it into a sample app * Should be testing but not getting the errors through Fuse so can't run into the logic * Probably gone as far as I can with 2.x fuel, let's try the 3.x * Move to Retrofit - seems to be going well so far, test runs * Tidying up, setTrait test not working * All the tests are passing so will finish the retrofit migration * Updated some of the logic and added setTraits * Checkpoint commit before trying generic converter * Generics working fine * All passing for flags and such with the new generic caching * Mostly swapped to Retrofit, now need to do the analytics * Analytics now over to retrofit * Add caching for the getFlags endpoint * Get rid of the last of Fuel * Another clear-out and all working fine on the tests * Now using Retrofit cache, remove the old stuff * Now just using HTTP caching * Delete the old caching logic * Finishing off, should be done for defaults and caching * Remove unneeded todo * Remove some more code * Still just playing around with it * Move cache configuration to its own data class * Tidy up the cache config and the tests * Update the comments * Now covers the caching tests * Tidy up some more of the tests * Some more tidying up * Default to caching disabled * Last few PR comments * Split the read and write timeout for HTTP * Initial basic implementation, let's try to get things hooked up to the server * Seems to be generally working * Checkpoint commit, seems to be generally working now just need to get the flags on update * Checkpoint commit before making the changes OK'd by Matthew to move the update clock into the event service * Ensure that the event source just reconnects if it loses the connection * Events and timers now all hooked-up and working in the manual integration test * Got the integration test working * Tidy everything up and move sensitive data to environment variables * Add a new test to cover the event stream going through a reconnect cycle * Added test for the live stream of flags, tidied up the imports and various thing, changed the logic a bit for when we need to do updates from events * Update FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt Co-authored-by: Matthew Elwell <mjelwell89@gmail.com> * Update environment variables in the github actions * Add some error checking on the environment variables so it's a bit more obvious what's going on if we don't configure properly * Noddy change to get the tests to run again * Try printing out unsuccessful responses in the integration tests * Push more more non-empty checks --------- Co-authored-by: Matthew Elwell <mjelwell89@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gazreese I've been through everything apart from the tests here (since they may yet change due to current failures).
One thing I've noticed however, is that we only have the integration tests. Are we planning to also add some unit tests for some of the more complex logic?
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithAnalytics.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithEventService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Show resolved
Hide resolved
…real-time-flags-squashed # Conflicts: # FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt
…pdates in the SDK so they're always passive
… make the check loops more CPU friendly
So the recent updates do a few things. I've simplified the way that the events get pushed to the live event stream on update, making this completely passive based on a successful getFlags() call rather than doing anything with the flags directly in the realtime updates logic. This should be easier to maintain and doesn't repeat as much code. https://developer.android.com/kotlin/flow/test#stateflows The testing StateFlows docs suggest polling the .value within unit tests for testing. I had a more reactive style initially, but due to some threading / coroutine issues this could well have been the cause of the issues on GH actions. I also noticed 100% CPU on the Java process while testing. I added some wait() statements in the loops (which were tighter since polling the value) and that seems to have been the final push that got them running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code to go with the comments coming shortly
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithAnalytics.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithEventService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithAnalytics.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Show resolved
Hide resolved
@matthewelwell re:
We currently use https://www.mock-server.com/ to mock the HTTP responses and unit test most of the functionality. Unfortunately neither MockServer or anything else I can find will mock SSE for us, so we can't do much useful testing without an integration test. Also, in the absence of an example app in the repo, I was much more confident in the implementation having done the integration tests and talking to the real servers. The only real logic in our SSE implementation is to reconnect on errors and things like that. We're reliant on the SSE library for much of the actual implementation, so the sensible approach is to either integration test or mock the HTTP. |
…ortions of the API into the test folder
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithEventService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Show resolved
Hide resolved
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Show resolved
Hide resolved
FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt
Outdated
Show resolved
Hide resolved
FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt
Show resolved
Hide resolved
FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt
Show resolved
Hide resolved
FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt
Show resolved
Hide resolved
…timeouts as it was taking a while for the realtime updates to come though
…n the reasoning for some of the timeouts and delays
Description
This PR adds the real time flags feature as described in #18 and Flagsmith/flagsmith#1498
Fixes #18
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Because the real-time updates are dependent upon the the operation of the SSE functionality I found it best to write end-to-end integrations test for this feature. They test the main 'update' scenario, as well as ensuring that the SSE connection reconnects. It
Test Configuration:
As these are integration tests they need some environment variables to connect to an account, feature and value. These are at the top of the integration test and follow the format seen in the existing documentation:
To skip the integration tests you can run the
gradle
command as shown in the build.gradle:Checklist: