From 5cf3f3093eca1469ebdfa9d3b8b6b143096783cb Mon Sep 17 00:00:00 2001 From: Gareth Reese Date: Fri, 1 Dec 2023 09:09:58 +0000 Subject: [PATCH 1/3] Remove the cache clearing at initialisation, add method to manually clear the cache, and add unit tests to cover the expected cache behaviour. --- .../src/main/java/com/flagsmith/Flagsmith.kt | 4 + .../internal/FlagsmithRetrofitService.kt | 3 - .../com/flagsmith/FeatureFlagCachingTests.kt | 85 +++++++++++++++++++ 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt b/FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt index ac2383d..e4d87f5 100644 --- a/FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt +++ b/FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt @@ -150,6 +150,10 @@ class Flagsmith constructor( retrofit.getIdentityFlagsAndTraits(identity).enqueueWithResult(defaults = null, result = result) .also { lastUsedIdentity = identity } + fun clearCache() { + cache?.evictAll() + } + private fun getFeatureFlag( featureId: String, identity: String?, diff --git a/FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt b/FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt index fd8e88e..cac1404 100644 --- a/FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt +++ b/FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt @@ -96,9 +96,6 @@ interface FlagsmithRetrofitService { .cache(cache) .build() - // Make sure that we start with a clean cache - client.cache?.evictAll() - val retrofit = Retrofit.Builder() .baseUrl(baseUrl) .addConverterFactory(GsonConverterFactory.create()) diff --git a/FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt b/FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt index a53b88c..1cfeec2 100644 --- a/FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt +++ b/FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt @@ -291,4 +291,89 @@ class FeatureFlagCachingTests { Assert.assertNotNull(foundFromServer) Assert.assertEquals(7.0, foundFromServer?.featureStateValue) } + + @Test + fun testGetFeatureFlagsWithNewCachedFlagsmithGetsCachedValue() { + mockServer.mockResponseFor(MockEndpoint.GET_IDENTITIES) + mockServer.mockFailureFor(MockEndpoint.GET_IDENTITIES) + + // First time around we should be successful and cache the response + var foundFromServer: Flag? = null + flagsmithWithCache.clearCache() + flagsmithWithCache.getFeatureFlags(identity = "person") { result -> + Assert.assertTrue(result.isSuccess) + + foundFromServer = + result.getOrThrow().find { flag -> flag.feature.name == "with-value" } + } + + await untilNotNull { foundFromServer } + Assert.assertNotNull(foundFromServer) + Assert.assertEquals(756.0, foundFromServer?.featureStateValue) + + // Now get a new Flagsmith instance with the same cache and expect the cached response to be returned + val newFlagsmithWithCache = Flagsmith( + environmentKey = "", + baseUrl = "http://localhost:${mockServer.localPort}", + enableAnalytics = false, + context = mockApplicationContext, + cacheConfig = FlagsmithCacheConfig(enableCache = true) + ) + + // Now we mock the failure and expect the cached response to be returned from the new flagsmith instance + var foundFromCache: Flag? = null + newFlagsmithWithCache.getFeatureFlags(identity = "person") { result -> + Assert.assertTrue("The request will fail but we should be successful as we fall back on the cache", result.isSuccess) + + foundFromCache = + result.getOrThrow().find { flag -> flag.feature.name == "with-value" } + } + + await untilNotNull { foundFromCache } + Assert.assertNotNull(foundFromCache) + Assert.assertEquals(756.0, foundFromCache?.featureStateValue) + } + + @Test + fun testGetFeatureFlagsWithNewCachedFlagsmithDoesntGetCachedValueWhenWeClearTheCache() { + mockServer.mockResponseFor(MockEndpoint.GET_IDENTITIES) + mockServer.mockFailureFor(MockEndpoint.GET_IDENTITIES) + + // First time around we should be successful and cache the response + var foundFromServer: Flag? = null + flagsmithWithCache.clearCache() + flagsmithWithCache.getFeatureFlags(identity = "person") { result -> + Assert.assertTrue(result.isSuccess) + + foundFromServer = + result.getOrThrow().find { flag -> flag.feature.name == "with-value" } + } + + await untilNotNull { foundFromServer } + Assert.assertNotNull(foundFromServer) + Assert.assertEquals(756.0, foundFromServer?.featureStateValue) + + // Now get a new Flagsmith instance with the same cache and evict the cache straight away + val newFlagsmithWithClearedCache = Flagsmith( + environmentKey = "", + baseUrl = "http://localhost:${mockServer.localPort}", + enableAnalytics = false, + context = mockApplicationContext, + cacheConfig = FlagsmithCacheConfig(enableCache = true) + ) + newFlagsmithWithClearedCache.clearCache() + + // Now we mock the failure and expect the get to fail as we don't have the cache to fall back on + var foundFromCache: Flag? = null + newFlagsmithWithClearedCache.getFeatureFlags(identity = "person") { result -> + Assert.assertFalse("This un-cached response should fail", result.isSuccess) + + foundFromCache = + result.getOrThrow().find { flag -> flag.feature.name == "with-value" } + } + + await untilNotNull { foundFromCache } + Assert.assertNotNull(foundFromCache) + Assert.assertEquals(756.0, foundFromCache?.featureStateValue) + } } \ No newline at end of file From 405f495007efb67c9cdda89a4ebf6fdeaa8dc9c6 Mon Sep 17 00:00:00 2001 From: Gareth Reese Date: Fri, 1 Dec 2023 09:25:33 +0000 Subject: [PATCH 2/3] Improvements to the negative caching test --- .../java/com/flagsmith/FeatureFlagCachingTests.kt | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt b/FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt index 1cfeec2..3c3ed7b 100644 --- a/FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt +++ b/FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt @@ -13,6 +13,7 @@ import com.flagsmith.mockResponses.mockResponseFor import org.awaitility.Awaitility import org.awaitility.kotlin.await import org.awaitility.kotlin.untilNotNull +import org.awaitility.kotlin.untilTrue import org.junit.After import org.junit.Assert import org.junit.Before @@ -25,6 +26,7 @@ import org.mockito.MockitoAnnotations import org.mockserver.integration.ClientAndServer import java.io.File import java.time.Duration +import java.util.concurrent.atomic.AtomicBoolean class FeatureFlagCachingTests { @@ -365,15 +367,16 @@ class FeatureFlagCachingTests { // Now we mock the failure and expect the get to fail as we don't have the cache to fall back on var foundFromCache: Flag? = null + val hasFinishedGetRequest = AtomicBoolean(false) newFlagsmithWithClearedCache.getFeatureFlags(identity = "person") { result -> Assert.assertFalse("This un-cached response should fail", result.isSuccess) foundFromCache = - result.getOrThrow().find { flag -> flag.feature.name == "with-value" } + result.getOrNull()?.find { flag -> flag.feature.name == "with-value" } + hasFinishedGetRequest.set(true) } - await untilNotNull { foundFromCache } - Assert.assertNotNull(foundFromCache) - Assert.assertEquals(756.0, foundFromCache?.featureStateValue) + await untilTrue(hasFinishedGetRequest) + Assert.assertNull("Shouldn't get any data back as we don't have a cache", foundFromCache) } } \ No newline at end of file From 166d04b175975783e71e3c76aba83ca50912b6f5 Mon Sep 17 00:00:00 2001 From: Gareth Reese Date: Mon, 4 Dec 2023 14:31:53 +0000 Subject: [PATCH 3/3] Increase some of the timeouts to ensure that the tests run correctly --- .../java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt b/FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt index 8fa930f..0a1d4fd 100644 --- a/FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt +++ b/FlagsmithClient/src/test/java/com/flagsmith/RealTimeUpdatesIntegrationTests.kt @@ -160,10 +160,10 @@ class RealTimeUpdatesIntegrationTests : FlagsmithEventTimeTracker { } // Update after 35 secs to ensure we've done a reconnect, should be done in 60 seconds or fail - // Though 60 seconds sounds like a long time it can take a while for the infrastructure to let + // Though 120 seconds sounds like a long time it can take a while for the infrastructure to let // us know that the value has changed. The test still finishes as soon as the value is updated // so will be as quick as the infrastructure allows. - @Test(timeout = 60_000) + @Test(timeout = 120_000) fun testGettingFlagsWithRealtimeUpdatesAfterPuttingNewValueAndReconnect() = runBlocking { val expectedNewValue = "new-value-after-reconnect" // Get the current value @@ -197,7 +197,7 @@ class RealTimeUpdatesIntegrationTests : FlagsmithEventTimeTracker { Assert.assertEquals(expectedNewValue, newUpdatedFeatureValueFromApi) } - @Test(timeout = 70_000) + @Test(timeout = 120_000) fun testGettingFlagsWithRealtimeUpdatesViaFlagUpdateFlow() = runBlocking { // Get the current value val currentFlagValueString =