You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/06/07 20:17:21 UTC
[GitHub] [camel-quarkus] ffang opened a new pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
ffang opened a new pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739
<!-- Uncomment and fill this section if your PR is not trivial
[ ] An issue should be filed for the change unless this is a trivial change (fixing a typo or similar). One issue should ideally be fixed by not more than one commit and the other way round, each commit should fix just one issue, without pulling in other changes.
[ ] Each commit in the pull request should have a meaningful and properly spelled subject line and body. Copying the title of the associated issue is typically enough. Please include the issue number in the commit message prefixed by #.
[ ] The pull request description should explain what the pull request does, how, and why. If the info is available in the associated issue or some other external document, a link is enough.
[ ] Phrases like Fix #<issueNumber> or Fixes #<issueNumber> will auto-close the named issue upon merging the pull request. Using them is typically a good idea.
[ ] Please run mvn process-resources -Pformat (and amend the changes if necessary) before sending the pull request.
[ ] Contributor guide is your good friend: https://camel.apache.org/camel-quarkus/latest/contributor-guide.html
-->
--
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:
users@infra.apache.org
[GitHub] [camel-quarkus] ffang commented on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
ffang commented on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-858617818
> The same kind of questions like @aldettinger asked came to my mind.
>
> > > ```
> > > 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.
>
> Yeah, having a separate itest module for each of the tested converter setups would probably be an overkill (although we have a couple of itest modules for e.g. camel-main). I wonder whether this is not a use case for a `io.quarkus.test.QuarkusUnitTest`. We have such tests e.g. in [Servlet extension](https://github.com/apache/camel-quarkus/tree/main/extensions/servlet/deployment/src/test/java/org/apache/camel/quarkus/component/servlet/test). A big advantage of those is that they are very lightweight and fast. You can have a special context setup per test class. But OTOH, they are executed only in JVM mode. So we maybe need to find a way how to keep a part of the scenario in a proper itest and move the rest to QuarkusUnitTest-style test.
>
> I'd say if the second setup with `context.getGlobalOptions().put(JacksonConstants.TYPE_CONVERTER_MODULE_CLASS_NAMES, JaxbAnnotationModule.class.getName());` uncovered the need for the reflection registration, then that setup should stay ghere and be hardwired for the whole itest module. The first setup (without TYPE_CONVERTER_MODULE_CLASS_NAMES) could then be transformed to a separate `io.quarkus.test.QuarkusUnitTest`.
>
> In that way we'd both have clean and readable tests without needing to configure the context in the middle of the flight and we still cover what we need in native mode. WDYT?
Thanks @ppalaga for the input!
I will take a close look at the tests used in servlet extension and see if we can split some of our testcases this way.
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:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga edited a comment on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
ppalaga edited a comment on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-861538206
Could you please rebase and squash your commits, @ffang so that we can merge straight away once this passes the review?
Single commit per issue makes it much easier to backport.
--
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:
users@infra.apache.org
[GitHub] [camel-quarkus] ffang commented on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
ffang commented on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-861732946
somehow this PR was messed up. Close this one and sent a new PR
https://github.com/apache/camel-quarkus/pull/2790
We can continue the discussion there
--
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:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
aldettinger commented on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-856923967
Ok, great to know that the test is needed and that we caught up another native issue :+1:
Related to point 3, I agree that having tests with a new context could help in such a situation. To my knowledge, there is no example for that in camel-quarkus. The only ideas I would have would be to create a new itest project or maybe investigate if something could be done with [quarkus test profile](https://quarkus.io/guides/getting-started-testing#testing_different_profiles).
I'm not sure about point 2, could you please confirm my understanding ? My understanding would be that we override the fallback converter, then we set the global options, then the converter is initialized on next call to `convertTo()` and from there will not change afterward. So, the overridden fallback converter is sometimes present when other tests are executed. But I guess that's no big deal because other tests are assumed not to use the fallback converter ?
--
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:
users@infra.apache.org
[GitHub] [camel-quarkus] ffang commented on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [camel-quarkus] ffang commented on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
ffang commented on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-861609932
Hi @ppalaga ,
Please find squashed commit here
https://github.com/apache/camel-quarkus/pull/2739/commits/751063196f3a99e07906cf69b512f6ab970392e7
This contains all changes for this issue.
Cheers
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:
users@infra.apache.org
[GitHub] [camel-quarkus] ffang closed pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
ffang closed pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739
--
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:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
aldettinger commented on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-856835768
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 ?
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 ?
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 ?
--
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:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
ppalaga commented on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-857139522
The same kind of questions like @aldettinger asked came to my mind.
> > ```
> > 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.
Yeah, having a separate itest module for each of the tested converter setups would probably be an overkill (although we have a couple of itest modules for e.g. camel-main). I wonder whether this is not a use case for a `io.quarkus.test.QuarkusUnitTest`. We have such tests e.g. in [Servlet extension](https://github.com/apache/camel-quarkus/tree/main/extensions/servlet/deployment/src/test/java/org/apache/camel/quarkus/component/servlet/test). A big advantage of those is that they are very lightweight and fast. You can have a special context setup per test class. But OTOH, they are executed only in JVM mode. So we maybe need to find a way how to keep a part of the scenario in a proper itest and move the rest to QuarkusUnitTest-style test.
I'd say if the second setup with `context.getGlobalOptions().put(JacksonConstants.TYPE_CONVERTER_MODULE_CLASS_NAMES,
JaxbAnnotationModule.class.getName());` uncovered the need for the reflection registration, then that setup should stay ghere and be hardwired for the whole itest module. The first setup (without TYPE_CONVERTER_MODULE_CLASS_NAMES) could then be transformed to a separate `io.quarkus.test.QuarkusUnitTest`.
In that way we'd both have clean and readable tests without needing to configure the context in the middle of the flight and we still cover what we need in native mode. WDYT?
--
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:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on pull request #2739: Fix JSON Jackson jacksonConversionPojo test #2726
Posted by GitBox <gi...@apache.org>.
ppalaga commented on pull request #2739:
URL: https://github.com/apache/camel-quarkus/pull/2739#issuecomment-861538206
Could you please rebase and squash your commits, @ffang so that we can merge straight away once this passes the review?
--
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:
users@infra.apache.org