TRUNK-6598: Add missing unit test for containingAnyFormField in HibernateFormDAO#5983
TRUNK-6598: Add missing unit test for containingAnyFormField in HibernateFormDAO#5983ShrutiSocrates wants to merge 6 commits intoopenmrs:masterfrom
Conversation
Add unit test for containingAnyFormField in HibernateFormDAO
| // Assert at least one form contains this form field | ||
| assertTrue(forms.size() > 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Are there any edge cases where multiple form fields exist but none match? It might be useful to include such scenarios in the tests.
There was a problem hiding this comment.
Good point. You're right—we should cover the scenario with multiple
non-existent fields
There was a problem hiding this comment.
Thank you for the suggestion! I have committed it directly.
There was a problem hiding this comment.
Nice work! @ShrutiSocrates .. I see there is thorough test coverage for the containingAnyFormField functionality as the ticket required....Looks good
Just one thing the comment "//A would-be fix for that" should be removed before merge
There was a problem hiding this comment.
Thank you for the approval and feedback! The "//A would-be fix for that" comment
has been removed.
VarshithReddy2006
left a comment
VarshithReddy2006
left a comment
There was a problem hiding this comment.
Suggested adding additional edge case coverage for test scenarios.
NitinKumar1-1
left a comment
NitinKumar1-1
left a comment
There was a problem hiding this comment.
Looks good to me
Agree with the suggestion about adding an edge case for non-matching form fields.
chaiwat123431
left a comment
chaiwat123431
left a comment
There was a problem hiding this comment.
Good addition. Filling in a TODO with a meaningful test is exactly the kind of contribution that improves long-term maintainability. The test covers the happy path well. Consider also adding an edge case where no form fields match, to verify the method returns an empty result rather than throwing an exception.
Add edge case tests for containingAnyFormField
|
Thank you for the feedback! I have added an edge case to verify the method returns an empty result when no form fields match rather Please review when you get a chance! |
api/src/test/java/org/openmrs/api/db/hibernate/HibernateFormDAOTest.java
Show resolved
Hide resolved
…OTest.java Add edge case for multiple non-existent form fields Co-authored-by: EDSON <[email protected]>
Good point! I have added this edge case covering multiple non-existent form fields that return an empty list. |
Add multiple non-existent fields edge case test
|
| // Assert at least one form contains this form field | ||
| assertTrue(forms.size() > 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice work! @ShrutiSocrates .. I see there is thorough test coverage for the containingAnyFormField functionality as the ticket required....Looks good
Just one thing the comment "//A would-be fix for that" should be removed before merge
|
Thank you so much @EDSONZ-WASSWA for the approval and feedback! I will remove the "//A would-be fix for that" comment right away. |
5 participants
TRUNK-6598: Add missing unit test for containingAnyFormField in HibernateFormDAO
Description of what I changed
Added a missing unit test for the containingAnyFormField functionality in HibernateFormDAO as indicated by the TODO comment on line 499.
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6598
Checklist: I completed these to help reviewers :)