TRUNK-5068: Add tests to ProgramWorkflowService #4600
Conversation
|
Can you look into the above build failures? |
|
yes, Im checking it right now! |
|
Just updated the code, please have a look. |
| @@ -465,7 +465,7 @@ public List<PatientProgram> getPatientProgramByAttributeNameAndValue(String attr | |||
| Query query; | |||
| try { | |||
| query = sessionFactory.getCurrentSession().createQuery( | |||
| "SELECT pp FROM patient_program pp " + | |||
| "SELECT pp FROM PatientProgram pp " + | |||
There was a problem hiding this comment.
Was this change also related to the ticket you were working on?
There was a problem hiding this comment.
yes, because after my changes, some other tests were failing.
There was a problem hiding this comment.
Would this mean that it was some sort of bug that your tests exposed?
There was a problem hiding this comment.
No! for this particular case, my task was to implement tests for this method getPatientProgramByAttributeNameAndValue from ProgramWorkflowService class. But after implementing the test, I faced an error related to the table mappings patient_program pp is not mapped [select pp from ....
So I applied this changes to deal with the related issue.
There was a problem hiding this comment.
I still do not get it. If it was not a bug, why do you need to fix it?
There was a problem hiding this comment.
It was, but not related to any code added by me. The sessionfactory query was there, but was not covered by the tests. once I wrote a test, the error came up.
There was a problem hiding this comment.
Now, that makes perfect sense. Thanks for the clarification. 😊
There was a problem hiding this comment.
Thank for your time.
I will be pushing the last changes (naming conventions) shortly
pom.xml
Outdated
| @@ -1210,7 +1210,7 @@ | |||
| <hibernateVersion>5.6.15.Final</hibernateVersion> | |||
| <hibernateSearchVersion>5.11.12.Final</hibernateSearchVersion> | |||
| <luceneVersion>5.5.5</luceneVersion> | |||
| <aspectjVersion>1.9.21.2</aspectjVersion> | |||
There was a problem hiding this comment.
Is this change also related to the ticket you were working on?
There was a problem hiding this comment.
this one is related to the rebase process, to have the latest code from the repo. So. this changes come in
There was a problem hiding this comment.
If you do it properly, your pull request will not have this change because it is not related to the ticket.
There was a problem hiding this comment.
Did you just forget removing this change from the pull request?
| * @see ProgramWorkflowService#getPatientProgramAttributeByAttributeName(List, String) | ||
| */ | ||
| @Test | ||
| public void getProgramAttributeByAttributeName_shouldPass(){ |
There was a problem hiding this comment.
Instead of shouldPass do you mind taking a look at the rest of the tests to follow the same naming convention? 😊
|
Can you squash these commits into one? |
…lation on purge program_attribute_type
|
@dkayiwa just applied all your recommendations |
|
@helderjosue as you wait for review, feel free to take on other tickets. |
| @@ -1095,6 +1101,42 @@ public void triggerStateConversion_shouldTestTransitionToState(){ | |||
| pwsi.triggerStateConversion(patient, trigger, dateConverted); | |||
| assertEquals(patientProgram.getStates().size(), (patientStatesSize + 1)); | |||
| } | |||
|
|
|||
| /** | |||
| * Test to check the implementation of Patient Program Attribute Type by Uuid | |||
There was a problem hiding this comment.
Given the method name, do would these comments add any value? 😊
https://improvingsoftware.com/2011/06/27/5-best-practices-for-commenting-your-code/
There was a problem hiding this comment.
I really think so, since methods generally have a maximum number of characters in their names, it is possible to have a name that does not fully explain the purpose of the method, so it is good to have a javadoc to give more details if they exist, especially when the method is complex.
However, if the practice is not valid, they may be removed.
There was a problem hiding this comment.
Ofcourse there will always be cases where we need comments. We just cannot completely get rid of them. 😊
But for these particular methods of yours, do they add any value?
There was a problem hiding this comment.
I just answered that, but after we agreed on the previous comment, makes me feel like they don't add any value. Correct?
There was a problem hiding this comment.
In that case, would we keep them?
There was a problem hiding this comment.
No! I will remove them 😏
Description of what I changed
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-5068
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 master