TRUNK-5472: Add unit tests for Allergy#hasSameValues#6097
Conversation
📝 WalkthroughWalkthroughRefactors Allergy.hasSameValues into null-safe private helper checks and updates reaction comparison logic; adds a JUnit5 test class exercising equality, inequality, reaction, and null scenarios across Allergy fields. ChangesAllergy.hasSameValues refactor + tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.24.0)api/src/main/java/org/openmrs/Allergy.java[ERROR] Cannot load ruleset rulesets/java/android.xml/DoNotHardCodeSDCard: Cannot resolve rule/ruleset reference 'rulesets/java/android.xml/DoNotHardCodeSDCard'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. api/src/test/java/org/openmrs/AllergyTest.java[ERROR] Cannot load ruleset rulesets/java/android.xml/DoNotHardCodeSDCard: Cannot resolve rule/ruleset reference 'rulesets/java/android.xml/DoNotHardCodeSDCard'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/src/test/java/org/openmrs/AllergyTest.java (2)
25-178: ⚡ Quick winNice work on comprehensive test coverage!
Your tests cover the main comparison scenarios thoroughly. As a future enhancement, you might consider adding a few edge cases to make the suite even more robust:
- Null comment comparison (one allergy with null comment, another with a value)
- Null severity comparison
- Comparing a coded allergen with a non-coded allergen
- Comparing allergies with different
AllergenTypevalues (e.g., DRUG vs FOOD for coded allergens)These are nice-to-have additions rather than essential—the current coverage is solid for the main use cases!
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/org/openmrs/AllergyTest.java` around lines 25 - 178, Add tests to AllergyTest to cover null/comment and null/severity edge cases and comparisons between coded vs non-coded allergens and differing AllergenType values: create new `@Test` methods (e.g., shouldHandleNullCommentComparison, shouldHandleNullSeverityComparison, shouldCompareCodedWithNonCodedAllergen, shouldCompareDifferentAllergenTypes) that reuse the existing helper constructors allergy(...), patient(...), codedAllergen(...), nonCodedAllergen(...), severity(...), and reaction(...); for each test construct left/right Allergy instances that reflect the edge case (one side null or different AllergenType) and assert expected behavior with hasSameValues (assertTrue/assertFalse or assertThrows where appropriate). Ensure tests assert not only boolean results but also reuse assertNotSame where relevant to confirm instance differences.
126-132: Good catch documenting this exception behavior!This test correctly documents that
hasSameValuesthrows aNullPointerExceptionwhen reaction IDs differ. While the test itself is appropriate for documenting existing behavior, this suggests the underlying implementation might have a defensive coding gap—typically, comparison methods should returnfalserather than throw NPE for mismatches.This isn't something to fix in this PR (you're testing existing behavior, which is exactly right!), but it might be worth filing a follow-up issue to investigate whether the implementation should handle this case more gracefully.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/org/openmrs/AllergyTest.java` around lines 126 - 132, The test shows Allergy.hasSameValues currently throws a NullPointerException when reaction IDs differ; update the Allergy.hasSameValues implementation to be null-safe and return false for non-matching reactions instead of letting an NPE propagate: in the Allergy.hasSameValues method (and any helper that compares Reaction objects/IDs) add explicit null checks or use Objects.equals on reaction.getId() values when comparing reaction lists, ensure mismatched IDs yield false, and keep the existing test (or add a new one) to assert the new behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/src/test/java/org/openmrs/AllergyTest.java`:
- Around line 25-178: Add tests to AllergyTest to cover null/comment and
null/severity edge cases and comparisons between coded vs non-coded allergens
and differing AllergenType values: create new `@Test` methods (e.g.,
shouldHandleNullCommentComparison, shouldHandleNullSeverityComparison,
shouldCompareCodedWithNonCodedAllergen, shouldCompareDifferentAllergenTypes)
that reuse the existing helper constructors allergy(...), patient(...),
codedAllergen(...), nonCodedAllergen(...), severity(...), and reaction(...); for
each test construct left/right Allergy instances that reflect the edge case (one
side null or different AllergenType) and assert expected behavior with
hasSameValues (assertTrue/assertFalse or assertThrows where appropriate). Ensure
tests assert not only boolean results but also reuse assertNotSame where
relevant to confirm instance differences.
- Around line 126-132: The test shows Allergy.hasSameValues currently throws a
NullPointerException when reaction IDs differ; update the Allergy.hasSameValues
implementation to be null-safe and return false for non-matching reactions
instead of letting an NPE propagate: in the Allergy.hasSameValues method (and
any helper that compares Reaction objects/IDs) add explicit null checks or use
Objects.equals on reaction.getId() values when comparing reaction lists, ensure
mismatched IDs yield false, and keep the existing test (or add a new one) to
assert the new behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 690bdd19-b2a0-42e1-9776-d06f2348fa38
📒 Files selected for processing (1)
api/src/test/java/org/openmrs/AllergyTest.java
|
@ibacher, @mogoodrich, @mseaton could you review this PR please. Thanks |
| Allergy left = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of(reaction(5, 6, "rash"))); | ||
| Allergy right = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of(reaction(99, 6, "rash"))); | ||
|
|
||
| assertThrows(NullPointerException.class, () -> left.hasSameValues(right)); |
There was a problem hiding this comment.
This test appears to codify a NullPointerException as expected behavior when the reaction IDs differ.
In this scenario, both allergies have one reaction, but left has reaction ID 5 and right has reaction ID 99. Allergy#hasSameReactions(...) looks up the matching reaction by ID, so allergy.getAllergyReaction(5) returns null, and then AllergyReaction#hasSameValues(null) throws.
Since Allergy#hasSameValues(...) is documented to return true when values match and false otherwise, a missing matching reaction seems like a “values differ” case rather than an exception case.
Should we update the production code to return false when no matching reaction is found, and then change this test to assert false instead of expecting an NPE?
For example:
AllergyReaction rc = allergy.getAllergyReaction(reaction.getAllergyReactionId());
if (rc == null || !reaction.hasSameValues(rc)) {
return false;
}Then the test can become:
assertFalse(left.hasSameValues(right));There was a problem hiding this comment.
Yes - i think that is a good idea. Its also possible that other Exceptions are thrown. So I think the method needs refactoring. Since no tests currently exist, I thought it would be a case of putting the tests in first and then refactoring the method (and changing tests are required). But also happy to do the refactor as part of the tests.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/src/test/java/org/openmrs/AllergyTest.java (1)
126-132:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe NullPointerException behavior may be unintentional.
This test codifies an NPE as expected behavior when reaction IDs differ. As noted in the previous review, this seems like a case where
hasSameValuesshould returnfalserather than throw an exception. When the method looks up a reaction by ID and finds no match, that indicates the values differ, not an error condition.Consider updating
Allergy#hasSameReactionsto handle the missing reaction case:AllergyReaction rc = allergy.getAllergyReaction(reaction.getAllergyReactionId()); if (rc == null || !reaction.hasSameValues(rc)) { return false; }Then this test can assert
falseinstead of expecting an NPE:assertFalse(left.hasSameValues(right));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/org/openmrs/AllergyTest.java` around lines 126 - 132, The test currently expects a NullPointerException when reaction IDs differ, but instead update Allergy#hasSameReactions (invoked by Allergy#hasSameValues) to treat a missing matched reaction as a difference: when calling getAllergyReaction(reaction.getAllergyReactionId()), if that returns null or if allergyReaction.hasSameValues(...) is false, return false rather than allowing an NPE; then change the test to assertFalse(left.hasSameValues(right)) to reflect the corrected behavior.
🧹 Nitpick comments (2)
api/src/test/java/org/openmrs/AllergyTest.java (2)
118-124: ⚡ Quick winConsider testing reactionNonCoded field differences.
Your test verifies that reactions with different concept IDs cause
hasSameValuesto return false, which is great! For completeness, you might also want a test that verifies thereactionNonCodedfield is compared. Currently, yourreactionhelper always sets this field, but there's no test where two reactions have the same concept but differentreactionNonCodedvalues.For example:
`@Test` void shouldReturnFalseWhenReactionNonCodedDiffers() { Allergy left = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of(reaction(5, 6, "rash"))); Allergy right = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of(reaction(5, 6, "hives"))); assertFalse(left.hasSameValues(right)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/org/openmrs/AllergyTest.java` around lines 118 - 124, Add a new unit test in AllergyTest (e.g., shouldReturnFalseWhenReactionNonCodedDiffers) that constructs two Allergy instances using the existing allergy(...) and reaction(...) helpers where the reactions have the same concept IDs but different reactionNonCoded strings, then assertFalse(left.hasSameValues(right)); this verifies hasSameValues compares the reactionNonCoded field as well.
77-83: ⚡ Quick winConsider adding a test for AllergenType difference.
Your test suite thoroughly covers allergen value differences (coded concept ID and non-coded string), but doesn't explicitly test the case where two allergies have different
AllergenTypevalues (e.g.,DRUGvsFOOD). IfAllergen#hasSameValuescompares the type field, adding a test for that scenario would improve completeness.For example:
`@Test` void shouldReturnFalseWhenAllergenTypesDiffer() { Allergen drugAllergen = new Allergen(); drugAllergen.setAllergenType(AllergenType.DRUG); drugAllergen.setCodedAllergen(concept(3)); Allergen foodAllergen = new Allergen(); foodAllergen.setAllergenType(AllergenType.FOOD); foodAllergen.setCodedAllergen(concept(3)); Allergy left = allergy(1, patient(2), drugAllergen, severity(4), "comment", List.of()); Allergy right = allergy(1, patient(2), foodAllergen, severity(4), "comment", List.of()); assertFalse(left.hasSameValues(right)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/org/openmrs/AllergyTest.java` around lines 77 - 83, Add a unit test that verifies Allergies with the same coded allergen but different AllergenType are not considered equal: create two Allergen instances (setAllergenType to AllergenType.DRUG and AllergenType.FOOD respectively) with the same coded allergen (use concept(3) or existing concept helper), wrap them using the allergy(...) helper to build left and right Allergy objects, and assert that left.hasSameValues(right) returns false; reference Allergen, AllergenType, Allergy, hasSameValues, and the allergy(...) helper to locate the appropriate test scaffolding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/test/java/org/openmrs/AllergyTest.java`:
- Around line 1-179: The CI is failing due to Spotless formatting violations in
AllergyTest (class AllergyTest and its helper methods allergy(), patient(),
codedAllergen(), nonCodedAllergen(), concept(), severity(), reaction()); fix by
running the Spotless formatter (mvn spotless:apply) or applying the same
formatting rules to this file, then reformat and commit the updated
AllergyTest.java so imports, spacing, and indentation match the project's
Spotless configuration.
---
Duplicate comments:
In `@api/src/test/java/org/openmrs/AllergyTest.java`:
- Around line 126-132: The test currently expects a NullPointerException when
reaction IDs differ, but instead update Allergy#hasSameReactions (invoked by
Allergy#hasSameValues) to treat a missing matched reaction as a difference: when
calling getAllergyReaction(reaction.getAllergyReactionId()), if that returns
null or if allergyReaction.hasSameValues(...) is false, return false rather than
allowing an NPE; then change the test to assertFalse(left.hasSameValues(right))
to reflect the corrected behavior.
---
Nitpick comments:
In `@api/src/test/java/org/openmrs/AllergyTest.java`:
- Around line 118-124: Add a new unit test in AllergyTest (e.g.,
shouldReturnFalseWhenReactionNonCodedDiffers) that constructs two Allergy
instances using the existing allergy(...) and reaction(...) helpers where the
reactions have the same concept IDs but different reactionNonCoded strings, then
assertFalse(left.hasSameValues(right)); this verifies hasSameValues compares the
reactionNonCoded field as well.
- Around line 77-83: Add a unit test that verifies Allergies with the same coded
allergen but different AllergenType are not considered equal: create two
Allergen instances (setAllergenType to AllergenType.DRUG and AllergenType.FOOD
respectively) with the same coded allergen (use concept(3) or existing concept
helper), wrap them using the allergy(...) helper to build left and right Allergy
objects, and assert that left.hasSameValues(right) returns false; reference
Allergen, AllergenType, Allergy, hasSameValues, and the allergy(...) helper to
locate the appropriate test scaffolding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74a4359c-432f-4406-a432-2943cf49fdfd
📒 Files selected for processing (1)
api/src/test/java/org/openmrs/AllergyTest.java
jwnasambu
left a comment
jwnasambu
left a comment
There was a problem hiding this comment.
@tony-waters This is a good progress. Kindly run mvn spotless:apply and commit the resulting formatting changes since the current pipeline failures are partly caused by Spotless violations.
| class AllergyTest { | ||
|
|
||
| @Test | ||
| void shouldReturnTrueWhenAllValuesMatch() { |
There was a problem hiding this comment.
Let's try to follow our test conventions and add hasSameValues_ prefix to test methods.
ex:
hasSameValues_shouldReturnTrueWhenAllValuesMatch
| assertTrue(left.hasSameValues(right)); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Let's try to add a @see comment to point out the method we are testing.
/**
* @see Allergy#hasSameValues(Allergy)
*/
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
api/src/main/java/org/openmrs/Allergy.java (2)
348-390: ⚡ Quick winRemove commented-out dead code before merging.
These ~60 lines of commented-out code make the file harder to navigate. Since the code is version-controlled, you can always retrieve the old implementation from Git history if needed. Keeping it commented out just adds noise.
Also applies to: 414-427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/main/java/org/openmrs/Allergy.java` around lines 348 - 390, Remove the commented-out legacy implementation of hasSameValues (and the other commented block later) from the Allergy class: delete the entire commented method body that references hasSameValues, hasSameReactions, getAllergyId, getPatient, getAllergen, getSeverity, getComment, and related nullSafeEquals checks so the file contains only active code; rely on VCS for history and ensure no other code references the commented methods before committing.
291-291: 💤 Low valueConsider using
getComments()instead of deprecatedgetComment().Since you're already touching this code, swapping to the non-deprecated accessor keeps the implementation consistent with the rest of the codebase. It's a small change but avoids deprecation warnings.
♻️ Proposed fix
- && OpenmrsUtil.nullSafeEquals(getComment(), allergy.getComment()) + && OpenmrsUtil.nullSafeEquals(getComments(), allergy.getComments())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/main/java/org/openmrs/Allergy.java` at line 291, In the Allergy.equals implementation replace the deprecated accessor calls getComment() with the non-deprecated getComments() so the null-safe comparison uses OpenmrsUtil.nullSafeEquals(getComments(), allergy.getComments()); update both occurrences in the equality check inside the Allergy class to keep consistency with the rest of the codebase and remove the deprecation warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/test/java/org/openmrs/AllergyTest.java`:
- Around line 134-139: The test shouldThrowWhenComparedAllergyIsNull incorrectly
expects a NullPointerException but Allergy.hasSameValues(Allergy) explicitly
returns false for null; update the test (method
shouldThrowWhenComparedAllergyIsNull) to assert that allergy.hasSameValues(null)
is false (use assertFalse or equivalent) so it matches the current
Allergy.hasSameValues() behavior.
- Around line 141-155: Update the two tests to assert the actual boolean
behavior instead of expecting an exception: replace
assertThrows(NullPointerException.class, () -> left.hasSameValues(right)) with
assertions that left.hasSameValues(right) returns false for the cases where one
allergen is null and the other is not (tests invoking hasSameValues which
delegates to hasSameAllergenValues); also consider adding a test that when both
allergens are null hasSameValues(...) returns true to cover the existing branch
that returns thisAllergen == otherAllergen.
- Around line 126-132: The test method named
shouldThrowWhenOtherAllergyDoesNotContainReactionWithSameId is misnamed because
it doesn't expect an exception; rename it to reflect the actual assertion (e.g.,
shouldReturnFalseWhenOtherAllergyDoesNotContainReactionWithSameId) and update
the method declaration in AllergyTest (the test that calls
left.hasSameValues(right) and uses assertFalse) so the name matches intent and
improves readability.
---
Nitpick comments:
In `@api/src/main/java/org/openmrs/Allergy.java`:
- Around line 348-390: Remove the commented-out legacy implementation of
hasSameValues (and the other commented block later) from the Allergy class:
delete the entire commented method body that references hasSameValues,
hasSameReactions, getAllergyId, getPatient, getAllergen, getSeverity,
getComment, and related nullSafeEquals checks so the file contains only active
code; rely on VCS for history and ensure no other code references the commented
methods before committing.
- Line 291: In the Allergy.equals implementation replace the deprecated accessor
calls getComment() with the non-deprecated getComments() so the null-safe
comparison uses OpenmrsUtil.nullSafeEquals(getComments(),
allergy.getComments()); update both occurrences in the equality check inside the
Allergy class to keep consistency with the rest of the codebase and remove the
deprecation warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0bb3615-826e-4954-9816-21530b78368d
📒 Files selected for processing (2)
api/src/main/java/org/openmrs/Allergy.javaapi/src/test/java/org/openmrs/AllergyTest.java
| @Test | ||
| void shouldThrowWhenOtherAllergyDoesNotContainReactionWithSameId() { | ||
| Allergy left = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of(reaction(5, 6, "rash"))); | ||
| Allergy right = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of(reaction(99, 6, "rash"))); | ||
|
|
||
| assertFalse(left.hasSameValues(right)); | ||
| } |
There was a problem hiding this comment.
Rename test method — it doesn't expect a throw.
The test name shouldThrowWhenOtherAllergyDoesNotContainReactionWithSameId implies an exception is expected, but the assertion is assertFalse(...). This mismatch will confuse future readers.
✏️ Proposed rename
`@Test`
- void shouldThrowWhenOtherAllergyDoesNotContainReactionWithSameId() {
+ void hasSameValues_shouldReturnFalseWhenOtherAllergyDoesNotContainReactionWithSameId() {
Allergy left = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of(reaction(5, 6, "rash")));
Allergy right = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of(reaction(99, 6, "rash")));
assertFalse(left.hasSameValues(right));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/test/java/org/openmrs/AllergyTest.java` around lines 126 - 132, The
test method named shouldThrowWhenOtherAllergyDoesNotContainReactionWithSameId is
misnamed because it doesn't expect an exception; rename it to reflect the actual
assertion (e.g.,
shouldReturnFalseWhenOtherAllergyDoesNotContainReactionWithSameId) and update
the method declaration in AllergyTest (the test that calls
left.hasSameValues(right) and uses assertFalse) so the name matches intent and
improves readability.
| @Test | ||
| void shouldThrowWhenComparedAllergyIsNull() { | ||
| Allergy allergy = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of()); | ||
|
|
||
| assertThrows(NullPointerException.class, () -> allergy.hasSameValues(null)); | ||
| } |
There was a problem hiding this comment.
Test will fail — implementation returns false for null, doesn't throw.
Looking at Allergy.hasSameValues() lines 284-286:
if (allergy == null) {
return false;
}The implementation explicitly handles null by returning false, so assertThrows(NullPointerException.class, ...) will fail. Update this test to expect false instead:
🐛 Proposed fix
`@Test`
- void shouldThrowWhenComparedAllergyIsNull() {
+ void hasSameValues_shouldReturnFalseWhenComparedAllergyIsNull() {
Allergy allergy = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of());
- assertThrows(NullPointerException.class, () -> allergy.hasSameValues(null));
+ assertFalse(allergy.hasSameValues(null));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/test/java/org/openmrs/AllergyTest.java` around lines 134 - 139, The
test shouldThrowWhenComparedAllergyIsNull incorrectly expects a
NullPointerException but Allergy.hasSameValues(Allergy) explicitly returns false
for null; update the test (method shouldThrowWhenComparedAllergyIsNull) to
assert that allergy.hasSameValues(null) is false (use assertFalse or equivalent)
so it matches the current Allergy.hasSameValues() behavior.
| @Test | ||
| void shouldThrowWhenThisAllergenIsNull() { | ||
| Allergy left = allergy(1, patient(2), null, severity(4), "comment", List.of()); | ||
| Allergy right = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of()); | ||
|
|
||
| assertThrows(NullPointerException.class, () -> left.hasSameValues(right)); | ||
| } | ||
|
|
||
| @Test | ||
| void shouldThrowWhenComparedAllergenIsNull() { | ||
| Allergy left = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of()); | ||
| Allergy right = allergy(1, patient(2), null, severity(4), "comment", List.of()); | ||
|
|
||
| assertThrows(NullPointerException.class, () -> left.hasSameValues(right)); | ||
| } |
There was a problem hiding this comment.
Tests will fail — implementation handles null allergen gracefully.
Looking at hasSameAllergenValues() lines 308-310:
if (thisAllergen == null || otherAllergen == null) {
return thisAllergen == otherAllergen;
}When one allergen is null and the other isn't, it returns false (not throws). When both are null, it returns true. Update these tests to match the actual behavior:
🐛 Proposed fix
`@Test`
- void shouldThrowWhenThisAllergenIsNull() {
+ void hasSameValues_shouldReturnFalseWhenThisAllergenIsNull() {
Allergy left = allergy(1, patient(2), null, severity(4), "comment", List.of());
Allergy right = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of());
- assertThrows(NullPointerException.class, () -> left.hasSameValues(right));
+ assertFalse(left.hasSameValues(right));
}
`@Test`
- void shouldThrowWhenComparedAllergenIsNull() {
+ void hasSameValues_shouldReturnFalseWhenComparedAllergenIsNull() {
Allergy left = allergy(1, patient(2), codedAllergen(3), severity(4), "comment", List.of());
Allergy right = allergy(1, patient(2), null, severity(4), "comment", List.of());
- assertThrows(NullPointerException.class, () -> left.hasSameValues(right));
+ assertFalse(left.hasSameValues(right));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/test/java/org/openmrs/AllergyTest.java` around lines 141 - 155,
Update the two tests to assert the actual boolean behavior instead of expecting
an exception: replace assertThrows(NullPointerException.class, () ->
left.hasSameValues(right)) with assertions that left.hasSameValues(right)
returns false for the cases where one allergen is null and the other is not
(tests invoking hasSameValues which delegates to hasSameAllergenValues); also
consider adding a test that when both allergens are null hasSameValues(...)
returns true to cover the existing branch that returns thisAllergen ==
otherAllergen.
…-test' into TRUNK-5472-allergy-hasSameValues-test
|
I have added the suggested changes from @jwnasambu and @wikumChamith. Once the unit tests are merged to master I would like to address the point raised by @denniskigen by refactoring the hasSameValues() method to not throw NullPointerExceptions but return false instead. Can you review again please @denniskigen @dkayiwa @jwnasambu @wikumChamith . |
| } | ||
|
|
||
| @Test | ||
| void hasSameValues_shouldThrowWhenOtherAllergyDoesNotContainReactionWithSameId() { |
There was a problem hiding this comment.
This test codifies a latent NPE bug as spec. I think we should remove it for now. It would also be better to fix the bug before merging this :)
There was a problem hiding this comment.
@wikumChamith - So should I (1) remove the NPE tests, or (2) fix the hasSameValues() method at the same time as the tests. Thanks :)
There was a problem hiding this comment.
I have refactored the Allergy.hasSameValues method and changed the tests to reflect it no longer throws Exceptions.
There was a problem hiding this comment.
Can you review again please @denniskigen @dkayiwa @jwnasambu @wikumChamith .
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6097 +/- ##
============================================
+ Coverage 59.04% 59.08% +0.04%
- Complexity 9239 9249 +10
============================================
Files 693 693
Lines 37257 37257
Branches 5485 5485
============================================
+ Hits 21998 22013 +15
+ Misses 13287 13277 -10
+ Partials 1972 1967 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
6 participants
Description of what I changed
Created a
AllergyTestclass with unit tests for the Allergy#hasSameValues method.Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-5472
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream masterSummary by CodeRabbit