Experimentation: replace FluxC with internal components#239
Experimentation: replace FluxC with internal components#239wzieba merged 16 commits intoexperimentation_refactorfrom
Conversation
To make it more readable
We don't need milliseconds precision here, so it's better to have the same unit for `timeToLive` and `fetchedAt`
`experimentStore` is no longer used
This makes maintenance of the SDK easier, and it's not a big setback for clients, as the value for `platform` has to be only checked once.
Even though the ExPlat doesn't work correctly (we don't ever save data in cache) the `ExPlatTest` suite is passing, just because everything here is mocked.
Also, makes the ignored test pass
| val cachedAssignments: Assignments = | ||
| runBlocking { repository.getCached() } ?: Assignments(emptyMap(), 0, 0) |
There was a problem hiding this comment.
Re: runBlocking
We run file-based operation on the caller thread (risking it's a main thread). It was happening before with experimentStore#getCachedAssignments, but now it's just more visible.
I plan to address this in a future PR.
There was a problem hiding this comment.
I plan to address this in a future PR.
Thanks for taking a note on that and trying to make this more "thread" robust! 🙏
| } | ||
|
|
||
| @Test | ||
| fun `force refresh is successful when cache is fresh `() = runBlockingTest { |
There was a problem hiding this comment.
I identify some issues with this tests suite. It's tightly coupled with implementation details of ExPlat and favors checks on method execution (all checks are verify) rather than actual outcomes.
For this reason, it doesn't really test things. E.g. the whole test suite was passing, even though I forgot to add logic for saving cache. See: 82aa328 (passing) but it has no saving logic.
The issue is that, e.g. we have "forceRefresh fetches assignments if cache is fresh", which checks if a fetch method was executed on a mock. We don't have verifications that assignments has been indeed really fetched. The, added by me in this PR, test "force refresh is successful when cache is fresh" is also not ideal: it checks if we called method on cache, but not that cache has been saved.
For all of these reasons I plan to work on improving this test suite, but I want to do this in a separate PR to not shadow changes here.
ParaskP7
left a comment
ParaskP7
left a comment
There was a problem hiding this comment.
👋 @wzieba !
I have reviewed this PR as per the instructions, everything LGTM as expected, great job on finally removing FluxC as a dependency from Tracks, including removing Utils too! 🎉 x 🎉 ^ 🎉
I have left one questions (❓), a couple of suggestions (💡) and some minor (🔍) comments for you to consider. I am NOT going to approve this PR for now, nor block it, just comment on it. I do that because I want to first review all the other chained PRs first. But, if that breaks your planning, let me know and we can move forward accordingly.
...entation/src/main/java/com/automattic/android/experimentation/remote/AssignmentsDtoMapper.kt
Show resolved
Hide resolved
...ion/src/main/java/com/automattic/android/experimentation/repository/AssignmentsRepository.kt
Show resolved
Hide resolved
...ion/src/main/java/com/automattic/android/experimentation/repository/AssignmentsRepository.kt
Outdated
Show resolved
Hide resolved
experimentation/src/main/java/com/automattic/android/experimentation/ExPlat.kt
Outdated
Show resolved
Hide resolved
experimentation/src/main/java/com/automattic/android/experimentation/ExPlat.kt
Outdated
Show resolved
Hide resolved
| import org.mockito.kotlin.verify | ||
| import org.mockito.kotlin.verifyNoInteractions | ||
| import org.mockito.kotlin.whenever | ||
| import org.wordpress.android.fluxc.model.experiments.Assignments |
There was a problem hiding this comment.
Suggestion (💡): Consider replacing runBlockingTest with runTest everywhere.
There was a problem hiding this comment.
Good suggestion! I applied it in #242 . I'll skip doing this here, to not introduce merge conflicts.
There was a problem hiding this comment.
Awesome, thanks, will take a look at it there then. 👍
experimentation/src/main/java/com/automattic/android/experimentation/ExPlat.kt
Show resolved
Hide resolved
2 participants
Description
This PR replaces usage of
ExperimentsStorefrom FluxC, with internal implementation ofAssignmentsRepositorythat fetches data from remote, saves it to cache and offer it later.I aimed for a minimal set of changes here, to not shadow that the logic remains unchanged here.
How to test
Unit tests are passing after the change. But as they're heavily mocked, we can't really be sure that this caught all possible regressions: #239 (comment)
Still, this is some safety net. To test this manually, please see #241