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

Write Unit Tests on Related Entity Location sync strategy enchancements #3144

Conversation

brandy-kay
Copy link
Contributor

@brandy-kay brandy-kay commented Mar 15, 2024

…urce identifierIMPORTANT: Where possible all PRs must be linked to a Github issue

Fixes #3105

Engineer Checklist

  • I have written Unit tests for any new feature(s) and edge cases for bug fixes
  • I have added any strings visible on UI components to the strings.xml file
  • I have updated the CHANGELOG.md file for any notable changes to the codebase
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the project's style guide
  • I have built and run the FHIRCore app to verify my change fixes the issue and/or does not break the app
  • I have checked that this PR does NOT introduce breaking changes that require an update to Content and/or Configs? If it does add a sample here or a link to exactly what changes need to be made to the content.

Code Reviewer Checklist

  • I have verified Unit tests have been written for any new feature(s) and edge cases
  • I have verified any strings visible on UI components are in the strings.xml file
  • I have verifed the CHANGELOG.md file has any notable changes to the codebase
  • I have verified the solution has been implemented in a configurable and generic way for reuseable components
  • I have built and run the FHIRCore app to verify the change fixes the issue and/or does not break the app

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.1%. Comparing base (ac82739) to head (90209bf).
Report is 30 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main   #3144     +/-   ##
=========================================
+ Coverage     29.6%   30.1%   +0.4%     
- Complexity     658     672     +14     
=========================================
  Files          239     239             
  Lines        11204   11231     +27     
  Branches      1948    1953      +5     
=========================================
+ Hits          3323    3384     +61     
+ Misses        7447    7406     -41     
- Partials       434     441      +7     
Flag Coverage Δ
engine 67.0% <ø> (+0.7%) ⬆️
geowidget 47.2% <ø> (ø)
quest 5.7% <ø> (+0.2%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 6 files with indirect coverage changes

@brandy-kay brandy-kay marked this pull request as ready for review March 15, 2024 10:08
Comment on lines 1173 to 1174
assertEquals("resourceId", questionnaireResponse.id)
assertEquals(subjectType, questionnaireConfig.resourceType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These assertions will always be evaluated to be true. The assertions are not done against the meta tags as the name of the test suggests.

Comment on lines 1179 to 1180
val bundleSlot = slot<Bundle>()
val bundle = bundleSlot.captured
Copy link
Collaborator

Choose a reason for hiding this comment

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

The slot is captured here when there is no function called with the bundle as a parameter.

Comment on lines 1195 to 1202
bundle.entry.forEach {
val resource = it.resource
val subjectType = ResourceType.Group
resource.id = "groupId"

assertEquals(subjectType, resource.resourceType)
assertEquals(questionnaireConfig.resourceIdentifier, resource.id)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The assertions here are not done against the metatags as the name of the test suggests.

}

@Test
fun `test saveExtractedResources for resource modification`() = runBlocking {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are we testing here?

context,
)

assertEquals("patientLogicalId", questionnaireResponse.subject.reference)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add more assertions/verifications for this test?

@ellykits
Copy link
Collaborator

Use the same naming convention for the test cases. See the documentation on OpenSRP docs

@ellykits
Copy link
Collaborator

@brandy-kay The tests should be re-written to verify and assert that related entity location meta tag is indeed added to the extracted resources from a questionnaire submission. To achieve this, the questionnaire needs to include a field that will be used to capture the location uuid. The link Id of this field is required to be configured in the Questionnaire Config. Refer to the code changes made in the PR 3097 referenced in the issue.

@brandy-kay
Copy link
Contributor Author

@ellykits Have made some improvements on the test could you please have a look at it

…ation_sync_strategy_enchancements' into Unit_Tests_on_Related_Entity_Location_sync_strategy_enchancements
Comment on lines 1206 to 1207
val listResource = questionnaireResponse.contained.firstOrNull() as ListResource
assertEquals(listResource.id, linkId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you assert that the Related Entity Location resource meta tag was added to the resource meat property?

@brandy-kay
Copy link
Contributor Author

@ellykits have added assertion for the related entity resource meta tag

val listResource = questionnaireResponse.contained.firstOrNull() as ListResource
assertEquals(listResource.id, linkId)
val resource = listResource.entry.firstOrNull()?.item?.resource
if (resource != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this condition use assertNotNull(

…i/questionnaire/QuestionnaireViewModelTest.kt

Co-authored-by: Martin Ndegwa <ndegwamartin@users.noreply.github.com>
subject = patient.asReference()
val listResource =
ListResource().apply {
id = UUID.randomUUID().toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a dummy test uuid so that you can use it in the assert as well (to confirm it is actually the correct one added)

if (resource != null) {
assertTrue(
resource.meta?.tag?.any {
it.system == "https://smartregister.org/related-entity-location-tag-id"
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert the REL UUID as well

Comment on lines 1236 to 1241
Assert.assertNotNull(
resource?.meta?.tag?.any { it.code == metaTagId },
)
Assert.assertNotNull(
resource?.meta?.tag?.any {
it.system == "https://smartregister.org/related-entity-location-tag-id"
Copy link
Contributor

Choose a reason for hiding this comment

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

The any( method of the Iterable interface will always return a Boolean, so this test will always pass regardless of the values checked.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my previous comment for the assertNotNull was only to replace the if condition that was there previously

@brandy-kay brandy-kay enabled auto-merge (squash) April 19, 2024 08:58
@ndegwamartin ndegwamartin enabled auto-merge (squash) April 19, 2024 09:33
@ndegwamartin ndegwamartin merged commit fa880da into main Apr 19, 2024
5 checks passed
@ndegwamartin ndegwamartin deleted the Unit_Tests_on_Related_Entity_Location_sync_strategy_enchancements branch April 19, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write Unit Tests on Related Entity Location sync strategy enchancements
4 participants