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

ARQ-2041 provide own CDIProvider #30

Merged
merged 1 commit into from Sep 1, 2016
Merged

Conversation

tremes
Copy link
Contributor

@tremes tremes commented Aug 31, 2016

This change is Reviewable

@bartoszmajsak
Copy link
Member

bartoszmajsak commented Aug 31, 2016

Related issue: https://issues.jboss.org/browse/ARQ-2041

Can you document why is it important and how to use it as a part of this PR?

Ideally we could have a ftest module (which is not eventually deployed to Maven Central) which can be used as a reference on how to you can use your custom provider.

@johnament
Copy link
Member

I can add a ftest module. It'll actually help with my concerns for #29 . This PR is important, if I'm understanding Martin, we run into an issue w/ Arquillian & Weld SE where stuff mostly works, but there's some caveats. The class originally referenced is deprecated, and the configuration provided now will correctly use the newly implemented CDI provider.

So I'm +1 to bring this in, and would happily add the ftest module if Tomas doesn't have time.

@tremes
Copy link
Contributor Author

tremes commented Sep 1, 2016

This implementation of CDIProvider is necessary when you want to call CDI.current() in your code and there is already test (https://github.com/arquillian/arquillian-container-weld/blob/master/src/test/java/org/jboss/arquillian/container/weld/embedded/WeldEmbeddedCDIProviderTest.java) for this. So this is basically internal stuff of this container.
Any suggestions where I should document it? I am not sure what do you want to add to this ftest module (sounds like functional tests) so you can share your ideas guys. :-)

@johnament
Copy link
Member

ftests usually serve as demos to show how a user may use it. Its not really relevant here, since you're basically fixing some broken behavior, e.g. the current container assumes to be a WAR file when it actually may not be.

@bartoszmajsak
Copy link
Member

Then I misunderstood. I thought that by introducing this SPI we are opening up for extensions.

ftest (functional tests) are not necessarily simple examples - they are obviously demonstrating how to use given functionality, but also verify that stuff works end-to-end.

@tremes
Copy link
Contributor Author

tremes commented Sep 1, 2016

Ok. So I think ftest module doesn't make sense here. @johnament is #31 really needed then?

@johnament
Copy link
Member

Yes, absolutely #31 is needed. If you think we should have a separate ticket for better test strategies, thats fine. We should have some end to end tests against the running container that make sure its working as expected and supports an appropriate level of functionality.

@johnament johnament merged commit 7123b78 into arquillian:master Sep 1, 2016
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.

None yet

3 participants