Added 'long' as a target type for 'convert_entry_type' processor #4359
Conversation
Signed-off-by: Utkarsh Agarwal <[email protected]>
Utkarsh-Aga
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
kkondaka,
asifsmohammed,
dlvenable and
oeyh
as code owners
Signed-off-by: Utkarsh Agarwal <[email protected]>
Signed-off-by: Utkarsh Agarwal <[email protected]>
Signed-off-by: Utkarsh Agarwal <[email protected]>
Signed-off-by: Utkarsh Agarwal <[email protected]>
There was a problem hiding this comment.
@Utkarsh-Aga , Thank you for this contribution. I have a couple comments. One is that the build itself is failing.
| void testInvalidStringConversion() { | ||
| IntegerConverter converter = new IntegerConverter(); | ||
| assertThrows(IllegalArgumentException.class, () -> converter.convert(new Object())); | ||
| IntegerConverter converter = new IntegerConverter(); |
There was a problem hiding this comment.
I'm seeing the following failure:
/home/runner/work/data-prepper/data-prepper/data-prepper-api/src/test/java/org/opensearch/dataprepper/typeconverter/LongConverterTests.java:51: error: variable converter is already defined in method testInvalidStringConversion()
IntegerConverter converter = new IntegerConverter();
I think you can remove lines 51 and 52 and they repeat the same tests from 49-50.
There was a problem hiding this comment.
Was a mistake on my part of using IntegerConverter in LongConverterTests. Fixed it.
| @Test | ||
| void testDoubleToLongConversion() { | ||
| LongConverter converter = new LongConverter(); | ||
| final Double doubleConstant = (double)12345678.12345678; |
There was a problem hiding this comment.
It may be good to have a test that converts a value which is larger than the maximum integer in addition to this one. You could make a large double constant like 2.0 * Integer.MAX_INT.
There was a problem hiding this comment.
Add a test case testDoubleToLongConversionWithBigValue for it.
Signed-off-by: Utkarsh Agarwal <[email protected]>
|
Thanks a lot @dlvenable for the review. I have made the changes to the commit and tried to address the comments. |
| final Double doubleConstant = (double)12345678.12345678; | ||
| assertThat(converter.convert(doubleConstant), equalTo((long)(double)doubleConstant)); | ||
| } | ||
| @Test |
There was a problem hiding this comment.
These test cases look good. One thing you could do to make it easier to add more tests would be to use @ParameterizedTest with an Arguments source. An example of that can be found here (
)There was a problem hiding this comment.
Added the ParameterizedTest for all the test except for testInvalidStringConversion as had to create a MethodSource.
|
Looks like build is failing due to jacoco test coverage reaching .9 only and being set at 1. Are you able to see the report to see which instructions are being missed in the tests to fill that gap? |
|
Sure, I was not sure why the build was failing but let me check more on it. |
Signed-off-by: Utkarsh Agarwal <[email protected]>
|
The build failure was because of missing test for |
|
@Utkarsh-Aga , thank you for this contribution! |
Description
Added the following type conversions -
Issues Resolved
Resolves #4120
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.