ffang commented on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-856891087
Hi @aldettinger ,
Thanks for the feedback, and please see my comments inline
> Thanks for contribution @ffang. As far as I understand, it's all about
testing the case where JAXB annotation support is configured on the whole
context. So, there is a difficulty because we have tests sharing a single
context that we want to be configured differently given the test we are
currently executing.
>
> If my understanding is correct, please find questions below:
>
> 1. Is it really needed to test this case in camel-quarkus ? I think
the test is already covered in camel, is there anything specific to
quarkus/graal in this situation ?
I just use the conservative way to add more tests, without assuming anything.
And this actually helps us find a issue in camel-quarkus native mode,
this line
```
context.getGlobalOptions().put(JacksonConstants.TYPE_CONVERTER_MODULE_CLASS_NAMES,
JaxbAnnotationModule.class.getName());
```
Fasterxml Jackson uses reflection way to init JaxbAnnotationModule, so we
need to add a ReflectiveClassBuildItem in camel-quarkus jackson extension
>
> 2. Couldn't we face situations where 2 JacksonJsonResource methods are
called concurrently ? And then global options could be applied during another
test as the context is not synchronized in other methods ?
>
Here the problem isn't the global options, but how camel-jackson
JacksonTypeConverters works
```
@Converter(fallback = true)
public <T> T convertTo(Class<T> type, Exchange exchange, Object value,
TypeConverterRegistry registry) throws Exception {
// only do this if enabled (disabled by default)
if (!init && exchange != null) {
```
It only init once and after that all new global options can't take effect(I
think this is OK in real world camel usage, our test just share the same camel
context which isn't very reasonable). That's why in the PR I just re-create the
convert method to ensure any new global options before it can take effect.
And I think I put all global options in sync block already.
> 3. When testing scenarios involving globalOptions(), would it be
better to have a new CamelContext for each subset of global options ? Maybe
another itest project or something like a quarkus test profile ?
Definitely you are correct, a new camelcontext for each testcase should be
more isolated and clear. but I don't know how to do it easily in camel-quarkus
test framework. So feel free to shed light or even point me an existing example.
Freeman
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
