[MS-799] Simplify CommCareIdentityDataSourceTest#1479
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors CommCareIdentityDataSourceTest by extracting repeating test setup and verification patterns into helper methods, significantly reducing code duplication and improving maintainability.
Key changes:
- Extracted 6 helper methods for common test operations: cursor setup, assertion logic, and verification steps
- Simplified all test methods by replacing repetitive mock setups and assertions with concise helper calls
- Maintained the same test coverage and behavior while improving readability
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
BurningAXE
force-pushed
the
MS-799-Simplify-CommCareIdentityDataSourceTest
branch
from
3cd86e3 to
d5d7011
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
BurningAXE
force-pushed
the
MS-799-Simplify-CommCareIdentityDataSourceTest
branch
from
d5d7011 to
0698270
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| private fun assertFingerprintIdentitiesMatch( | ||
| expected: List<Identity>, | ||
| actual: List<Identity>, | ||
| templateFormat: String, | ||
| ) { | ||
| val areContentsEqual = expected | ||
| .filter { identity -> identity.samples.any { it.format == templateFormat } } | ||
| .zip(actual) { exp, act -> | ||
| exp.subjectId == act.subjectId && | ||
| exp.samples | ||
| .zip(act.samples) { expSample, actSample -> | ||
| expSample.identifier == actSample.identifier && | ||
| expSample.template.contentEquals(actSample.template) && | ||
| expSample.format == actSample.format | ||
| }.all { it } | ||
| }.all { it } | ||
| assertTrue(areContentsEqual) | ||
| } | ||
|
|
||
| private fun assertFaceIdentitiesMatch( | ||
| expected: List<Identity>, | ||
| actual: List<Identity>, | ||
| templateFormat: String, | ||
| ) { | ||
| val areContentsEqual = expected | ||
| .filter { identity -> identity.samples.any { it.format == templateFormat } } | ||
| .zip(actual) { exp, act -> | ||
| exp.subjectId == act.subjectId && | ||
| exp.samples | ||
| .zip(act.samples) { expSample, actSample -> | ||
| expSample.template.contentEquals(actSample.template) && | ||
| expSample.format == actSample.format | ||
| }.all { it } | ||
| }.all { it } | ||
| assertTrue(areContentsEqual) | ||
| } |
There was a problem hiding this comment.
The assertion logic in both assertFingerprintIdentitiesMatch and assertFaceIdentitiesMatch has a potential bug: if the filtered expected list has a different size than the actual list, the .zip() operation will silently truncate to the shorter list, potentially missing identities that should have been asserted. This could hide bugs where fewer identities are returned than expected. Consider adding a size check: assertEquals(expected.filter { ... }.size, actual.size) before the zip operation.
| private fun assertFingerprintIdentitiesMatch( | ||
| expected: List<Identity>, | ||
| actual: List<Identity>, | ||
| templateFormat: String, | ||
| ) { | ||
| val areContentsEqual = expected | ||
| .filter { identity -> identity.samples.any { it.format == templateFormat } } | ||
| .zip(actual) { exp, act -> | ||
| exp.subjectId == act.subjectId && | ||
| exp.samples | ||
| .zip(act.samples) { expSample, actSample -> | ||
| expSample.identifier == actSample.identifier && | ||
| expSample.template.contentEquals(actSample.template) && | ||
| expSample.format == actSample.format | ||
| }.all { it } | ||
| }.all { it } | ||
| assertTrue(areContentsEqual) | ||
| } | ||
|
|
||
| private fun assertFaceIdentitiesMatch( | ||
| expected: List<Identity>, | ||
| actual: List<Identity>, | ||
| templateFormat: String, | ||
| ) { | ||
| val areContentsEqual = expected | ||
| .filter { identity -> identity.samples.any { it.format == templateFormat } } | ||
| .zip(actual) { exp, act -> | ||
| exp.subjectId == act.subjectId && | ||
| exp.samples | ||
| .zip(act.samples) { expSample, actSample -> | ||
| expSample.template.contentEquals(actSample.template) && | ||
| expSample.format == actSample.format | ||
| }.all { it } | ||
| }.all { it } | ||
| assertTrue(areContentsEqual) | ||
| } |
There was a problem hiding this comment.
The assertion methods assertFingerprintIdentitiesMatch and assertFaceIdentitiesMatch have significant code duplication and provide unhelpful error messages. When assertTrue(areContentsEqual) fails, developers won't know which specific field mismatched. Consider consolidating these into a single parameterized method or using a more descriptive assertion library like AssertJ that would provide detailed diff output on failure.
BurningAXE
requested review from
a team,
TristramN,
alex-vt,
alexandr-simprints,
luhmirin-s,
meladRaouf and
ybourgery
and removed request for
a team
JIRA ticket
Will be released in: 2026.1.0
Notable changes
Testing guidance
Additional work checklist