Skip to content
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

ExPlat: Handle local Experiment registration #14559

Merged
merged 14 commits into from
May 13, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
StatsModule.class,
TrackerModule.class,
SuggestionSourceModule.class,
ExperimentModule.class,
// Login flow library
LoginAnalyticsModule.class,
LoginFragmentModule.class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@
ThreadModule.class,
TrackerModule.class,
SuggestionSourceModule.class,
ExperimentModule.class,
// Login flow library
LoginAnalyticsModule.class,
LoginFragmentModule.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.wordpress.android.modules

import dagger.Module
import dagger.multibindings.Multibinds
import org.wordpress.android.util.experiments.Experiment

@Module
interface ExperimentModule {
@Multibinds fun experiments(): Set<Experiment>

// Copy and paste the line below to add a new experiment.
// @Binds @IntoSet fun exampleExperiment(experiment: ExampleExperiment): Experiment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it viable to introduce some kind of a lint rule that would fail if there is an experiment that is not registered in the module? Maybe that's unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably viable, although I have no experience with custom lint rules so I can't say for sure.

That said, if that's something we want to do, we should probably do it in a different PR (specially since I guess it would need to be submitted to WordPress-Lint-Android?).

}
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package org.wordpress.android.util.experiments

import dagger.Lazy
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import org.wordpress.android.BuildConfig
import org.wordpress.android.fluxc.model.experiments.Assignments
import org.wordpress.android.fluxc.model.experiments.Variation
import org.wordpress.android.fluxc.model.experiments.Variation.Control
import org.wordpress.android.fluxc.store.ExperimentStore
import org.wordpress.android.fluxc.store.ExperimentStore.Platform
import org.wordpress.android.fluxc.utils.AppLogWrapper
Expand All @@ -19,19 +22,21 @@ import javax.inject.Singleton
@Singleton
class ExPlat
@Inject constructor(
private val experiments: Lazy<Set<Experiment>>,
private val experimentStore: ExperimentStore,
private val appLog: AppLogWrapper,
@Named(APPLICATION_SCOPE) private val coroutineScope: CoroutineScope
) {
private val platform = Platform.WORDPRESS_ANDROID
private val activeVariations = mutableMapOf<String, Variation>()
private val experimentNames by lazy { experiments.get().map { it.name } }

fun refreshIfNeeded() {
getAssignments(refreshStrategy = IF_STALE)
refresh(refreshStrategy = IF_STALE)
}

fun forceRefresh() {
getAssignments(refreshStrategy = ALWAYS)
refresh(refreshStrategy = ALWAYS)
}

fun clear() {
Expand All @@ -48,11 +53,27 @@ class ExPlat
* is returned from the cached [Assignments] and then set as active. If the cached [Assignments]
* is stale and [shouldRefreshIfStale] is `true`, then new [Assignments] are fetched and their
* variations are going to be returned by this method on the next session.
*
* If the provided [Experiment] was not included in [ExPlat.start], then [Control] is returned.
* If [BuildConfig.DEBUG] is `true`, an [IllegalArgumentException] is thrown instead.
*/
internal fun getVariation(experiment: Experiment, shouldRefreshIfStale: Boolean) =
activeVariations.getOrPut(experiment.name) {
getAssignments(if (shouldRefreshIfStale) IF_STALE else NEVER).getVariationForExperiment(experiment.name)
}
internal fun getVariation(experiment: Experiment, shouldRefreshIfStale: Boolean): Variation {
if (!experimentNames.contains(experiment.name)) {
val message = "ExPlat: experiment not found: \"${experiment.name}\"! " +
"Make sure to include it in the set provided via constructor."
appLog.e(T.API, message)
if (BuildConfig.DEBUG) throw IllegalArgumentException(message) else return Control
}
return activeVariations.getOrPut(experiment.name) {
getAssignments(if (shouldRefreshIfStale) IF_STALE else NEVER).getVariationForExperiment(experiment.name)
}
}

private fun refresh(refreshStrategy: RefreshStrategy) {
if (experimentNames.isNotEmpty()) {
getAssignments(refreshStrategy)
}
}

private fun getAssignments(refreshStrategy: RefreshStrategy): Assignments {
val cachedAssignments = experimentStore.getCachedAssignments() ?: Assignments()
Expand All @@ -62,7 +83,7 @@ class ExPlat
return cachedAssignments
}

private suspend fun fetchAssignments() = experimentStore.fetchAssignments(platform, emptyList()).also {
private suspend fun fetchAssignments() = experimentStore.fetchAssignments(platform, experimentNames).also {
if (it.isError) {
appLog.d(T.API, "ExPlat: fetching assignments failed with result: ${it.error}")
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package org.wordpress.android.util.experiments

import javax.inject.Inject

class ExampleExperiment
@Inject constructor(
exPlat: ExPlat
) : Experiment(
"example_experiment",
exPlat
)
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import com.nhaarman.mockitokotlin2.anyOrNull
import com.nhaarman.mockitokotlin2.never
import com.nhaarman.mockitokotlin2.times
import com.nhaarman.mockitokotlin2.verify
import com.nhaarman.mockitokotlin2.verifyZeroInteractions
import com.nhaarman.mockitokotlin2.whenever
import dagger.Lazy
import org.assertj.core.api.Assertions.assertThat
import org.junit.Before
import org.junit.Test
Expand All @@ -26,15 +28,18 @@ import java.util.Date

@RunWith(MockitoJUnitRunner::class)
class ExPlatTest : BaseUnitTest() {
@Mock lateinit var experiments: Lazy<Set<Experiment>>
@Mock lateinit var experimentStore: ExperimentStore
@Mock lateinit var appLog: AppLogWrapper
private lateinit var exPlat: ExPlat
private lateinit var dummyExperiment: Experiment

@Before
fun setUp() {
exPlat = ExPlat(experimentStore, appLog, testScope())
exPlat = ExPlat(experiments, experimentStore, appLog, testScope())
dummyExperiment = object : Experiment("dummy", exPlat) {}

setupExperiments(setOf(dummyExperiment))
}

@Test
Expand Down Expand Up @@ -145,6 +150,50 @@ class ExPlatTest : BaseUnitTest() {
assertThat(secondVariation).isEqualTo(controlVariation)
}

@Test
fun `forceRefresh fetches assignments if experiments is not empty`() = test {
setupExperiments(setOf(dummyExperiment))

exPlat.forceRefresh()

verify(experimentStore, times(1)).fetchAssignments(any(), any(), anyOrNull())
}

@Test
fun `forceRefresh does not interact with store if experiments is empty`() = test {
setupExperiments(emptySet())

exPlat.forceRefresh()

verifyZeroInteractions(experimentStore)
}

@Test
fun `refreshIfNeeded does not interact with store if experiments is empty`() = test {
setupExperiments(emptySet())

exPlat.refreshIfNeeded()

verifyZeroInteractions(experimentStore)
}

@Test
fun `getVariation does not interact with store if experiments is empty`() = test {
setupExperiments(emptySet())

try {
exPlat.getVariation(dummyExperiment, false)
} catch (e: IllegalArgumentException) {
// Do nothing.
} finally {
verifyZeroInteractions(experimentStore)
}
}

private fun setupExperiments(experiments: Set<Experiment>) {
whenever(this.experiments.get()).thenReturn(experiments)
}

private suspend fun setupAssignments(cachedAssignments: Assignments?, fetchedAssignments: Assignments) {
whenever(experimentStore.getCachedAssignments()).thenReturn(cachedAssignments)
whenever(experimentStore.fetchAssignments(any(), any(), anyOrNull()))
Expand Down