#2633 jacksonxml: add test coverage #2635
Conversation
There was a problem hiding this comment.
Thanks, the test coverage is highly appreciated. Except for the two minor comments inline, could you please move the Jackson XML related stuff to separate test classes (JacksonXmlTest and JacksonXmlIT) and to a separate package (e.g. org.apache.camel.quarkus.component.dataformats.jackson.xml) in the test app? That would make it easier for the future readers to understand which parts are testing which extensions.
integration-tests/dataformats-json/src/main/resources/routes/jackson-routes.xml
Outdated
Show resolved
Hide resolved
|
Indeed, splitting more between jackson and jacksonxml would make the code more readable. Concerning jackson features, I was first in the idea that it is the responsibility of quarkus-jackson to test them. But given, we already found an issue with @ JsonView, I wonder if it would make sense to test @ JsonInclude also ? |
There was a problem hiding this comment.
Indeed, splitting more between jackson and jacksonxml would make the code more readable.
Concerning jackson features, I was first in the idea that it is the responsibility of quarkus-jackson to test them. But given, we already found an issue with @ JsonView, I wonder if it would make sense to test @ JsonInclude also ?
I think for camel-quarkus-jackson or camel-quarkus-jacksonxml, if we cover all usecases in camel-jackson and camel-jacksonxml components, we should be good. Back to the @ JsonInclude, only NON_NULL is used in camel related components and this has been covered in the tests I added(the jacksonXmlIncludeNoNull test)
Cheers
Freeman
Thanks for the feedback! Will revise accordingly. |
|
pushed a new commit which I believe addressed all feedback. |
* apache#2633 jacksonxml: add test coverage * address feedback
* apache#2633 jacksonxml: add test coverage * address feedback
No description provided.