Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement test for custom exception mappers #5225

Closed

Conversation

AnetaCadova
Copy link
Contributor

@AnetaCadova AnetaCadova commented Aug 25, 2023

No description provided.

@oscerd
Copy link
Contributor

oscerd commented Aug 25, 2023

Please don"t use downstream issue number on upstream repository. There is no reason for that and in terms of community it's not a useful information. Please amend the commit message and the PR title. Thanks.

@aldettinger
Copy link
Contributor

aldettinger commented Aug 25, 2023

Great to see contributions @AnetaCadova 👍 The failing test in ci is not related.

+1 to not using downstream info, ticket number...

Thinking more, the test looks good. However, is it a test for Camel Quarkus ? Like are we testing only a Quarkus feature there ? Should we expect that it's already covered in Quarkus tests ?

@AnetaCadova AnetaCadova force-pushed the test-custom-exception-mappers branch from 8f5576c to 3316790 Compare August 27, 2023 18:38
@AnetaCadova AnetaCadova changed the title [CEQ-5762 test] Implement test for custom exception mappers Implement test for custom exception mappers Aug 27, 2023
@AnetaCadova
Copy link
Contributor Author

@aldettinger sorry for using the downstream issue number, I removed it. And yes, the issue is covered in Quarkus tests: quarkusio/quarkus#32619. The Quarkus test also focuses on the problem with exception mappers occurring in DEV mode. My tests are focused only on the precedence of given types of exceptions, which seems to me as the main exception mappers issue. So I covered it here too.

@aldettinger
Copy link
Contributor

Many thanks for moving the subject forward @AnetaCadova

My tests are focused only on the precedence of given types of exceptions, which seems to me as the main exception mappers issue. So I covered it here too.

So, this specific case is not covered by Quarkus test yet ? Are we testing something specific to camel ? Or could other Quarkus Platform participants be affected ?

Another subject, we generally open prs against main first, tag the pr with backport-2.13x and then do the actual backporting. Could you please change the target branch ?

@vkasala
Copy link

vkasala commented Sep 1, 2023

@aldettinger @AnetaCadova The idea was to test the custome-execption together with camel as that was the primary use case where the original issue in Quarkus appeared.

@AnetaCadova
Copy link
Contributor Author

@vkasala thanks for the clarification, I will add camel context to the test. @aldettinger sorry for the confusion

@aldettinger
Copy link
Contributor

So, on the top of my head this issue was first discovered in a Quarkus app quarkusio/quarkus#7883 and then the same issue was also hit in Camel Quarkus.

So the question is about possibility to test the Quarkus custom-exception handling together with Camel.
@jamesnetherton @ppalaga Is camel involved at all in the context of this pr where custom Quarkus exception mappers are at play ? And so, is it a good test for Quarkus or Camel Quarkus ?

@ppalaga
Copy link
Contributor

ppalaga commented Sep 4, 2023

@ppalaga Is camel involved at all in the context of this pr where custom Quarkus exception mappers are at play ? And so, is it a good test for Quarkus or Camel Quarkus ?

I do not know, sorry.

@jamesnetherton
Copy link
Contributor

Is camel involved at all in the context of this pr where custom Quarkus exception mappers are at play ? And so, is it a good test for Quarkus or Camel Quarkus ?

I guess we should test it in the context of platform-http routes. E.g if there's a custom exception mapper and some exception is thrown or a 404, do we get the expected response. Correct me if I'm wrong, but I think that was the original intent.

Also, integration-tests/vertx is maybe not the best place for such tests. camel-quarkus-vertx is not really related to HTTP.

We either need to create a new itest module for this or find an unintrusive way to introduce it to an existing one.

@aldettinger
Copy link
Contributor

yeah, non intrusive would be difficult.

@AnetaCadova Maybe it could be experimented to create integration-tests/platform-http-custom-exception-mapping from integration-tests/platform-http and see where it leads.

@apache apache deleted a comment from AnetaCadova Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants