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

MP OpenAPI - Adding CDI integration tests #111

Merged
merged 1 commit into from
Dec 18, 2019

Conversation

fabiobrz
Copy link
Member

Integration of a peculiar CDI test case, see https://javaee.github.io/tutorial/jaxrs-advanced004.html

Please make sure your PR meets the following requirements:

  • Pull Request contains a description of the changes
  • Pull Request does not include fixes for multiple issues/topics
  • Code is formatted, imports ordered, code compiles and tests are passing
  • N/A Link to the passing job is provided (No job can currently handle the subsequent build of projects needed)
  • Code is self-descriptive and/or documented
  • Description of the tests scenarios is included (see Update PR template to include TPG stuff #46)

*/
@RunWith(Arquillian.class)
@RunAsClient
public class CdiIntegrationTest {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiobrz Just a small note: do you prefer writing CDIIntegrationTest or CdiIntegrationTest? What is better in your opinion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zhantemirov if it is to me, I'd go for how is it now. But please point me to any resource that can help having a clearer idea in case you're referring to some specific naming convention.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiobrz you were right keeping CdiIntegrationTest way of writing. Because we can encounter with "HTTP SID or HTTPS ID" situation. So let's keep it that way as it is. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you too, @zhantemirov. I'll resolve this conversation following your consideration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer CDI over Cdi, I looked at WF and Quarkus code and both CDI and Cdi are used, Quarkus uses mainly CDI.

CDIIntegrationTest looks weird, what about IntegrationWithCDITest ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rsvoboda, thanks for looking at WF for a comparison. Fixing according to your suggestion that also addresses @zhantemirov concerns about naming conventions, IMHO.

Copy link

@zhantemirov zhantemirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving and also would like to mention using test description annotations - very useful for a reviewer, thanks!

@rsvoboda
Copy link
Member

@zhantemirov please add " ready for final review " label

@rsvoboda
Copy link
Member

Created #113 for the future discussion. CdiIntegrationTest is an example of "a lot of Assert.assertXYZ checks in one test". This doesn't block merge of this PR.

@rsvoboda rsvoboda merged commit 88cf68e into jboss-eap-qe:master Dec 18, 2019
@fabiobrz
Copy link
Member Author

Created #113 for the future discussion. CdiIntegrationTest is an example of "a lot of Assert.assertXYZ checks in one test". This doesn't block merge of this PR.

Yes, I get your point and definitely going to follow the discussion through the issue you created. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants