SDK:309-Switching to the new Date and Time API #226
Conversation
|
|
||
| if (lastReport != null) { | ||
| return (currentDate.getTime() - lastReport.getTime()) / (60 * 60 * 1000 * 24) % 60 > 7; |
There was a problem hiding this comment.
Can we add some tests for things like this?
There was a problem hiding this comment.
@dkayiwa this method is private, Should I write tests for this:
or use reflection?There was a problem hiding this comment.
Is the method that calls it public? If yes, does it have a test?
There was a problem hiding this comment.
This gets called by the following method.
This method does not have any tests. It sends a report to Google Forms.
There was a problem hiding this comment.
Can you create some tests for it?
There was a problem hiding this comment.
I think just making the method public would be easier 😅
There was a problem hiding this comment.
It does not sound ideal. In that case, i would rather go for the hacky reflection. 😊
|
@dkayiwa I made a few changes to the code and added the tests. Is this alright? |
| @@ -37,4 +42,20 @@ public void shouldSetStatsEnabledMode(){ | |||
| assertThat(sdkStatistics.getStatsEnabled(), is(true)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testCheckIfOneWeekApart_LessThanOneWeekApart() { | |||
There was a problem hiding this comment.
What i had in mind was tests for the original method checkIfOneWeekFromLastReport such that they still pass even after the method internal implementation details are changed.
| if (lastReport != null) { | ||
| return (currentDate.getTime() - lastReport.getTime()) / (60 * 60 * 1000 * 24) % 60 > 7; | ||
| } else { | ||
| public boolean checkIfOneWeekApart(LocalDate lastDate) { |
There was a problem hiding this comment.
It does not look right to make this method public only for the sake of tests.
There was a problem hiding this comment.
Can we completely avoid creating this new method?
|
@dkayiwa I changed the tests with reflection. |
|
|
||
| /** | ||
| * | ||
| */ | ||
| public class SdkStatisticsTest { | ||
|
|
||
| private static final String DATE_FORMAT = "dd-M-yyyy"; |
There was a problem hiding this comment.
What is the use of this constant?
There was a problem hiding this comment.
to format the dates for statsLastReported property.
There was a problem hiding this comment.
I could change the code like this if you want.
private final DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern("dd-M-yyyy");
I see quite a number of repeated lines in your tests. |
@dkayiwa I refactored the tests. |
|
@dkayiwa I didn't squash the commits. Is that alright?? |
No need to do so because i squash on merge. |
* SDK:309-Switching to the new Date and Time API * SDK-308: Switching to the new Date and Time API * SDK-309: Adding tests * SDK-309: Changing tests * SDK-309: Refactoring tests
* SDK:309-Switching to the new Date and Time API * SDK-308: Switching to the new Date and Time API * SDK-309: Adding tests * SDK-309: Changing tests * SDK-309: Refactoring tests
Jira ticket: https://issues.openmrs.org/browse/SDK-309