Closed eliasnogueira closed 1 year ago
Can one of the admins verify this patch?
Amazing, there is a lot of improvement :)
@otaviojava what about the open questions in the PR description?
@eliasnogueira the PR is good enough, IMHO: let's merge it and open issues to solve on different PRs.
@eliasnogueira could you solve the pom.xml conflict?
@eliasnogueira could you solve the pom.xml conflict?
done :-)
@otaviojava I will go through the issues found during the build and adjust this PR
Test improvements
This PR adds test improvements in the
jnosql-communication-core
module.Added
Changed
src/test/java
:@DisplayName
annotationRemoved
ValueTest
class and the test methods were already being verified by theDefaultValueTest
Review before the merge
The following classes have
//TODO
comments and must be discussed before this PR can be merged.ByteValueReaderTest
The test
shouldConvert()
is using thevalueReader
method that, previously, was using theInteger.class
to verify the possible conversions, but it was causing aCastException
. It was changed toByte.class
because it will return the converted value based on a byte value.I believe the
Byte
is the correct class, but I need your guys input byLocalDateTimeReaderTest
When the assertions were changed by the soft ones I had to change the parameters, as assertions methods are comparing always the actual vs expected. Before we had expected vs actual. Without the inversion, this test was throwing
NullPointerExceptions
We need just a quick look at the
shouldConvert()
test to see if the actual objects make sense.OffsetDateTimeReaderTest
The test
shouldConvert()
was using theOffSetDate
class to read, but I believe the correct one should be theOffSetDateTime
class, which also makes sense in the context of this test.Can you guys double-check and see if the
OffSetDateTime
class is the correct one to use?