-
Notifications
You must be signed in to change notification settings - Fork 59
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
Caching questionnaires and their SM #3461
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3461 +/- ##
=========================================
+ Coverage 27.1% 27.3% +0.2%
- Complexity 775 782 +7
=========================================
Files 279 281 +2
Lines 14563 14551 -12
Branches 2608 2602 -6
=========================================
+ Hits 3957 3985 +28
+ Misses 10065 10023 -42
- Partials 541 543 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@aurangzaibumer could we add a unit test for the ViewModel itself? You can mock anything you don't need/already tested in the Cache test if need be. |
android/quest/src/main/java/org/smartregister/fhircore/quest/ui/questionnaire/ContentCache.kt
Outdated
Show resolved
Hide resolved
...st/src/main/java/org/smartregister/fhircore/quest/ui/questionnaire/QuestionnaireViewModel.kt
Outdated
Show resolved
Hide resolved
android/engine/src/main/java/org/smartregister/fhircore/engine/task/FhirCarePlanGenerator.kt
Outdated
Show resolved
Hide resolved
ContentCache.getResource(ResourceType.Questionnaire.name + "/" + questionnaireConfig.id), | ||
) | ||
Assert.assertNotNull(questionnaire) | ||
Assert.assertEquals(questionnaireConfig.id, questionnaire?.id?.extractLogicalIdUuid()) |
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.
Lets add a verify assertion here to verify that the
defaultRepository.loadResource<Questionnaire>(questionnaireConfig.id)
method was not called (i.e. was called zero times)
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.
And we can probably split this to 2 unit tests to get better coverage for the changes in this class.
- Test case 1 to test for when the questionnaire is Found in the cache
- Test case 2 to test for when the questionnaire is Not Found in the cache
So basically use the:
val questionnaire =
questionnaireViewModel.retrieveQuestionnaire(
questionnaireConfig = questionnaireConfig,
)
method as the focus of the test case and leave out the ContentCache.save invocations since those are already tested in the ContentCacheTest
class
structureMapUrl?.substringAfterLast("/")?.let { smID -> | ||
ContentCache.getResource(ResourceType.StructureMap.name + "/" + smID)?.let { | ||
it as StructureMap | ||
} | ||
?: run { | ||
defaultRepository.loadResource<StructureMap>(smID)?.also { | ||
ContentCache.saveResource(smID, it) |
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.
We can add some test cases for the Structure Map caching as well
...st/src/main/java/org/smartregister/fhircore/quest/ui/questionnaire/QuestionnaireViewModel.kt
Outdated
Show resolved
Hide resolved
...st/src/main/java/org/smartregister/fhircore/quest/ui/questionnaire/QuestionnaireViewModel.kt
Outdated
Show resolved
Hide resolved
android/engine/src/main/java/org/smartregister/fhircore/engine/datastore/ContentCache.kt
Outdated
Show resolved
Hide resolved
private val maxMemory: Int = (Runtime.getRuntime().maxMemory() / 1024).toInt() | ||
private val cacheSize: Int = maxMemory / 8 | ||
private val cache = LruCache<String, Resource>(cacheSize) |
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.
What's the strategy used when this cache is full?
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.
Least recently used
8172e1e
to
b40f86e
Compare
8feccd3
to
e632103
Compare
IMPORTANT: Where possible all PRs must be linked to a Github issue
Fixes [link to issue]
Engineer Checklist
strings.xml
file./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the project's style guideCode Reviewer Checklist
strings.xml
file