[incubator-kie-issues#1612] Adding validation for calendar properties. #3769
Conversation
Abhitocode
changed the title
|
PR job Reproducerbuild-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u #3769 --skipParallelCheckout NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3769/1/display/redirect Test results:
Those are the test failures: org.jbpm.process.core.timer.BusinessCalendarImplTest.testCalculateMinutesPassingHolidayInvalid configuration: business.hours.per.day must be equal to the difference between business.end.hour and business.start.hour.org.jbpm.process.core.timer.BusinessCalendarImplTest.testCalculateHoursCustomWorkingHoursexpected: "2012-05-04 15:45" but was: "2012-05-07 09:45" org.jbpm.bpmn2.calendar.BusinessCalendarTest.(?)Invalid configuration: business.end.hour must be between 0 and 23. |
| Field weekendDaysField = BusinessCalendarImpl.class.getDeclaredField("weekendDays"); | ||
| weekendDaysField.setAccessible(true); | ||
|
|
||
| assertEquals(expectedValuesMap.get(DAYS_PER_WEEK), daysPerWeekField.get(businessCalendar)); |
There was a problem hiding this comment.
Please do not use assertEquals. Use assertj assertions instead.
| List<BusinessCalendarImpl> businessCalendarList = new ArrayList<>(); | ||
| assertDoesNotThrow(() -> { | ||
| businessCalendarList.add(new BusinessCalendarImpl(businessCalendarProperties)); | ||
| }); |
There was a problem hiding this comment.
You are testing that you can create the object in the proper way. Remove the adding to the list, this is not part of the test at all.
| assertDoesNotThrow(() -> { | ||
| businessCalendarList.add(new BusinessCalendarImpl(businessCalendarProperties)); | ||
| }); | ||
| assertCalendarProperties(businessCalendarList.get(0), expectedValuesMap); |
There was a problem hiding this comment.
The method hides what are your intentions. Given that it is used only here, it is easier to inline the whole method.
|
|
||
| private void assertCalendarProperties(BusinessCalendarImpl businessCalendar, Map<String, Object> expectedValuesMap) throws NoSuchFieldException, IllegalAccessException { | ||
| Field daysPerWeekField = BusinessCalendarImpl.class.getDeclaredField("daysPerWeek"); | ||
| daysPerWeekField.setAccessible(true); |
There was a problem hiding this comment.
these two lines are a good candidate for a small helper method that takes as input a string fieldName and returns a field
At that point you can do a test like assertThat(field("startHour").get(businessCalendar).isEqualTo(startHour).
Modified BusinessCalendarImpl class to include validation for calendar properties preventing misconfigurations.
Modified init as there should be no validation error and consider default value incase of scenario where user does not provide value for end hours per day/hours per day/start hour.
Added validations for properties to throw exceptions when calendar.properties file is misconfigured.
Added test testValidation() in BusinessCalendarImplTest to verify the modified intialization logic.
Closes: apache/incubator-kie-issues#1612