Improve threading and test suite#242
Merged
wzieba merged 23 commits intoexperimentation_refactorfrom Aug 30, 2024
Merged
Conversation
In order to remove `runBlocking` from `ExPlat`, we have to have a way to get value from cache in a way, that doesn't require sync file-operation. That's why I introduce `latest` field, which keeps the latest cache value in memory. Providing dispatchers is considered a good practice and might come handy during tests
Now, that we have few new/changed tests, I extract some similarities to make tests more readable
…instead of mock invocation
…nts, even if cache is fresh`
This is a confusing API, because, even if user requests a refresh, we don't return fresh data immediately, but only queue this to fetch. This is not something I'd expect as the SDK consumer. Also, we already have specialized refresh methods. I don't think we need more. I plan to remove `shouldRefreshIfStale` in another commit.
I've added some comments explaining a story of this test, as I found it a little confusing when I started working on this.
…don't think their necessary (nothing bad will happen if the list will be empty, and the check itself is trivial), and they bring maintenance overhead at the same time.
…periment I'm not sure if it's good idea to test the debug code, but at the same time this test is tiny, so maybe let's keep it
Personal preference, I'm not a fan of "dummy"
Fixing typo
ParaskP7
reviewed
Aug 29, 2024
experimentation/src/main/java/com/automattic/android/experimentation/ExPlat.kt
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
Show resolved
Hide resolved
experimentation/src/main/java/com/automattic/android/experimentation/ExPlat.kt
Outdated
Show resolved
Hide resolved
...tion/src/test/java/com/automattic/android/experimentation/remote/ExperimentRestClientTest.kt
Show resolved
Hide resolved
experimentation/src/test/java/com/automattic/android/experimentation/ExPlatTest.kt
Show resolved
Hide resolved
experimentation/src/test/java/com/automattic/android/experimentation/ExPlatTest.kt
Show resolved
Hide resolved
experimentation/src/test/java/com/automattic/android/experimentation/ExPlatTest.kt
Show resolved
Hide resolved
| setupAssignments(cachedAssignments = treatmentAssignments, fetchedAssignments = treatmentAssignments) | ||
|
|
||
| val secondVariation = exPlat.getVariation(dummyExperiment, shouldRefreshIfStale = false) | ||
| assertThat(secondVariation).isEqualTo(controlVariation) |
Contributor
There was a problem hiding this comment.
Suggestion (💡): I usually split such code with an extra /* HELPER */ grouping comment, which means, see helper functions below.
… not found in cache It's more reasonable to return `null` if there are no assignments, rather than a fake assignment with made up values.
Member
Author
|
@ParaskP7 I think this PR is ready to re-review. Re tests we discussed in #240 (comment) - I think I'll add it rather in #243 as there are some changes to tests there, so I don't want to work unnecessarily addressing conflicts |
Contributor
|
Reviewed and everything looks good to me, feel free to merge! ✅ PS: Just one note from me, maybe we should consider testing the publicly available |
Member
Author
|
Sorry, my bad. Actually |
Contributor
Ah, right, true! 💯 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR aims to:
runBlockingcall ingetAssignmentsmethodTo test
This PR introduces a robust test suite. Still, you can perform some smoke tests if you'd like following instructions from #241